[BUGFIX release] Avoid re-freezing already frozen objects. #15312
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
stefanpenner
approved these changes
May 31, 2017
mixonic
reviewed
May 31, 2017
| @@ -38,6 +38,19 @@ export const UPDATE = symbol('UPDATE'); | |||
|
|
|||
| export { NULL_REFERENCE, UNDEFINED_REFERENCE } from '@glimmer/runtime'; | |||
|
|
|||
| let maybeFreeze = () => {}; | |||
| if (DEBUG) { | |||
| maybeFreeze = (obj) => { | |||
There was a problem hiding this comment.
Can you add a note here regarding intended usage? I notice you're left the if (DEBUG) { statements even though the function is a noop when !DEBUG and I presume it is for DCE to do its thing. A note here might help a future traveller from thinking they can rely on the noop in non-debug.
There was a problem hiding this comment.
Sounds good. And yes, that is why I left the if guards.
When a helper is invoked we pass in the arguments, those arguments are frozen (via Object.freeze) so that helpers can't mutate them (and cause issues in the rendering engine itself). When a helper doesn't have hash arguments, we use a shared `EMPTY_HASH` empty object to avoid allocating a bunch of empty objects for no reason (we do roughly the same thing when no positional params are passed). Since these objects are shared they are being frozen over and over again (throughout the lifetime of the running application). Turns out that there is what we think is almost certainly a bug in Chrome, where re-freezing the same object many times starts taking significantly more time upon each freeze attempt. This change introduces a guard to ensure we do not re-freeze repeatedly. Thanks to @krisselden for identifying the root cause and reporting the upstream Chrome bug: https://bugs.chromium.org/p/v8/issues/detail?id=6450
|
@krisselden - thanks for tracing down the root cause - when I posted #15290 I didn't specify which browser we were on - didn't occur to me that the browser itself was the issue (though I guess it was obvious from the profiler...) |
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.
When a helper is invoked we pass in the arguments, those arguments are frozen (via Object.freeze) so that helpers can't mutate them (and cause issues in the rendering engine itself). When a helper doesn't have hash arguments, we use a shared
EMPTY_HASHempty object to avoid allocating a bunch of empty objects for no reason (we do roughly the same thing when no positional params are passed). Since these objects are shared they are being frozen over and over again (throughout the lifetime of the running application). Turns out that there is what we think is almost certainly a bug in Chrome, where re-freezing the same object many times starts taking significantly more time upon each freeze attempt.This change introduces a guard to ensure we do not re-freeze repeatedly.
Thanks to @krisselden for identifying the root cause.
Fixes #15290