refactor(core/shared_queue): overflow_response overwrite bugfix & improved shared queue overflow performance #9433
Conversation
There was a problem hiding this comment.
I reviewed the shared queue message passing code (js <-> rs).
- The core.js and shared_queue.rs files only include aesthetic changes for better readability and also more similar code between both files.
While I appreciate the work, these changes are highly subjective and are only adding to diff lines; I'd prefer to discuss them after fixing actual bugs.
- The have_unpolled_ops variable in runtime.rs was changed to not be encapsulated by a Cell<>, as this is not needed.
👍
- In runtime.rs the overflow_response logic was changed, now ALL async responces are always polled in poll_pending_ops and returned in an Vec, this also fixes a bug where the value in overflow_response could be overwritten by the second loop. Also both the request to check the shared_queue and the overflow data are send in one v8 function call now.
These changes are in the hot code path and I'm a bit worried about creating a lot of vectors, but let's verify on the benchmarks.
Some changes might be a bit opinionated, I'm open for any adaptations that are still needed.
No performance degradation is observed in the benchmarks. In case the workload overflows the queue a lot, this implementation will probably be more performant as all async responses can be transfered from rs to js in 1 function call (maybe a test-case is needed here if for you this does not make sense).
Yes, a bench case would be perfect as there's no benchmark for overflown responses; take a look at cli/bench/main.rs for example benchmarks.
|
Ok, I reverted some of the changes and added an extra benchmark. I propose that if the changes can be merged I create a new pull request that includes the changes done to core.js and shared_queue.rs, so these can be further discussed like you mentioned. |
inteon
changed the title
…eue overflow performance have_unpolled_ops: remove unneeded cell + set to false once structure field reordering (grouped by functional use) & branching readability improvement allow for multiple messages in overflow, also combining overflow call with shared buffer call format fix revert aesthetic changes add overflow benchmark format + add lint exception + add copyright removed unnecessary loop in test added bug unittest Merge branch 'master' of https://github.com/denoland/deno into code_review
There was a problem hiding this comment.
LGTM, thanks @inteon. As stated previously this change is in the hot path, so I'll be watching benchmarks after landing this PR and might have to revert if performance deteriorates.
I reviewed the shared queue message passing code (js <-> rs).
No performance degradation is observed in the benchmarks. In case the workload overflows the queue a lot, this implementation will probably be more performant as all async responses can be transfered from rs to js in 1 function call (maybe a test-case is needed here if for you this does not make sense).