Skip to content

Commit

Permalink
[BUGFIX release] Avoid re-freezing already frozen objects.
Browse files Browse the repository at this point in the history
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
  • Loading branch information
rwjblue committed Jun 1, 2017
1 parent 5291c74 commit 567b1d7
Showing 1 changed file with 22 additions and 12 deletions.
34 changes: 22 additions & 12 deletions packages/ember-glimmer/lib/utils/references.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,22 @@ export const UPDATE = symbol('UPDATE');

export { NULL_REFERENCE, UNDEFINED_REFERENCE } from '@glimmer/runtime';

let maybeFreeze;
if (DEBUG) {
// gaurding this in a DEBUG gaurd (as well as all invocations)
// so that it is properly stripped during the minification's
// dead code elimination
maybeFreeze = (obj) => {
// re-freezing an already frozen object introduces a significant
// performance penalty on Chrome (tested through 59).
//
// See: https://bugs.chromium.org/p/v8/issues/detail?id=6450
if (!Object.isFrozen(obj) && HAS_NATIVE_WEAKMAP) {
Object.freeze(obj);
}
}
}

// @abstract
// @implements PathReference
class EmberPathReference {
Expand Down Expand Up @@ -296,10 +312,8 @@ export class SimpleHelperReference extends CachedReference {
let namedValue = named.value();

if (DEBUG) {
if (HAS_NATIVE_WEAKMAP) {
Object.freeze(positionalValue);
Object.freeze(namedValue);
}
maybeFreeze(positionalValue);
maybeFreeze(namedValue);
}

let result = helper(positionalValue, namedValue);
Expand Down Expand Up @@ -333,10 +347,8 @@ export class SimpleHelperReference extends CachedReference {
let namedValue = named.value();

if (DEBUG) {
if (HAS_NATIVE_WEAKMAP) {
Object.freeze(positionalValue);
Object.freeze(namedValue);
}
maybeFreeze(positionalValue);
maybeFreeze(namedValue);
}

return helper(positionalValue, namedValue);
Expand Down Expand Up @@ -365,10 +377,8 @@ export class ClassBasedHelperReference extends CachedReference {
let namedValue = named.value();

if (DEBUG) {
if (HAS_NATIVE_WEAKMAP) {
Object.freeze(positionalValue);
Object.freeze(namedValue);
}
maybeFreeze(positionalValue);
maybeFreeze(namedValue);
}

return instance.compute(positionalValue, namedValue);
Expand Down

0 comments on commit 567b1d7

Please sign in to comment.