Various MessageHandler improvements when using Streams
#11107
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
…rc/display/api.js`, with arrow functions instead
…s, in `MessageHandler`, when using Streams
With PR 11069 we're now using Streams for OperatorList parsing (in addition to just TextContent parsing), which brings the nice benefit of being able to easily abort parsing on the worker-thread thus saving resources.
However, since we're now creating many more `ReadableStream` there appears to be a tiny bit more overhead because of it (giving ~1% slower runtime of `browsertest` on the bots). In this case we're just going to have to accept such a small regression, since the benefits of using Streams clearly outweighs it.
What we *can* do here, is to try and make the Streams part of the `MessageHandler` implementation slightly more efficient by e.g. removing unnecessary function calls (which has been helpful in other parts of the code-base). To that end, this patch makes the following changes:
- Actually support `transfers` in `MessageHandler.sendWithStream`, since the parameter was being ignored.
- Inline the `sendStreamRequest`/`sendStreamResponse` helper functions at their respective call-sites. Obviously this causes some amount of code duplication, however I still think this change seems reasonable since for each call-site:
- It avoids making one unnecessary function call.
- It avoids allocating one temporary object.
- It avoids sending, and thus structure clone, various undefined object properties.
- Inline objects in the `MessageHandler.{send, sendWithPromise}` methods.
- Finally, directly call `comObj.postMessage` in various methods when `transfers` are *not* present, rather than calling `MessageHandler.postMessage`, to further reduce the amount of function calls.
… from a String to a Number Given that the `stream` property is an internal implementation detail, changing its type shouldn't be a problem. By using Numbers instead, we can avoid unnecessary String allocations when creating/processing Streams.
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.67.70.0:8877/b991284c39df3cb/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.215.176.217:8877/c2e9d3ed27da7ef/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/b991284c39df3cb/output.txt Total script time: 17.57 mins
|
From: Bot.io (Windows)SuccessFull output at http://54.215.176.217:8877/c2e9d3ed27da7ef/output.txt Total script time: 26.20 mins
|
Snuffleupagus
force-pushed
the
MessageHandler-postMessage
branch
2 times, most recently
from
4d57b75
to
da49cee
Compare
…son` instead, in `src/shared/message_handler.js` Since `wrapReason` and `makeReasonSerializable` are essentially functionally equivalent it doesn't seem necessary to keep both of them around, especially when `makeReasonSerializable` only has a *single* call-site.
Snuffleupagus
force-pushed
the
MessageHandler-postMessage
branch
from
da49cee
to
f71ea2d
Compare
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.67.70.0:8877/887e87db5f13e54/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.215.176.217:8877/bf777a90076d688/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/887e87db5f13e54/output.txt Total script time: 17.61 mins
|
From: Bot.io (Windows)SuccessFull output at http://54.215.176.217:8877/bf777a90076d688/output.txt Total script time: 26.25 mins
|
timvandermeij
approved these changes
Aug 30, 2019
|
Nice refactoring! |
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.
Please refer to the individual commit messages for additional details.