Remove control slice from ops #6048
Merged
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
ry
reviewed
Jun 3, 2020
I think the benchmarks should go together with frequency analysis; I'll probably do that later today or tomorrow. |
bartlomieju
added
cli
Benchmarkszero_copy length counts for different programsFetch remote images and store them to diskInitial fetchCheck for new images to fetch (none added)Single fetch and display processed dataOak's getting started returning
|
piscisaureus
reviewed
Jun 5, 2020
piscisaureus
reviewed
Jun 5, 2020
piscisaureus
reviewed
Jun 5, 2020
There was a problem hiding this comment.
LGTM.
I think we should rename ZeroCopyBuf to JSBuf or something because it is a bit misleading now (doesn't have to be done in this patch).
Also we should probably document that ops shouldn't clone and store zero_copy[0] because it might be overwritten immediately.
@ry PTAL
|
|
|
master: this branch try 1 this branch try 2 |
|
Also pretty wonky. Node and Hyper differ a lot between the runs :/ |
ry
previously requested changes
Jun 11, 2020
There was a problem hiding this comment.
@SyrupThinker We have concerns about the correctness of this patch in the way the control buffer gets rewritten on the shared buffer. I'm making this patch as "Request changes" so it doesn't accidentally get merged.
I suspect we can come up with a test case that introduces errors: modify the control buf during an async op, while there are other async ops happening.
|
From Discord chat:
|
|
@SyrupThinker Can you please merge with master? Note there's been a recent patch that introduced the |
|
What's the status on this? |
bartlomieju
previously requested changes
Jul 7, 2020
There was a problem hiding this comment.
Marking as "request changes" because in the current state this PR introduces panics when Deno.core.dispatch() is called without at least two arguments
v1.1.3
$ deno
Deno 1.1.3
exit using ctrl+d or close()
> const a = Deno.core.dispatch(10)
undefined
> new TextDecoder().decode(a)
{"err":{"message":"EOF while parsing a value at line 1 column 0","kind":17},"promiseId":null}
this PR
target/debug/deno
Deno 1.1.2
exit using ctrl+d or close()
> const a = Deno.core.dispatch(10)
thread 'main' panicked at 'index out of bounds: the len is 0 but the index is 0', cli/state.rs:107:32
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
fatal runtime error: failed to initiate panic, error 5
[1] 21816 abort target/debug/deno
@SyrupThinker I think there should be some validation added in core/bindings.rs::send that there's at least one zero copy buffer passed or some JS error returned informing that too few arguments were passed.
That seems like an arbitrary restriction to me. If my op just changes the system state and doesn't take parameters why should I be forced to pass an argument anyway? |
|
@SyrupThinker I apologize for the delay in landing this patch. It's a very central change so we have to be careful. I will land this today. I want to see its effects on the benchmarks. Potentially it will be reverted if we see some negative side effects. Potentially we'll just land some fixes for the issue Bartek brought up. Bartek, it's a good point but let's just land this anyway. Just waiting for the last few commits I landed to go green. Then I will merge this. |
Thats what the last commit did ;) |
ry
approved these changes
Jul 8, 2020
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.
Observation
The control slice is special cased in the dispatch interface despite it being treated similar to zero copy buffers internally.
Does the interface benefit from this distinction, or should it be removed for simplicity?
Points that need to be addressed in the implementation of this PR
DenoCoreinterface now has two identical functionsdispatchandsendOne could be removed.
Is the distinction of control and data bytes still appropriate for theMetrics? The PR currently merges them. This changes the stable public interface, so it might be undesired.How manyZeroCopyBufs should be stack allocated?I'm currently planning on measuring the frequency of buffer counts for different workloads to determine this.