Refactor Meta and object destruction tracking. #14359

rwjblue · 2016-09-26T20:33:21Z

Moves the isDestroying and isDestroyed flags into meta
Prevents recreating meta while destroying the object (was happening
during chain node removal)
Allows reading values/caches after object destruction, but triggers an
assertion if non-readable meta methods are called after the object is
destroyed
Avoids doing extra work when unwatching keys/paths for an object that
is also destroyed

stefanpenner · 2016-09-26T20:38:12Z

packages/ember-metal/lib/property_events.js

  }

-  if (obj.isDestroying) { return; }
+  if (m && m.sourceDestroying) { return; }


i believe we may also bail of m is explicity null?

Ya we can, but we don't ever set it to null manually anymore (used to be nulled out in deleteMeta).

krisselden · 2016-09-26T20:48:21Z

packages/ember-metal/lib/meta.js

+  this._isDestroyed = false;
+
+  // keep the objects destruction state
+  this.sourceDestroying = false;


Should we just have a flags field?

I can do that.

krisselden · 2016-09-26T20:49:52Z

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

+    },
+
+    set(value) {
+      // prevent setting while applying mixins


Why do we allow this to be set?

The guard here is to prevent an issue in our mixin apply code (we set the value before calling the descriptor#setup so the value being guarded against is this descriptor itself).

In general, I don't care about allowing this to be set outside of destroy and I can change this to throw if not being set by the quirk/bug in defineProperty mentioned above.

krisselden · 2016-09-26T20:50:48Z

packages/ember-metal/lib/meta.js

 };

+Meta.prototype.destroy = function() {
+  // clean up chain watchers


We should do chain watchers cleanup here

It is being done when this is called in ember-metal/watching's destroy function (which is the only place this is being called). I can move it here though if you'd prefer.

stefanpenner · 2016-09-26T21:59:06Z

@krisselden thoughts on us getting clever \w a bitmask for these flags?

rwjblue · 2016-09-26T22:03:44Z

@stefanpenner - Ya, that is what he was meaning in #14359 (comment) (and he DM'ed to explain).

stefanpenner · 2016-09-26T22:04:24Z

@stefanpenner - Ya, that is what he was meaning in #14359 (comment) (and he DM'ed to explain).

Ah, yup that makes sense.

krisselden · 2016-09-27T00:53:46Z

@stefanpenner it isn't just getting clever, it will be only one IC stub per read/write and places that check both isDestroying and isDestroyed can do it with one test.

stefanpenner · 2016-09-27T00:57:12Z

@stefanpenner it isn't just getting clever, it will be only one IC stub per read/write and places that check both isDestroying and isDestroyed can do it with one test.

I wasn't implying "clever" negatively, I think in this situation it makes sense but clever it is.

rwjblue · 2016-09-27T02:03:10Z

Updated:

Uses bitwise logic to track multiple flags in a single field
Moves chain node watcher cleanup into Meta#destroy
Throw an error when someone manually sets isDestroyed / isDestroying

Ready for another round of reviews...

stefanpenner · 2016-09-27T02:21:06Z

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

+  isDestroying: descriptor({
+    get() {
+      let m = meta(this);
+      return (m._flags & SOURCE_DESTROYING) !== 0;


can we hide the bitmasks behind functions?m.isSourceDestroying(); I believe it should come at little or no overhead, but be a-bit more maintainable.

Petka of bluebird fame took this approach, because those methods tend to inline quite nicely. Nice related blog post -> https://reaktor.com/blog/javascript-performance-fundamentals-make-bluebird-fast/

@stefanpenner - Yeah, I started with that but then switched to using the flags directly in the source after discussing with @krisselden. I have swapped back to using methods for now (it is much easier to grok for those of us that don't understand the bitwise operations at a glance), but we need to confirm with @krisselden that this is not going to negatively impact the IC hit/miss stuff he has been investigating lately.

Maybe I'm just old so it looks familiar but I don't find if (m._flags & SOURCE_DESTROYING) { unreadable or confusing.

Same, same ... not sure it's worth it to take a hard line against bitmasks that aren't even publicly exposed.

rwjblue · 2016-09-27T10:29:51Z

@stefanpenner / @krisselden - Updated again. Exposes helper methods for the flags, and uses them throughout the source. Ready for another review...

krisselden · 2016-09-27T15:43:20Z

packages/ember-metal/lib/meta.js

+      if (node._watching) {
+        nodeObject = node._object;
+        if (nodeObject) {
+          removeChainWatcher(nodeObject, node._key, node);


can we pass in meta here?

nevermind I realize this is the foreign object pointing at us

also, didn't we talk about not doing this if the object point at us was destroying or destroyed? the cycle doesn't matter the other object isn't going to be long lived.

krisselden · 2016-09-27T15:54:10Z

@rwjblue / @stefanpenner should we black hole sets after destroy? It can hide bugs and still fail anything but it also causes a lot of guards.

stefanpenner · 2016-09-27T15:24:49Z

packages/ember-metal/lib/meta.js

  this._chains = undefined;
  this._tag = undefined;

+  // initial value for all flags right now is false


it help future travelers could we also make a comment here describing the layout of this bitfield?

dgeb · 2016-09-27T16:12:25Z

packages/ember-metal/lib/property_events.js


-function notifyBeforeObservers(obj, keyName) {
-  if (obj.isDestroying) { return; }
+function notifyBeforeObservers(obj, keyName, m) {


how about meta (instead of m) for consistency with dependentKeysDidChange and dependentKeysWillChange?

dgeb · 2016-09-27T16:13:15Z

packages/ember-metal/lib/property_events.js


-function notifyObservers(obj, keyName) {
-  if (obj.isDestroying) { return; }
+function notifyObservers(obj, keyName, m) {


again m -> meta

rwjblue · 2016-09-27T17:37:54Z

OK, updated again:

Rename single char variables to better names
Prevent cleaning up chain nodes when both current object and foreign object are being destroyed
Add comment describing flags

krisselden · 2016-09-27T17:46:56Z

packages/ember-metal/lib/meta.js

+const NODE_STACK = [];
+
+Meta.prototype.destroy = function() {
+  // remove chainWatchers to remove circular references that would prevent GC


destroy should be idempotent, should we not check the meta destroyed flag here and set it?

Great point, updated to noop if isMetaDestroying is already set.

krisselden · 2016-09-27T17:48:22Z

packages/ember-metal/lib/meta.js

+        if (nodeObject) {
+          let foreignMeta = peekMeta(nodeObject);
+          // avoid cleaning up chain watchers when both current and
+          // foreign objects are being destroyed


just comment here that marking a source as destroying means you are expected to not be holding onto a reference to the source from anything reachable.

Great point! Updated...

krisselden · 2016-09-27T17:53:39Z

packages/ember-metal/lib/meta.js

+
+function buildFlagAccessors(name, flag, Meta) {
+  let capitalized = capitalize(name);
+  Meta.prototype['is' + capitalized] = function() {


lets just do normal methods here please

krisselden · 2016-09-27T17:54:56Z

packages/ember-metal/lib/meta.js

+    this._flags |= flag;
+  };
+
+  Meta.prototype['unset' + capitalized] = function() {


is this just for completeness? do we currently unset any flags?

Yes, I just did it for completeness. Will remove.

* Moves the `isDestroying` and `isDestroyed` flags into meta * Prevents recreating meta _while_ destroying the object (was happening during chain node removal) * Allows reading values/caches after object destruction, but triggers an assertion if non-readable meta methods are called after the object is destroyed * Avoids doing extra work when unwatching keys/paths for an object that is also destroyed * Use bitwise flagging instead of many boolean properties for meta, and expose helpful methods to interact with them * Move chains cleanup to `Meta#destroy`.

stefanpenner reviewed Sep 26, 2016

View reviewed changes

krisselden reviewed Sep 26, 2016

View reviewed changes

rwjblue force-pushed the move-is-destroyed-flags branch from 4feb560 to 3624127 Compare September 27, 2016 02:00

stefanpenner reviewed Sep 27, 2016

View reviewed changes

rwjblue force-pushed the move-is-destroyed-flags branch from 3624127 to 2dc9334 Compare September 27, 2016 10:26

krisselden reviewed Sep 27, 2016

View reviewed changes

stefanpenner reviewed Sep 27, 2016

View reviewed changes

dgeb reviewed Sep 27, 2016

View reviewed changes

rwjblue force-pushed the move-is-destroyed-flags branch from 2dc9334 to bcf3acb Compare September 27, 2016 17:36

krisselden reviewed Sep 27, 2016

View reviewed changes

rwjblue force-pushed the move-is-destroyed-flags branch from bcf3acb to 5c0e590 Compare September 27, 2016 21:56

rwjblue mentioned this pull request Sep 27, 2016

[WIP] Ensure correct shaping for isDestroy[ing|ed] on CoreObject. #13782

Closed

rwjblue merged commit 3141f83 into master Sep 28, 2016

rwjblue deleted the move-is-destroyed-flags branch September 28, 2016 00:44

homu mentioned this pull request Sep 28, 2016

Create FunctionMeta to store metadata for functions. #13800

Closed

rwjblue mentioned this pull request Nov 10, 2016

don't recreate meta on an object after it is destroyed #13454

Closed

Uh oh!

Refactor Meta and object destruction tracking. #14359

Refactor Meta and object destruction tracking. #14359

Uh oh!

Conversation

rwjblue commented Sep 26, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stefanpenner commented Sep 26, 2016

Uh oh!

rwjblue commented Sep 26, 2016

Uh oh!

stefanpenner commented Sep 26, 2016

Uh oh!

krisselden commented Sep 27, 2016

Uh oh!

stefanpenner commented Sep 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rwjblue commented Sep 27, 2016

Uh oh!

stefanpenner Sep 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rwjblue commented Sep 27, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krisselden commented Sep 27, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rwjblue commented Sep 27, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stefanpenner commented Sep 27, 2016 •

edited

Loading

stefanpenner Sep 27, 2016 •

edited

Loading