Skip to content

Modernize Cursor API #596

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Apr 18, 2025
Merged

Conversation

frankmcsherry
Copy link
Member

The Cursor API has historically relied on functions foo_valid and foo, so that one could independently validate a thing and without validation access it. In practice this results in two expensive calls, rather than one get_foo call that returns an option. Mostly, we see this when there are more complex containers that need to track down the destination of a reference, as opposed to a simple slice access.

The PR encourages more use of get_foo by removing their default implementations, adds BatchContainer::get, and .. somewhat unrelatedly updates the CursorList internals, following and extended #595.

Potentially we should add in default implementations of foo and foo_valid, but no need to do so at the moment, and seemingly all of our cursor implementations would like the manual implementation of these as well (there are moments where it helps to call them directly, and so it's hard to deprecate them completely).

@frankmcsherry frankmcsherry requested a review from antiguru April 17, 2025 20:50
Copy link
Member

@antiguru antiguru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thank you! You could add more inline annotations, but we can also add them later if we notice they might be needed. I wonder if we should provide a default implementation for get or not, and if it's not too much effort, and they're different, I'd lean towards not providing one. It's a breaking change anyway, so we don't have to worry about breaking more APIs.

@frankmcsherry
Copy link
Member Author

I added a get implementation for Vec<T> to avoid the default implementation, but .. at the moment at least forcing inlining and removing defaults seem out of sync with the surrounding code. Batch container implementors can put those annotations on their own implementations if they desire, but e.g. we don't do it for the BatchContainer::{index, len} implementations for Vec<T>. Happiest deferring that until some data-driven moment.

@frankmcsherry frankmcsherry merged commit b27c8ab into TimelyDataflow:master Apr 18, 2025
7 checks passed
@github-actions github-actions bot mentioned this pull request Apr 18, 2025
@frankmcsherry frankmcsherry deleted the cursor_api branch April 18, 2025 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants