refactor(ext/http) refer to HttpRecord directly using v8::External #20770
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
|
@mmastrac I believe this is ready for review, let me know if anything else is needed. I recommend reviewing commit by commit since the renaming to remove the slab terminology is quite noisy. Log of serve_test with __http_tracing enabled: https://gist.github.com/lrowe/c91d41898156ddbb529a525027f29cf4 |
|
nice! |
aapoalas
reviewed
Oct 3, 2023
|
Great work! This looks exactly like we discussed. Good job on the additional optimizations as well. I looked over it once and didn't see anything that looked out of place, but I'll take another pass. |
mmastrac
reviewed
Oct 3, 2023
|
I may be able to eke out another half a percent or so of performance by pooling Ideally I'd like to get to the point where the HttpRecord lives on the service future and those futures are pooled, but I'm not sure that's quite possible without async drop since the service future might be dropped midway (e.g. on TCP connection closed) but the HttpRecord must live until the JS serve handler completes. |
mmastrac
approved these changes
Oct 3, 2023
hexiaokang
approved these changes
Oct 4, 2023
This was referenced Oct 6, 2023
lucacasonato
previously requested changes
Oct 9, 2023
mmastrac
force-pushed
the
lrowe-http-external
branch
3 times, most recently
from
064e87f
to
393d48b
Compare
…tead of through slabId.
mmastrac
force-pushed
the
lrowe-http-external
branch
from
e3442b8
to
126476c
Compare
mmastrac
dismissed
lucacasonato’s stale review
Addressed via deno_core type-checked externals
|
/bench http[minimal,realistic] |
http[minimal]
http[realistic]
start: id: server: |
mmastrac
added a commit
that referenced
this pull request
Nov 13, 2023
Reuse existing existing allocations for HttpRecord and response HeaderMap where possible. At request end used allocations are returned to the pool and the pool and the pool sized to 1/8th the current number of inflight requests. For http1 hyper will reuse the response HeaderMap for the following request on the connection. Builds upon #20770 --------- Co-authored-by: Matt Mastracci <[email protected]>
mmastrac
added a commit
that referenced
this pull request
Nov 13, 2023
…dy completion (#20822) Use HttpRecord as response body so requests can be tracked all the way to response body completion. This allows Request properties to be accessed while the response body is streaming. Graceful shutdown now awaits a future instead of async spinning waiting for requests to finish. On the minimal benchmark this refactor improves performance an additional 2% over pooling alone for a net 3% increase over the previous deno main branch. Builds upon #20809 and #20770. --------- Co-authored-by: Matt Mastracci <[email protected]>
kt3k
pushed a commit
that referenced
this pull request
Nov 17, 2023
…20770) Makes the JavaScript Request use a v8:External opaque pointer to directly refer to the Rust HttpRecord. The HttpRecord is now reference counted. To avoid leaks the strong count is checked at request completion. Performance seems unchanged on the minimal benchmark. 118614 req/s this branch vs 118564 req/s on main, but variance between runs on my laptop is pretty high. --------- Co-authored-by: Matt Mastracci <[email protected]>
kt3k
pushed a commit
that referenced
this pull request
Nov 17, 2023
Reuse existing existing allocations for HttpRecord and response HeaderMap where possible. At request end used allocations are returned to the pool and the pool and the pool sized to 1/8th the current number of inflight requests. For http1 hyper will reuse the response HeaderMap for the following request on the connection. Builds upon #20770 --------- Co-authored-by: Matt Mastracci <[email protected]>
kt3k
pushed a commit
that referenced
this pull request
Nov 17, 2023
…dy completion (#20822) Use HttpRecord as response body so requests can be tracked all the way to response body completion. This allows Request properties to be accessed while the response body is streaming. Graceful shutdown now awaits a future instead of async spinning waiting for requests to finish. On the minimal benchmark this refactor improves performance an additional 2% over pooling alone for a net 3% increase over the previous deno main branch. Builds upon #20809 and #20770. --------- Co-authored-by: Matt Mastracci <[email protected]>
zifeo
pushed a commit
to metatypedev/deno
that referenced
this pull request
Nov 22, 2023
…enoland#20770) Makes the JavaScript Request use a v8:External opaque pointer to directly refer to the Rust HttpRecord. The HttpRecord is now reference counted. To avoid leaks the strong count is checked at request completion. Performance seems unchanged on the minimal benchmark. 118614 req/s this branch vs 118564 req/s on main, but variance between runs on my laptop is pretty high. --------- Co-authored-by: Matt Mastracci <[email protected]>
zifeo
pushed a commit
to metatypedev/deno
that referenced
this pull request
Nov 22, 2023
…#20809) Reuse existing existing allocations for HttpRecord and response HeaderMap where possible. At request end used allocations are returned to the pool and the pool and the pool sized to 1/8th the current number of inflight requests. For http1 hyper will reuse the response HeaderMap for the following request on the connection. Builds upon denoland#20770 --------- Co-authored-by: Matt Mastracci <[email protected]>
zifeo
pushed a commit
to metatypedev/deno
that referenced
this pull request
Nov 22, 2023
…dy completion (denoland#20822) Use HttpRecord as response body so requests can be tracked all the way to response body completion. This allows Request properties to be accessed while the response body is streaming. Graceful shutdown now awaits a future instead of async spinning waiting for requests to finish. On the minimal benchmark this refactor improves performance an additional 2% over pooling alone for a net 3% increase over the previous deno main branch. Builds upon denoland#20809 and denoland#20770. --------- Co-authored-by: Matt Mastracci <[email protected]>
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.
Makes the JavaScript Request use a v8:External opaque pointer to directly refer to the Rust HttpRecord.
The HttpRecord is now reference counted. To avoid leaks the strong count is checked at request completion.
Performance seems unchanged on the minimal benchmark. 118614 req/s this branch vs 118564 req/s on main, but variance between runs on my laptop is pretty high.