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.
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
support compression for IPC with revamped feature flags #2369
support compression for IPC with revamped feature flags #2369
Changes from 28 commits
e51eca3
ede5115
67e7de5
58488a3
932e381
fcc4f5f
5b0d711
0c61067
9ac4b01
49edfc8
5d974b3
a14e1e1
7f90fb2
4287c0f
45a5389
3bd610d
f7b1803
201de6e
c32f6e1
0b407e8
37504da
e2456f5
5ab5afd
e5d9747
8ab3d39
3b7f94a
21eb68d
443d7fb
3db3d54
bef901d
8d1c50d
ee41c32
76a31c1
c78dd22
c51c8cc
2ed7ce3
257c3b4
4f59de4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just an observation, but this copy is kind of unfortunate (although existed before)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you thinking the alternate is to create a Buffer initially and write this code in terms of
Buffer
rather than&[u8]
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah... Not important for this PR, but it seems unfortunate the amount of memory copying we are doing, especially when the major design goal of the IPC spec is to avoid this 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Filed #2437
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is what gets run if the
ipc_compression
flag is not enabledThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might be misunderstanding this lint, but I don't think it should be firing for this method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I remove it and run clippy like
Clippy tells me:
Which while true in this case, is not true for the actual codec implementation, and they both need to have the same signature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a very daft lint,
&mut Vec<u8>
is not the same as&mut [u8]
, in particular the latter must initialize memory... I can understand&[u8]
vs&Vec<u8>
but the mutability makes a difference... I somewhat assumed clippy wasn't being silly, guess I was wrong 😅