Re-enumerating every time massively slows down Chai

[RubenVerborgh]

In addChainableMethod, we find this code:

// Re-enumerate every time to better accomodate plugins.
var asserterNames = Object.getOwnPropertyNames(ctx);
asserterNames.forEach(function (asserterName) {
  var pd = Object.getOwnPropertyDescriptor(ctx, asserterName)
    , functionProtoPD = Object.getOwnPropertyDescriptor(Function.prototype, asserterName);
  // Avoid trying to overwrite things that we can't, like `length` and `arguments`.
  if (functionProtoPD && !functionProtoPD.configurable) return;
  if (asserterName === 'arguments') return; // @see chaijs/chai/issues/69
  Object.defineProperty(assert, asserterName, pd);
});

However, this re-enumerating very negatively affects performance. Consider this simple script:

for (var i = 0; i < 100000; i++)
  'x'.should.be.a('string');

This takes 22.6s with the re-enumeration code, and 0.6s without. That's huge.

Can this code somehow be:

1. moved outside the getter (i.e., don't accommodate plugins better)?
2. ran conditionally (i.e., only if something changed)?

However, profiling suggests that defineProperty itself is also expensive.

[domenic]

Great detective work; this will be really helpful to straighten out. Your #128 is a good start.

I wonder if we could re-compute and cache the list of property descriptors every time chai.use is called. That still leaves the Object.defineProperty calls, but maybe takes care of the majority of the slowness?

[RubenVerborgh]

Ideally, we'd want all those properties created in the forEach to be defined on a prototype. This would enable us to just return a new function with this prototype, and we wouldn't have to redefine the properties all the time (provided chai.use resets them, maybe even provide a rebuild method).

Unfortunately, I haven't found a way to give a function another prototype than Function (might have overlooked something). Then… would adding something to the Function prototype be a possibility, or is this a no-go?

[domenic]

We actually used to do that, and you can give a function another prototype with __proto__. The problem is, this does not work in any version of IE, up to and including IE 10.

And yes, modifying the Function prototype is a no-go.

[RubenVerborgh]

Only IE10? I didn't want to mess with __proto__ because I thought it was far less supported than that.

But…

It doesn't seem justified to suffer this performance loss just for those engines that don't implement __proto__.
So how about this? Let's implement it with __proto__, but provide the current solution as a fallback.
I'd be willing to do that.

[domenic]

Is the hit really in the Object.defineProperty? Or is it the Object.getOwnPropertyDescriptor?

[RubenVerborgh]

It's both, but defineProperty most:

 [JavaScript]:
   ticks  total  nonlib   name
    819    3.8%    3.8%  LazyCompile: *DefineObjectProperty native v8natives.js:695
    595    2.8%    2.8%  LazyCompile: *ToPropertyDescriptor native v8natives.js:420
    537    2.5%    2.5%  LazyCompile: *PropertyDescriptor native v8natives.js:482
    525    2.4%    2.4%  LazyCompile: IN native runtime.js:354
    453    2.1%    2.1%  LazyCompile: ToString native runtime.js:550
    449    2.1%    2.1%  LazyCompile: *FromPropertyDescriptor native v8natives.js:373
    395    1.8%    1.8%  LazyCompile: *ConvertDescriptorArrayToDescriptor native v8natives.js:581

[domenic]

Sigh. OK. Well, that's probably the best approach. Although I'd need to think about it for a while to make sure it's not going to introduce suble cross-browser differences that cause someone to pull their hair out after hours of debugging.

In the meantime, you might find our old __proto__ usage educational: e1256e9

[RubenVerborgh]

I've now quickly moved getOwnPropertyNames outside the loop, and execution time changed from 19.5s to 8.8s. That's already a lot. I might find some time later to do the __proto__ implementation.

[RubenVerborgh]

$ git co master
Switched to branch 'master'
$ time node ../test.js 
real    0m22.186s
user    0m22.111s
sys     0m0.218s
$ git co proto
Switched to branch 'proto'
$ time node ../test.js 
real    0m1.232s
user    0m1.176s
sys     0m0.062s

[RubenVerborgh]

#131 has been merged