stampit-org / stampit

OOP is better with stamps: Composable object factories.
https://stampit.js.org
MIT License
3.02k stars 102 forks source link

Performance degradation in node.js 8 #325

Closed koresar closed 6 years ago

koresar commented 6 years ago

Stampit has two benchmark tests. They all run fine in node.js v7 and less.

TAP version 13
# benchmarking object creation
ok 1 creating a stamp should be less than 50 times slower, not more
# Stamp x 1,329,803 ops/sec ±1.51% (86 runs sampled). Class x 23,924,811 ops/sec ±1.08% (88 runs sampled)
# benchmarking property access
ok 2 object instances property access must be as fast as plain object.
# Stampit x 90,346,971 ops/sec ±1.41% (86 runs sampled). Plain object x 88,215,582 ops/sec ±1.42% (86 runs sampled)

1..2
# tests 2
# pass  2

# ok

But in node.js 8 we have a problem:

TAP version 13
# benchmarking object creation
ok 1 creating a stamp should be less than 50 times slower, not more
# Stamp x 2,299,526 ops/sec ±1.37% (90 runs sampled). Class x 86,760,921 ops/sec ±1.19% (86 runs sampled)
# benchmarking property access
not ok 2 object instances property access must be as fast as plain object.
  ---
    operator: ok
    expected: true
    actual:   false
    at: Suite.<anonymous> (/Users/vasyl/code/stampit/test/benchmark/property-access.js:66:9)
  ...
# Stampit x 19,520,623 ops/sec ±1.33% (88 runs sampled). Plain object x 298,272,859 ops/sec ±1.10% (83 runs sampled)

1..2
# tests 2
# pass  1
# fail  1

1) Good news. Stampit object creation is 2x times faster. 2) Bad news. Stampit-created objects property access got x4 time slower. But native objects got 3x faster in node.js 8.

Need help.

koresar commented 6 years ago

Created a bug in V8 bugtracker: https://bugs.chromium.org/p/v8/issues/detail?id=6909

ericelliott commented 6 years ago

It sounds like V8 implemented some optimizations that are not available for objects created with Object.assign(). Probably not a bug in v8 -- they just opened up a new fast path.

A performance degradation would mean that something got slower between v7 and v8. Sounds like the opposite happened?

koresar commented 6 years ago

It got x3 faster for plain objects, but x6 times slower for Obejct.assign-ed objects.

It happened after they moved to the new pipeline in V8 v6.0: https://v8project.blogspot.com.au/2017/05/launching-ignition-and-turbofan.html

ericelliott commented 6 years ago

Oh, I see: Before: Stampit x 90,346,971 ops/sec After: Stampit x 19,520,623 ops/sec

ericelliott commented 6 years ago

Looks like the null prototype is the problem. Try it with {} as the prototype?

koresar commented 6 years ago

I will try.

koresar commented 6 years ago

Ok. Fixed it. Instead of Object.create(descriptor.methods || null) I put descriptor.methods ? Object.create(descriptor.methods) : {}. The performance is back!

koresar commented 6 years ago

Even better:

descriptor.methods ? {__proto__: descriptor.methods} : {};

Now, that's fast in any case! :)

ericelliott commented 6 years ago

Did you test the {__proto__: descriptor.methods} : {}; vs descriptor.methods ? Object.create(descriptor.methods) : {}? Was there any perf difference?

koresar commented 6 years ago

descriptor.methods ? Object.create(descriptor.methods) : {} and descriptor.methods ? {__proto__: descriptor.methods} : {}; and {__proto__: descriptor.methods} are identical in my benchmark.

The last one is less code though. Tested in IE11, it works.

ericelliott commented 6 years ago

Faster in Node v7, too?

koresar commented 6 years ago

Tested against all node.js LTS 4,6,8. Property access of plain and stampit objects identical now in all three.

Node v7 did not have that problem. Only 8.x had.