New issue
Advanced search Search tips
Starred by 1 user
Status: Fixed
Owner:
Closed: Oct 4
Cc:
Components:
HW: All
OS: All
Priority: 2
Type: FeatureRequest

Blocking:
issue 6278
issue 6344



Sign in to add a comment
Polymorphic constructor inlining
Project Member Reported by bmeu...@chromium.org, Oct 4 Back to list
Considering again the example from  crbug.com/v8/6367 , extracted from the ARES6 ML benchmark:

-------------------------------------------------------------
class A {
  static get [Symbol.species]() { return this; }
  clone() { return new this.constructor[Symbol.species](); }
}

class B extends A {
  static get [Symbol.species]() { return this; }
}

function foo(o) { return o.clone(); }
foo(new A());
foo(new B());
-------------------------------------------------------------

This seems to be evolving as a common pattern, esp. since the standard already introduces Symbol.species for exactly this purpose. So it makes sense to support this paradigm well in TurboFan.

The polymorphic lookup in this.constructor[Symbol.species] is now fixed and properly optimized. However the inlining of the new call to this is still not working, since the checks introduced for the accessor block the inlining. With some tweaking in TurboFan it should be possible to also eliminate these checks and teach TurboFan to inline the two constructors.

Early experiments show a 1.5% improvement on ARES6 ML, but I've also created a dedicated micro-benchmark to check the improvement:

=================< bench-polymorphic-species-clone.js >=================
if (typeof console === 'undefined') console = {log:print};

class A {
  static get [Symbol.species]() { return this; }
  clone() { return new this.constructor[Symbol.species](); }
}

class B extends A {
  static get [Symbol.species]() { return this; }
}

const N = 1e7;
const OBJS = [new A, new B];

function testClone() {
  let result;
  for (const o of OBJS) {
    for (let i = 0; i < N; ++i) {
      result = o.clone();
    }
  }
  return result;
}

var TESTS = [
    testClone
];

function test(fn) {
  var result;
  for (var o of OBJS) result = fn();
  return result;
}

test(x => x);

for (var j = 0; j < TESTS.length; ++j) {
  test(TESTS[j]);
}

for (var j = 0; j < TESTS.length; ++j) {
  var startTime = Date.now();
  test(TESTS[j]);
  console.log(TESTS[j].name + ':', (Date.now() - startTime), 'ms.');
}
========================================================================

We can see that thanks to earlier fixes we already made progress on this since 6.1:

========================================================================
$ ./d8-6.1.534.15 bench-polymorphic-species-clone.js
testClone: 1439 ms.
$ ./d8-master bench-polymorphic-species-clone.js
testClone: 828 ms.
========================================================================

 
Project Member Comment 1 by bugdroid1@chromium.org, Oct 4
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/947101c8734f7f67346e9414a5aefda00b948ae2

commit 947101c8734f7f67346e9414a5aefda00b948ae2
Author: Benedikt Meurer <bmeurer@chromium.org>
Date: Wed Oct 04 13:41:59 2017

[turbofan] Properly optimize polymorphic constructor call inlining.

This CL addresses a couple of minor issues that were in the way of
properly inlining polymorphic constructors calls, i.e. as found in
this common pattern using Symbol.species:

  class A {
    static get [Symbol.species]() { return this; }
    clone() { return new this.constructor[Symbol.species](); }
  }
  class B extends A {
    static get [Symbol.species]() { return this; }
  }

  function foo(o) { return o.clone(); }
  foo(new A());
  foo(new B());

Here the call to this.constructor[Symbol.species]() is the interesting
site. To get this fully inlined, we had to

  - make sure we don't introduce too many CheckHeapObject eagerly that
    block later optimizations (instead we try harder to see whether the
    receiver is already provably a HeapObject), and
  - also update the new.target of polymorphic JSConstruct nodes, when
    it refers to the same node as the target that we're specializing
    to (this way the JSCreate becomes fully inlinable later).

This seems to yield a solid 1.5% on the ARES6 ML benchmark (run via the
d8 cli runner), which confirms the previous profiled estimation. On the
micro-benchmark that specifically measures this feature in isolation we
go from

  testClone: 828 ms.

on V8 ToT as of today and

  testClone: 1439 ms.

on V8 6.1 to

  testClone: 219 ms.

which is a 3.7x improvement, on top of the previous ~2x boost that we
got from inlining the polymorphic symbol lookup.

Bug:  v8:6885 , v8:6278, v8:6344
Change-Id: Ida7abf683c7879978f181ba7f52a125f4f83ae6f
Reviewed-on: https://chromium-review.googlesource.com/700596
Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
Reviewed-by: Jaroslav Sevcik <jarin@chromium.org>
Commit-Queue: Benedikt Meurer <bmeurer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#48284}
[modify] https://crrev.com/947101c8734f7f67346e9414a5aefda00b948ae2/src/compiler/js-inlining-heuristic.cc
[modify] https://crrev.com/947101c8734f7f67346e9414a5aefda00b948ae2/src/compiler/property-access-builder.cc

Status: Fixed
Sign in to add a comment