Use a single "message" event listener to dispatch received messages #649
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.
We make heavy use of Comlink for our WebWorker communication. On some recent profiling traces I noticed a hot-path on the "addEventListener" line.
While its hard to say exactly how the event listeners are stored, most implementations I've seen for such interfaces involve storing them in an array and removing the listener equally requires adjusting an array. The existing logic also creates two closures - one for the Promise (which is unavoidable from what I can tell), and another for the event listener. As it was suggested in #647 there is a potential for improvement by using a single "message" event listener and doing the dispatch manually by looking up the resolve functions in a Map.
This definitely reduces the runtime cost of the
requestResponseMessage
call itself - though there's still cost tonew Promise
and the closure. There's a shifted cost to the ID lookup on the map though in my profiling that did not show up as a hot path item.Conceptually using a map for the lookup and one handler seems like it should perform better but if folks have some ideas on how to more robustly benchmark this PR that would be helpful.
@josephrocca you filed #647 so maybe you have some thoughts?
Fixes: #647
TODO: