Implement optimized objectInfo structure

src/object-observe.js

 // Copyright 2012 the V8 project authors. All rights reserved.
 // Redistribution and use in source and binary forms, with or without
 // modification, are permitted provided that the following conditions are
 // met:
 //
 //     * Redistributions of source code must retain the above copyright
 //       notice, this list of conditions and the following disclaimer.
 //     * Redistributions in binary form must reproduce the above
 //       copyright notice, this list of conditions and the following
 //       disclaimer in the documentation and/or other materials provided
@@ skipping 13 matching lines @@
 // THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
 // (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
 // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
 "use strict";
 
 var observationState = %GetObservationState();
 if (IS_UNDEFINED(observationState.callbackInfoMap)) {
   observationState.callbackInfoMap = %ObservationWeakMapCreate();
   observationState.objectInfoMap = %ObservationWeakMapCreate();
-  observationState.notifierTargetMap = %ObservationWeakMapCreate();
-  observationState.pendingObservers = new InternalArray;
+  observationState.notifierObjectInfoMap = %ObservationWeakMapCreate();
+  observationState.pendingObservers = { __proto__: null };
+  observationState.anyPendingObservers = false;
   observationState.nextCallbackPriority = 0;
 }
 
 function ObservationWeakMap(map) {
   this.map_ = map;
 }
 
 ObservationWeakMap.prototype = {
   get: function(key) {
     key = %UnwrapGlobalProxy(key);
     if (!IS_SPEC_OBJECT(key)) return void 0;
     return %WeakCollectionGet(this.map_, key);
   },
   set: function(key, value) {
     key = %UnwrapGlobalProxy(key);
     if (!IS_SPEC_OBJECT(key)) return void 0;
     %WeakCollectionSet(this.map_, key, value);
   },
   has: function(key) {
     return !IS_UNDEFINED(this.get(key));
   }
 };
 
 var callbackInfoMap =
     new ObservationWeakMap(observationState.callbackInfoMap);
 var objectInfoMap = new ObservationWeakMap(observationState.objectInfoMap);
-var notifierTargetMap =
-    new ObservationWeakMap(observationState.notifierTargetMap);
+var notifierObjectInfoMap =
+    new ObservationWeakMap(observationState.notifierObjectInfoMap);
 
-function CreateObjectInfo(object) {
-  var info = {
-    changeObservers: new InternalArray,
-    notifier: null,
-    inactiveObservers: new InternalArray,
-    performing: { __proto__: null },
-    performingCount: 0,
-  };
-  objectInfoMap.set(object, info);
-  return info;
+function TypeMapCreate() {
+  return { __proto__: null };
 }
 
-var defaultAcceptTypes = {
-  __proto__: null,
-  'new': true,
-  'updated': true,
-  'deleted': true,
-  'prototype': true,
-  'reconfigured': true
-};
-
-function CreateObserver(callback, accept) {
-  var observer = {
-    __proto__: null,
-    callback: callback,
-    accept: defaultAcceptTypes
-  };
-
-  if (IS_UNDEFINED(accept))
-    return observer;
-
-  var acceptMap = { __proto__: null };
-  for (var i = 0; i < accept.length; i++)
-    acceptMap[accept[i]] = true;
-
-  observer.accept = acceptMap;
-  return observer;
+function TypeMapAddType(typeMap, type, ignoreDuplicate) {
+  typeMap[type] = ignoreDuplicate ? 1 : (typeMap[type] || 0) + 1;

[rossberg, 2013/08/07 13:02:14]

Hm, this hack could be avoided if AcceptArgIsValid flagged duplicates as errors.

[rafaelw, 2013/08/07 20:36:28]

That's not the issue. objectInfo.performing is a typeMap and it needs to have a
*count* of each active type.

On 2013/08/07 13:02:14, rossberg wrote:

 }
 
-function ObserverIsActive(observer, objectInfo) {
-  if (objectInfo.performingCount === 0)
+function TypeMapRemoveType(typeMap, type) {
+  typeMap[type]--;
+}
+
+function TypeMapCreateFromList(typeList) {
+  var typeMap = TypeMapCreate();
+  for (var i = 0; i < typeList.length; i++) {
+    TypeMapAddType(typeMap, typeList[i], true);
+  }
+  return typeMap;
+}
+
+function TypeMapHasType(typeMap, type) {
+  return typeMap[type];

[rossberg, 2013/08/07 13:02:14]

Nit: !!typeMap[type]

[rafaelw, 2013/08/07 20:36:28]

Done.

+}
+
+function TypeMapIsDisjointFrom(typeMap1, typeMap2) {
+  if (!typeMap1 || !typeMap2)
     return true;
 
-  var performing = objectInfo.performing;
-  for (var type in performing) {
-    if (performing[type] > 0 && observer.accept[type])
+  for (var type in typeMap1) {
+    if (TypeMapHasType(typeMap1, type) && TypeMapHasType(typeMap2, type))
       return false;
   }
 
   return true;
 }
 
-function ObserverIsInactive(observer, objectInfo) {
-  return !ObserverIsActive(observer, objectInfo);
+var defaultAcceptTypes = TypeMapCreateFromList([
+  'new',
+  'updated',
+  'deleted',
+  'prototype',
+  'reconfigured'
+]);
+
+// An Observer is a registration to observe an object by a callback with a
+// a given set of accept types. If the set of accept types is the default

[rossberg, 2013/08/07 13:02:14]

Nit: duplicate "a"

[rafaelw, 2013/08/07 20:36:28]

Done.

+// set for Object.observe, the observer is represented as a direct reference
+// to the callback. An observer never changes its accept types and thus never
+// needs to "normalize".
+function ObserverCreate(callback, acceptList) {
+  return IS_UNDEFINED(acceptList) ? callback : {
+    __proto__: null,
+    callback: callback,
+    accept: TypeMapCreateFromList(acceptList)
+  };
 }
 
-function RemoveNullElements(from) {
-  var i = 0;
-  var j = 0;
-  for (; i < from.length; i++) {
-    if (from[i] === null)
-      continue;
-    if (j < i)
-      from[j] = from[i];
-    j++;
+function ObserverGetCallback(observer) {
+  return IS_SPEC_FUNCTION(observer) ? observer : observer.callback;
+}
+
+function ObserverGetAcceptTypes(observer) {
+  return IS_SPEC_FUNCTION(observer) ? defaultAcceptTypes : observer.accept;
+}
+
+function ObserverIsActive(observer, objectInfo) {
+  return TypeMapIsDisjointFrom(ObjectInfoGetPerformingTypes(objectInfo),
+                               ObserverGetAcceptTypes(observer));
+}
+
+function ObjectInfoGetOrCreate(object) {
+  var objectInfo = objectInfoMap.get(object);
+  if (IS_UNDEFINED(objectInfo)) {
+    if (!%IsJSProxy(object))
+      %SetIsObserved(object);
+
+    objectInfo = {
+      object: object,
+      changeObserver: null,
+      changeObservers: null,
+      notifier: null,
+      performing: null,
+      performingCount: 0,
+    };
+    objectInfoMap.set(object, objectInfo);
+  }
+  return objectInfo;
+}
+
+function ObjectInfoGet(object) {

[rossberg, 2013/08/07 13:02:14]

Is this still used at all? If not, you could as well drop the 'OrCreate' suffix
from the above function.

[rafaelw, 2013/08/07 20:36:28]

Done.

+  return objectInfoMap.get(object);
+}
+
+function ObjectInfoGetFromNotifier(notifier) {
+  return notifierObjectInfoMap.get(notifier);
+}
+
+function ObjectInfoGetNotifier(objectInfo) {
+  if (IS_NULL(objectInfo.notifier)) {
+    objectInfo.notifier = { __proto__: notifierPrototype };
+    notifierObjectInfoMap.set(objectInfo.notifier, objectInfo);
   }
 
-  if (i !== j)
-    from.length = from.length - (i - j);
+  return objectInfo.notifier;
 }
 
-function RepartitionObservers(conditionFn, from, to, objectInfo) {
-  var anyRemoved = false;
-  for (var i = 0; i < from.length; i++) {
-    var observer = from[i];
-    if (conditionFn(observer, objectInfo)) {
-      anyRemoved = true;
-      from[i] = null;
-      to.push(observer);
-    }
+function ObjectInfoGetObject(objectInfo) {
+  return objectInfo.object;
+}
+
+// The set of observers on an object is called 'changeObservers'. The first
+// observer is referenced directly via objectInfo.changeObserver. When a second

[rossberg, 2013/08/07 13:02:14]

Wouldn't it be slightly simpler to use only 'changeObservers' and distinguish
based on whether it's a function?

[rafaelw, 2013/08/07 20:36:28]

The problem is that an "observer" can be normal (an object with a .callback
property) or optimized (just the callback).

All of the places which touch the set of observers will need to test for 

if typeof objectInfo.changeObserver === 'function' ||
   typeof objectInto.changeObserver.callback === 'function'

(which can obvious be a utility function), but at that point, I feel like it
violates the encapsulation of the optimized/normalized structures.

If you feel strongly about it, I'm happy to do it (preferably in a follow-on
patch).

On 2013/08/07 13:02:14, rossberg wrote:

[rossberg, 2013/08/09 11:26:51]

I don't feel too strongly about it. But I would argue that contrary to what you
say, the current version actually violates abstraction, because it points to a
piece of information in two different ways, depending solely on that
information's normalization state.

[rafaelw, 2013/08/09 14:14:37]

Fair enough. Done.

On 2013/08/09 11:26:51, rossberg wrote:

+// is added, changeObservers "normalizes" to become a mapping of callback
+// priority -> observer and is then stored on objectInfo.changeObservers.
+function ObjectInfoNormalizeChangeObservers(objectInfo) {
+  var observer = objectInfo.changeObserver;
+  objectInfo.changeObserver = null;
+  objectInfo.changeObservers = { __proto__: null };
+  var callback = ObserverGetCallback(observer);
+  var callbackInfo = CallbackInfoGet(callback);
+  var priority = CallbackInfoGetPriority(callbackInfo);
+  objectInfo.changeObservers[priority] = observer;
+}
+
+function ObjectInfoAddObserver(objectInfo, callback, acceptList) {
+  var callbackInfo = CallbackInfoGetOrCreate(callback);
+  var observer = ObserverCreate(callback, acceptList);
+
+  if (!objectInfo.changeObserver && !objectInfo.changeObservers) {
+    objectInfo.changeObserver = observer;
+    return;
   }
 
-  if (anyRemoved)
-    RemoveNullElements(from);
+  if (objectInfo.changeObserver)
+    ObjectInfoNormalizeChangeObservers(objectInfo);
+
+  var priority = CallbackInfoGetPriority(callbackInfo);
+  objectInfo.changeObservers[priority] = observer;
 }
 
-function BeginPerformChange(objectInfo, type) {
-  objectInfo.performing[type] = (objectInfo.performing[type] || 0) + 1;
-  objectInfo.performingCount++;
-  RepartitionObservers(ObserverIsInactive,
-                       objectInfo.changeObservers,
-                       objectInfo.inactiveObservers,
-                       objectInfo);
+function ObjectInfoRemoveObserver(objectInfo, callback) {
+  if (objectInfo.changeObserver) {
+    if (callback === ObserverGetCallback(objectInfo.changeObserver))
+      objectInfo.changeObserver = null;
+
+    return;
+  }
+
+  var callbackInfo = CallbackInfoGet(callback);
+  var priority = CallbackInfoGetPriority(callbackInfo);
+  delete objectInfo.changeObservers[priority];
 }
 
-function EndPerformChange(objectInfo, type) {
-  objectInfo.performing[type]--;
-  objectInfo.performingCount--;
-  RepartitionObservers(ObserverIsActive,
-                       objectInfo.inactiveObservers,
-                       objectInfo.changeObservers,
-                       objectInfo);
+function ObjectInfoHasActiveObservers(objectInfo) {
+  if (IS_UNDEFINED(objectInfo))
+    return false;
+
+  if (objectInfo.changeObserver)
+    return ObserverIsActive(objectInfo.changeObserver, objectInfo);
+
+  for (var priority in objectInfo.changeObservers) {
+    if (ObserverIsActive(objectInfo.changeObservers[priority], objectInfo))
+      return true;
+  }
+
+  return false;
 }
 
-function EnsureObserverRemoved(objectInfo, callback) {
-  function remove(observerList) {
-    for (var i = 0; i < observerList.length; i++) {
-      if (observerList[i].callback === callback) {
-        observerList.splice(i, 1);
-        return true;
-      }
-    }
-    return false;
-  }
+function ObjectInfoAddPerformingType(objectInfo, type) {
+  objectInfo.performing = objectInfo.performing || TypeMapCreate();
+  TypeMapAddType(objectInfo.performing, type);
+  objectInfo.performingCount++;
+}
 
-  if (!remove(objectInfo.changeObservers))
-    remove(objectInfo.inactiveObservers);
+function ObjectInfoRemovePerformingType(objectInfo, type) {
+  objectInfo.performingCount--;
+  TypeMapRemoveType(objectInfo.performing, type);
+}
+
+function ObjectInfoGetPerformingTypes(objectInfo) {
+  return objectInfo.performingCount > 0 ? objectInfo.performing : null;
 }
 
 function AcceptArgIsValid(arg) {
   if (IS_UNDEFINED(arg))
     return true;
 
   if (!IS_SPEC_OBJECT(arg) ||
       !IS_NUMBER(arg.length) ||
       arg.length < 0)
     return false;
 
   var length = arg.length;
   for (var i = 0; i < length; i++) {
     if (!IS_STRING(arg[i]))
       return false;
   }
   return true;
 }
 
-function EnsureCallbackPriority(callback) {
-  if (!callbackInfoMap.has(callback))
-    callbackInfoMap.set(callback, observationState.nextCallbackPriority++);
+function CallbackInfoGet(callback) {
+  return callbackInfoMap.get(callback);
 }
 
-function NormalizeCallbackInfo(callback) {
+function CallbackInfoGetOrCreate(callback) {
+  var callbackInfo = callbackInfoMap.get(callback);
+  if (!IS_UNDEFINED(callbackInfo))
+    return callbackInfo;
+
+  var priority = observationState.nextCallbackPriority++
+  callbackInfoMap.set(callback, priority);
+  return priority;
+}
+
+function CallbackInfoGetPriority(callbackInfo) {
+  if (IS_NUMBER(callbackInfo))
+    return callbackInfo;
+  else
+    return callbackInfo.priority;
+}
+
+function CallbackInfoNormalize(callback) {
   var callbackInfo = callbackInfoMap.get(callback);
   if (IS_NUMBER(callbackInfo)) {
     var priority = callbackInfo;
     callbackInfo = new InternalArray;
     callbackInfo.priority = priority;
     callbackInfoMap.set(callback, callbackInfo);
   }
   return callbackInfo;
 }
 
-function ObjectObserve(object, callback, accept) {
+function ObjectObserve(object, callback, acceptList) {
   if (!IS_SPEC_OBJECT(object))
     throw MakeTypeError("observe_non_object", ["observe"]);
   if (!IS_SPEC_FUNCTION(callback))
     throw MakeTypeError("observe_non_function", ["observe"]);
   if (ObjectIsFrozen(callback))
     throw MakeTypeError("observe_callback_frozen");
-  if (!AcceptArgIsValid(accept))
+  if (!AcceptArgIsValid(acceptList))
     throw MakeTypeError("observe_accept_invalid");
 
-  EnsureCallbackPriority(callback);
-
-  var objectInfo = objectInfoMap.get(object);
-  if (IS_UNDEFINED(objectInfo)) {
-    objectInfo = CreateObjectInfo(object);
-    %SetIsObserved(object);
-  }
-
-  EnsureObserverRemoved(objectInfo, callback);
-
-  var observer = CreateObserver(callback, accept);
-  if (ObserverIsActive(observer, objectInfo))
-    objectInfo.changeObservers.push(observer);
-  else
-    objectInfo.inactiveObservers.push(observer);
-
+  var objectInfo = ObjectInfoGetOrCreate(object);
+  ObjectInfoAddObserver(objectInfo, callback, acceptList);
   return object;
 }
 
 function ObjectUnobserve(object, callback) {
   if (!IS_SPEC_OBJECT(object))
     throw MakeTypeError("observe_non_object", ["unobserve"]);
   if (!IS_SPEC_FUNCTION(callback))
     throw MakeTypeError("observe_non_function", ["unobserve"]);
 
   var objectInfo = objectInfoMap.get(object);
   if (IS_UNDEFINED(objectInfo))
     return object;
 
-  EnsureObserverRemoved(objectInfo, callback);
+  ObjectInfoRemoveObserver(objectInfo, callback);
   return object;
 }
 
 function ArrayObserve(object, callback) {
   return ObjectObserve(object, callback, ['new',
                                           'updated',
                                           'deleted',
                                           'splice']);
 }
 
 function ArrayUnobserve(object, callback) {
   return ObjectUnobserve(object, callback);
 }
 
-function EnqueueToCallback(callback, changeRecord) {
-  var callbackInfo = NormalizeCallbackInfo(callback);
+function ObserverEnqueueIfActive(observer, objectInfo, changeRecord) {
+  if (!ObserverIsActive(observer, objectInfo) ||
+      !TypeMapHasType(ObserverGetAcceptTypes(observer), changeRecord.type)) {
+    return;
+  }
+
+  var callback = ObserverGetCallback(observer);
+  var callbackInfo = CallbackInfoNormalize(callback);
   observationState.pendingObservers[callbackInfo.priority] = callback;
+  observationState.anyPendingObservers = true;
   callbackInfo.push(changeRecord);
   %SetObserverDeliveryPending();
 }
 
-function EnqueueChangeRecord(changeRecord, observers) {
+function ObjectInfoEnqueueChangeRecord(objectInfo, changeRecord) {
   // TODO(rossberg): adjust once there is a story for symbols vs proxies.
   if (IS_SYMBOL(changeRecord.name)) return;
 
-  for (var i = 0; i < observers.length; i++) {
-    var observer = observers[i];
-    if (IS_UNDEFINED(observer.accept[changeRecord.type]))
-      continue;
+  if (objectInfo.changeObserver) {
+    var observer = objectInfo.changeObserver;
+    ObserverEnqueueIfActive(observer, objectInfo, changeRecord);
+    return;
+  }
 
-    EnqueueToCallback(observer.callback, changeRecord);
+  for (var priority in objectInfo.changeObservers) {
+    var observer = objectInfo.changeObservers[priority];
+    ObserverEnqueueIfActive(observer, objectInfo, changeRecord);
   }
 }
 
 function BeginPerformSplice(array) {
   var objectInfo = objectInfoMap.get(array);
   if (!IS_UNDEFINED(objectInfo))
-    BeginPerformChange(objectInfo, 'splice');
+    ObjectInfoAddPerformingType(objectInfo, 'splice');
 }
 
 function EndPerformSplice(array) {
   var objectInfo = objectInfoMap.get(array);
   if (!IS_UNDEFINED(objectInfo))
-    EndPerformChange(objectInfo, 'splice');
+    ObjectInfoRemovePerformingType(objectInfo, 'splice');
 }
 
 function EnqueueSpliceRecord(array, index, removed, addedCount) {
   var objectInfo = objectInfoMap.get(array);
-  if (IS_UNDEFINED(objectInfo) || objectInfo.changeObservers.length === 0)
+  if (!ObjectInfoHasActiveObservers(objectInfo))
     return;
 
   var changeRecord = {
     type: 'splice',
     object: array,
     index: index,
     removed: removed,
     addedCount: addedCount
   };
 
   ObjectFreeze(changeRecord);
   ObjectFreeze(changeRecord.removed);
-  EnqueueChangeRecord(changeRecord, objectInfo.changeObservers);
+  ObjectInfoEnqueueChangeRecord(objectInfo, changeRecord);
 }
 
 function NotifyChange(type, object, name, oldValue) {
   var objectInfo = objectInfoMap.get(object);
-  if (objectInfo.changeObservers.length === 0)
+  if (!ObjectInfoHasActiveObservers(objectInfo))
     return;
 
   var changeRecord = (arguments.length < 4) ?
       { type: type, object: object, name: name } :
       { type: type, object: object, name: name, oldValue: oldValue };
   ObjectFreeze(changeRecord);
-  EnqueueChangeRecord(changeRecord, objectInfo.changeObservers);
+  ObjectInfoEnqueueChangeRecord(objectInfo, changeRecord);
 }
 
 var notifierPrototype = {};
 
 function ObjectNotifierNotify(changeRecord) {
   if (!IS_SPEC_OBJECT(this))
     throw MakeTypeError("called_on_non_object", ["notify"]);
 
-  var target = notifierTargetMap.get(this);
-  if (IS_UNDEFINED(target))
+  var objectInfo = ObjectInfoGetFromNotifier(this);
+  if (IS_UNDEFINED(objectInfo))
     throw MakeTypeError("observe_notify_non_notifier");
   if (!IS_STRING(changeRecord.type))
     throw MakeTypeError("observe_type_non_string");
 
-  var objectInfo = objectInfoMap.get(target);
-  if (IS_UNDEFINED(objectInfo) || objectInfo.changeObservers.length === 0)
+  if (!ObjectInfoHasActiveObservers(objectInfo))
     return;
 
-  var newRecord = { object: target };
+  var newRecord = { object: ObjectInfoGetObject(objectInfo) };
   for (var prop in changeRecord) {
     if (prop === 'object') continue;
     %DefineOrRedefineDataProperty(newRecord, prop, changeRecord[prop],
         READ_ONLY + DONT_DELETE);
   }
   ObjectFreeze(newRecord);
 
-  EnqueueChangeRecord(newRecord, objectInfo.changeObservers);
+  ObjectInfoEnqueueChangeRecord(objectInfo, newRecord);
 }
 
 function ObjectNotifierPerformChange(changeType, changeFn, receiver) {
   if (!IS_SPEC_OBJECT(this))
     throw MakeTypeError("called_on_non_object", ["performChange"]);
 
-  var target = notifierTargetMap.get(this);
-  if (IS_UNDEFINED(target))
+  var objectInfo = ObjectInfoGetFromNotifier(this);
+
+  if (IS_UNDEFINED(objectInfo))
     throw MakeTypeError("observe_notify_non_notifier");
   if (!IS_STRING(changeType))
     throw MakeTypeError("observe_perform_non_string");
   if (!IS_SPEC_FUNCTION(changeFn))
     throw MakeTypeError("observe_perform_non_function");
 
   if (IS_NULL_OR_UNDEFINED(receiver)) {
     receiver = %GetDefaultReceiver(changeFn) || receiver;
   } else if (!IS_SPEC_OBJECT(receiver) && %IsClassicModeFunction(changeFn)) {
     receiver = ToObject(receiver);
   }
 
-  var objectInfo = objectInfoMap.get(target);
-  if (IS_UNDEFINED(objectInfo))
-    return;
-
-  BeginPerformChange(objectInfo, changeType);
+  ObjectInfoAddPerformingType(objectInfo, changeType);
   try {
     %_CallFunction(receiver, changeFn);
   } finally {
-    EndPerformChange(objectInfo, changeType);
+    ObjectInfoRemovePerformingType(objectInfo, changeType);
   }
 }
 
 function ObjectGetNotifier(object) {
   if (!IS_SPEC_OBJECT(object))
     throw MakeTypeError("observe_non_object", ["getNotifier"]);
 
   if (ObjectIsFrozen(object)) return null;
 
-  var objectInfo = objectInfoMap.get(object);
-  if (IS_UNDEFINED(objectInfo)) {
-    objectInfo = CreateObjectInfo(object);
-    %SetIsObserved(object);
-  }
-
-  if (IS_NULL(objectInfo.notifier)) {
-    objectInfo.notifier = { __proto__: notifierPrototype };
-    notifierTargetMap.set(objectInfo.notifier, object);
-  }
-
-  return objectInfo.notifier;
+  var objectInfo = ObjectInfoGetOrCreate(object);
+  return ObjectInfoGetNotifier(objectInfo);
 }
 
 function CallbackDeliverPending(callback) {
   var callbackInfo = callbackInfoMap.get(callback);
   if (IS_UNDEFINED(callbackInfo) || IS_NUMBER(callbackInfo))
     return false;
 
   // Clear the pending change records from callback and return it to its
   // "optimized" state.
   var priority = callbackInfo.priority;
@@ skipping 10 matching lines @@
 }
 
 function ObjectDeliverChangeRecords(callback) {
   if (!IS_SPEC_FUNCTION(callback))
     throw MakeTypeError("observe_non_function", ["deliverChangeRecords"]);
 
   while (CallbackDeliverPending(callback)) {}
 }
 
 function DeliverChangeRecords() {
-  while (observationState.pendingObservers.length) {
+  while (observationState.anyPendingObservers) {

[rossberg, 2013/08/07 13:02:14]

I don't think you need to make this flag part of the global state. To detect a
fix point it should be enough to have a local flag that gets set in the for-loop
below (without a bit of extra logic you will redundantly create an extra
pendingObservers object, but I don't think that matters).

[rafaelw, 2013/08/07 20:36:28]

I don't understand. observationState.anyPendingObservers is set to true in
ObserverEnqueueIfActive. This is handling the case that an observer gets
"re-activated" because of mutations to an observed object. I don't think there's
a way to retrieve that information locally in this function.

If you object to the flag, we can enqueue the fact that there are any pending
with pendingObservers being null in that case. That, however, means that several
other places in the code need to check it for being null (and i don't think it
buys us anything perf-wise).

On 2013/08/07 13:02:14, rossberg wrote:

[rossberg, 2013/08/09 11:26:51]

What I mean is that all you are really implementing here is a local fixpoint
iteration. And all the flag really does is helping you detect when you have
reached that fixpoint. But since the fixpoint iteration is a concern of this
function only, it would be better to implement it without having an
implementation detail (the flag) leak into the global state and make it a
concern of other code.

I'd implement the fixpoint iteration using only local logic:

  var changed
  do {
    changed = false
    var pendingObservers = observationState.pendingObservers
    observationState.pendingObservers = { __proto__: null }
    for (var i in pendingObservers) {
      changed = true
      CallbackDeliverPending(pendingObservers[i])
    }
  } while(changed)

The only extra cost here is that you allocate a fresh pendingObservers object
even when it was empty. But I highly doubt that cost is measurable at all, given
that this function is only called once per turn.

Or am I missing something?

[rafaelw, 2013/08/09 14:14:37]

I tried & profiled this version, but it made deliveringAll in the noop case 5x
more expensive.

This doesn't matter much now, but we'll soon have a microtask delivery queue and
we may have to repeatedly deliverAll when there is nothing to deliver.

Instead I implemented the version where observationState.pendingObservers is
null when there are no pending observers (getting rid of the anyPendingObservers
flag).

On 2013/08/09 11:26:51, rossberg wrote:

[rossberg, 2013/08/13 09:19:25]

Hm, that sounds like a lot. Did you try a version that avoids the unnecessary
allocation?

  var changed
  do {
    changed = false
    var pendingObservers = observationState.pendingObservers
    for (var i in pendingObservers) {
      if (!changed) {
        changed = true
        observationState.pendingObservers = { __proto__: null }
      }
      CallbackDeliverPending(pendingObservers[i])
    }
  } while(changed)

But maybe the cost actually is in initialising the for-in.
OK, it still doesn't look ideal to me, and I find it somewhat hard to believe
that such a micro optimisation should matter. But since I don't really
understand the microtask story yet, I'll shut up. :)

[rafaelw, 2013/08/19 18:51:24]

That one is 3x more expensive.

On 2013/08/13 09:19:25, rossberg wrote:

     var pendingObservers = observationState.pendingObservers;
-    observationState.pendingObservers = new InternalArray;
+    observationState.pendingObservers = { __proto__: null };
+    observationState.anyPendingObservers = false
     for (var i in pendingObservers) {
       CallbackDeliverPending(pendingObservers[i]);
     }
   }
 }
 
 function SetupObjectObserve() {
   %CheckIsBootstrapping();
   InstallFunctions($Object, DONT_ENUM, $Array(
     "deliverChangeRecords", ObjectDeliverChangeRecords,
     "getNotifier", ObjectGetNotifier,
     "observe", ObjectObserve,
     "unobserve", ObjectUnobserve
   ));
   InstallFunctions($Array, DONT_ENUM, $Array(
     "observe", ArrayObserve,
     "unobserve", ArrayUnobserve
   ));
   InstallFunctions(notifierPrototype, DONT_ENUM, $Array(
     "notify", ObjectNotifierNotify,
     "performChange", ObjectNotifierPerformChange
   ));
 }
 
 SetupObjectObserve();