[BUGFIX release] Simplify factory instantiation #19088

pzuraq · 2020-08-08T16:06:15Z

Simplifies factory instantiation and object creation in a number of ways:

Removes the side-channeling done to provide the Owner and Factory during initialization. These are instead provided using native Symbols on the props that are provided during create.
Converts to using native symbols wherever possible (and in browsers that support them), so these properties are non-enumerable in general.
Removes the notion of FrameworkClasses that receive the Owner as a constructor param. Any CoreObject registered on the container will now receive the owner as the first parameter.

See the individual commits for more detailed explanations of the changes.

Benchmark results:

Removes `_initFactory` as a method and instead sets the owner factory directly in an INIT_FACTORY_MAP. If the factory being initialized extends from CoreObject, it will pull the factory off immediately and use it. Simplifies passing the owner to CoreObject as well. The owner is now always passed as an injection, then plucked off and passed as the first argument to the constructor. It is not gotten through side-channeling the factory at all.

FrameworkClass limited the owner injection to just a few specific classes, but the brand check required to figure out what those classes were was costly. This removes it and updates to passing the owner to any object that extends from CoreObject.

Converts to native symbol in as many places as we can, and uses the benefits of native symbols non-enumerability in a few places, such as assigning properties OWNER to the instance of CoreObject/EmberObject. A few symbols still need to be enumerable, either because they are passed into mixins (which do not support non-enumerable symbols) or because we use enumerability in other ways. We should look into removing these in the future.

rwjblue · 2020-08-08T17:44:54Z

$ tsc --noEmit
##[error]packages/@ember/-internals/container/lib/container.ts(270,9): error TS6133: 'proxy' is declared but its value is never read.

packages/@ember/-internals/container/lib/container.ts

packages/@ember/-internals/container/tests/container_test.js

packages/@ember/-internals/container/tests/owner_test.js

packages/@ember/-internals/glimmer/lib/renderer.ts

packages/@ember/-internals/metal/lib/events.ts

packages/@ember/-internals/runtime/tests/system/core_object_test.js

packages/@ember/-internals/runtime/lib/system/core_object.js

rwjblue

Changes generally look good to me, only a few remaining concerns from my perspective:

What enumerable properties remain on an object if it operates with the right protocol? Specifically, does Object.keys(EmberObject.create()) return anything, is it platform dependent? I think we need to bring back tests for this, it has been important in the past. IMHO, if we end up increasing the number of own properties this probably can't land in release (e.g. it would at least need to flow through the beta cycle).
Should we consider adding a version of setOwner that does not set LEGACY_OWNER (either another export, or a third argument)? I'd much prefer to never export OWNER and force all users (internal and external) through the same protocol...
I haven't looked at what is up with the tests.

pzuraq · 2020-08-09T16:59:35Z

What enumerable properties remain on an object if it operates with the right protocol? Specifically, does Object.keys(EmberObject.create()) return anything, is it platform dependent? I think we need to bring back tests for this, it has been important in the past. IMHO, if we end up increasing the number of own properties this probably can't land in release (e.g. it would at least need to flow through the beta cycle).

Added a test for this. No enumerable properties are added, on any platform, for EmberObject specifically.

Should we consider adding a version of setOwner that does not set LEGACY_OWNER (either another export, or a third argument)? I'd much prefer to never export OWNER and force all users (internal and external) through the same protocol...

We also need to export the symbols for defining the getter/setter in IE11, which is how IE11 makes these properties non-enumerable on CoreObject. Definitely agree though, we should force everything through the protocol once we drop support for older platforms.

I haven't looked at what is up with the tests.

Fixed them! IE11 test was failing because we defined the non-enumerable getter/setter on EmberObject, not CoreObject, and we always set those properties on CoreObject now (so we don't have to do any checks to see if they're undefined). Moved the getter/setter to CoreObject instead, it makes sense to have them there.

rwjblue · 2020-08-10T16:53:51Z

packages/@ember/-internals/container/lib/container.ts

@@ -470,8 +468,12 @@ function buildInjections(
  typeInjections: Injection[],
  injections: Injection[]
 ): BuildInjectionsResult {
+  let injectionsHash = {};
+
+  injectionsHash[OWNER as any] = container.owner!;


Don't we need both OWNER and LEGACY_OWNER? Specifically, if you were using a non-Ember.CoreObject based class with a static create...

So, if we add LEGACY_OWNER here, it will result in an enumerable LEGACY_OWNER property on Ember.CoreObject unless we delete it or purposefully ignore it, which seems bad.

Given that Symbol works with Object.assign, I think it should be fine in most real use cases to just do this. The only one we've seen that relied on enumerability used setOwner, not the container API.

If we do think we need it though, I think we have to accept that CoreObject will need to do a bit more work to ignore it.

Sadly, I do think we should do this. An idea that @pzuraq came up with in chat was to make this use setOwner (and therefore set LEGACY_OWNER) but also add a set [LEGACY_OWNER]() {} on CoreObject to "black hole" the property (and avoid enumerability).

packages/@ember/-internals/container/tests/container_test.js

rwjblue · 2020-08-10T17:08:29Z

packages/@ember/-internals/metal/lib/events.ts

+
+    let type = typeof method;
+    if (type === 'string' || type === 'symbol') {
+      method = target[method as string] as Function;
    }

-    method.apply(target, params);
+    (method as Function).apply(target, params);


Did we resolve the perf concern here? Should we do this (which is what I think I had suggested/mentioned), or go back to:

if (typeof method === 'string' || typeof method === 'symbol') { method = target[method] as Function; }

packages/@ember/-internals/runtime/tests/system/object/create_test.js

Chris Garrett added 3 commits August 7, 2020 13:00

[REFACTOR] Remove setFrameworkClass

cc432a5

FrameworkClass limited the owner injection to just a few specific classes, but the brand check required to figure out what those classes were was costly. This removes it and updates to passing the owner to any object that extends from CoreObject.

rwjblue requested changes Aug 8, 2020

View reviewed changes

pzuraq force-pushed the refactor/simplify-factory-instantiation branch from 1ad23fe to 60453d0 Compare August 9, 2020 15:43

rwjblue reviewed Aug 9, 2020

View reviewed changes

pzuraq force-pushed the refactor/simplify-factory-instantiation branch from 60453d0 to 29a6513 Compare August 9, 2020 16:23

rwjblue reviewed Aug 10, 2020

View reviewed changes

pzuraq force-pushed the refactor/simplify-factory-instantiation branch from 29a6513 to 035f61b Compare August 10, 2020 17:32

address comments

56495f7

pzuraq force-pushed the refactor/simplify-factory-instantiation branch from 035f61b to 56495f7 Compare August 10, 2020 18:06

rwjblue approved these changes Aug 10, 2020

View reviewed changes

rwjblue merged commit cabe7da into master Aug 10, 2020

rwjblue deleted the refactor/simplify-factory-instantiation branch August 10, 2020 19:45

lifeart mentioned this pull request Aug 12, 2020

Ember 3.20.4 (upstream + perf improvements) krausest/js-framework-benchmark#781

Merged

rwjblue mentioned this pull request Sep 15, 2020

Fix v1.x issue with Ember 3.20.4+. ember-modifier/ember-modifier#61

Merged

rwjblue mentioned this pull request Sep 30, 2020

[FEAT] Implements invokeHelper #19171

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BUGFIX release] Simplify factory instantiation #19088

[BUGFIX release] Simplify factory instantiation #19088

pzuraq commented Aug 8, 2020

rwjblue commented Aug 8, 2020

rwjblue left a comment

pzuraq commented Aug 9, 2020

rwjblue Aug 10, 2020

pzuraq Aug 10, 2020

rwjblue Aug 10, 2020

rwjblue Aug 10, 2020

[BUGFIX release] Simplify factory instantiation #19088

[BUGFIX release] Simplify factory instantiation #19088

Conversation

pzuraq commented Aug 8, 2020

rwjblue commented Aug 8, 2020

rwjblue left a comment

Choose a reason for hiding this comment

pzuraq commented Aug 9, 2020

rwjblue Aug 10, 2020

Choose a reason for hiding this comment

pzuraq Aug 10, 2020

Choose a reason for hiding this comment

rwjblue Aug 10, 2020

Choose a reason for hiding this comment

rwjblue Aug 10, 2020

Choose a reason for hiding this comment