New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reduce scheduler serialization overhead #14249

Merged
merged 1 commit into from Nov 16, 2018

Conversation

Projects
None yet
6 participants
@developit
Copy link
Contributor

developit commented Nov 16, 2018

In the process of switching to MessageChannel, it seems the postMessage call was modified to pass "*" (originally the target origin value from window.postMessage). This actually ends up triggering serialization, whereas passing undefined bypasses.

To save some investigation, passing a Number like 0 still incurs serialization overhead - undefined has special behavior.

Reduce scheduler serialization overhead
In the process of switching to MessageChannel, it seems the postMessage call was modified to pass `"*"` (originally the target origin value from `window.postMessage`). This actually ends up triggering serialization, whereas passing `undefined` bypasses.

To save some investigation, passing a Number like `0` still incurs serialization overhead - `undefined` has special behavior.
@facebook-github-bot

This comment has been minimized.

Copy link

facebook-github-bot commented Nov 16, 2018

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@sebmarkbage

This comment has been minimized.

Copy link
Member

sebmarkbage commented Nov 16, 2018

what about passing zero arguments?

@bvaughn

This comment has been minimized.

Copy link
Contributor

bvaughn commented Nov 16, 2018

what about passing zero arguments?

postMessage errors if you don't provide exactly 1 argument.

@acdlite

This comment has been minimized.

Copy link
Member

acdlite commented Nov 16, 2018

What about null? Would that work? We like to use null if it's intentional.

I'll go ahead and merge, though. Thanks Jason!

@acdlite acdlite merged commit a22fabc into facebook:master Nov 16, 2018

1 check passed

ci/circleci Your tests passed on CircleCI!
Details
@developit

This comment has been minimized.

Copy link
Contributor

developit commented Nov 16, 2018

I admit I hadn't had time to check null. I need to verify with Mathias but I don't think it triggers the bypass. Source is here:
https://cs.chromium.org/chromium/src/third_party/blink/renderer/bindings/core/v8/script_value.h?type=cs&g=0&l=74

@developit developit deleted the developit:patch-1 branch Nov 16, 2018

@sophiebits

This comment has been minimized.

Copy link
Contributor

sophiebits commented Nov 16, 2018

I didn’t think IsEmpty would return true for undefined; was this observably faster?

@sebmarkbage

This comment has been minimized.

Copy link
Member

sebmarkbage commented Nov 16, 2018

@developit For the formality, mind asking to be added to our CLA?

@developit

This comment has been minimized.

Copy link
Contributor

developit commented Nov 17, 2018

Yup, working on it!

@sophiebits yes - consistently 5x faster. I'll set up a proper benchmark.

@sophiebits

This comment has been minimized.

Copy link
Contributor

sophiebits commented Nov 17, 2018

:o

@developit

This comment has been minimized.

Copy link
Contributor

developit commented Nov 27, 2018

Update on the CLA - I was told to just sign as an individual, which I've just done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment