-
-
Notifications
You must be signed in to change notification settings - Fork 720
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
🛑 DNM Deserialization: zero-copy merge subframes when possible #5112
Closed
gjoseph92
wants to merge
14
commits into
dask:main
from
gjoseph92:serialization/zero-copy-merge-frames
Closed
🛑 DNM Deserialization: zero-copy merge subframes when possible #5112
gjoseph92
wants to merge
14
commits into
dask:main
from
gjoseph92:serialization/zero-copy-merge-frames
Commits on Jul 23, 2021
-
Configuration menu - View commit details
-
Copy full SHA for 4fc3dae - Browse repository at this point
Copy the full SHA 4fc3daeView commit details -
This is a hack because in real use, I am pretty sure that `frames` will be either all memoryviews or all not, since they'll either be coming from a comm or from a bytestring. But in tests where we call `loads` directly, this may not be the case.
Configuration menu - View commit details
-
Copy full SHA for 1869b18 - Browse repository at this point
Copy the full SHA 1869b18View commit details -
Configuration menu - View commit details
-
Copy full SHA for 75922a6 - Browse repository at this point
Copy the full SHA 75922a6View commit details -
Configuration menu - View commit details
-
Copy full SHA for cf62d30 - Browse repository at this point
Copy the full SHA cf62d30View commit details
Commits on Jul 28, 2021
-
Co-authored-by: crusaderky <[email protected]>
Configuration menu - View commit details
-
Copy full SHA for cc528c0 - Browse repository at this point
Copy the full SHA cc528c0View commit details -
Configuration menu - View commit details
-
Copy full SHA for 934526f - Browse repository at this point
Copy the full SHA 934526fView commit details
Commits on Jul 29, 2021
-
Configuration menu - View commit details
-
Copy full SHA for ee679fd - Browse repository at this point
Copy the full SHA ee679fdView commit details
Commits on Jul 30, 2021
-
Configuration menu - View commit details
-
Copy full SHA for 523957b - Browse repository at this point
Copy the full SHA 523957bView commit details -
Configuration menu - View commit details
-
Copy full SHA for 2885366 - Browse repository at this point
Copy the full SHA 2885366View commit details -
Assert instead of subframe copy
Less useful, but also less complex. Short of writing the frames by hand, I couldn't come up with a test that could even trigger this behavior. I'd feel uncomfortable having that code be untested and only run on what's already an edge case. So if this assert is ever triggered, we can come back and add the zero-copy and test it.
Configuration menu - View commit details
-
Copy full SHA for 39ce2b6 - Browse repository at this point
Copy the full SHA 39ce2b6View commit details -
Configuration menu - View commit details
-
Copy full SHA for 31e0e6e - Browse repository at this point
Copy the full SHA 31e0e6eView commit details -
Revert "Assert instead of subframe copy"
This reverts commit 39ce2b6.
Configuration menu - View commit details
-
Copy full SHA for 6ace0a4 - Browse repository at this point
Copy the full SHA 6ace0a4View commit details
Commits on Jul 31, 2021
-
Changed mind: multiple-memoryview case with copy
A separate `copy_frames` function makes this more readable and easier to test. Also, I came up with a test for this case that's still contrived, but not ridiculously contrived. That said, we don't want this copy to happen. And I'm pretty confident it will never happen with reall comms, because either the whole message is one buffer (TCP), or memoryviews aren't used at all. This mix-and-match only even happens in tests; see 1869b18. So maybe we should stick with the assert as a warning to future developers, so nobody messes this up and it keeps working with a silent performance regression?
Configuration menu - View commit details
-
Copy full SHA for ab6119a - Browse repository at this point
Copy the full SHA ab6119aView commit details -
Changed mind again: back to AssertionError
As noted in ab6119a: I think this error is currently impossible to raise in real use, and we want to keep it that way. We'd like a future test to fail if something causes this case to happen, rather than a silent copy.
Configuration menu - View commit details
-
Copy full SHA for e2da610 - Browse repository at this point
Copy the full SHA e2da610View commit details
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.