prefetch may select the wrong memory segment for multi-segment slices #14109
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
uschindler
approved these changes
Jan 7, 2025
There was a problem hiding this comment.
Looks fine. Thus is consistent to all other methods in subclasses.
Sorry for this code complexity, it's way too easy to add new methods but forget to look at code of methods. You're not the first one that broke this. But with normal read methods it's causing test failures, but prefetch just works.
Code is consistent so keep it like it is. The trap is there but when dealing with the IO layer you've to be careful.
jpountz
approved these changes
Jan 7, 2025
ChrisHegarty
added a commit
that referenced
this pull request
Jan 8, 2025
…#14109) This commit fixes a bug where by prefetch may select the wrong memory segment for multi-segment slices. The issue was discovered when debugging a large test scenario, where the index input was backed by several memory segments. When sliced, a multi-segment index input uses an offset into the initial memory segment. This offset should be added to the prefetch offset to determine the absolute offset.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit fixes a bug where by prefetch may select the wrong memory segment for multi-segment slices.
The issue was discovered when debugging a large test scenario, where the index input was backed by several memory segments. When sliced, a multi-segment index input uses an offset into the initial memory segment. This offset should be added to the prefetch offset to determine the absolute offset. The unfortunate part is that the bounds checks need to either be moved to the subclasses, or somehow coerced to look at segment byteSizes rather than length. I found the former to be more straightforward.