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

Blocking:
issue 5996
issue 6936



Sign in to add a comment
Optimize Reflect.has and Reflect.get
Project Member Reported by bmeu...@chromium.org, Oct 16 Back to list
The chai test in the web-tooling-benchmark (http://github.com/bmeurer/web-tooling-benchmark) seems to spend a significant amount of time in the Reflect.get and Reflect.has builtins. Running that test separately with --runtime-call-stats reveals:

                      Runtime Function/C++ Builtin        Time             Count
========================================================================================
                                      JS_Execution   3158.37ms  50.59%    304375   2.64%
                                       SetProperty    380.57ms   6.10%   2007921  17.44%
                                        ReflectGet    324.64ms   5.20%   3134862  27.22%
                            ErrorCaptureStackTrace    296.94ms   4.76%     21330   0.19%
                                        ReflectHas    232.47ms   3.72%   3135186  27.23%

That's roughly 9% of the overall time just executing those builtins. Currently both are implemented as C++ builtins, but we generally have the relevant bits to turn them into something faster in the CSA land already. And we could also teach TurboFan about them.
 
Status: Available
Comment 2 Deleted
Here's a simple micro-benchmark to measure the overhead of Reflect.get / Reflect.has:

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

const N = 1e7;
const TESTS = [];

(function() {
  const o = {'present': 1};

  TESTS.push(function reflectHasPresent() {
    return Reflect.has(o, 'present');
  }, function reflectHasAbsent() {
    return Reflect.has(o, 'absent');
  }, function reflectGetPresent() {
    return Reflect.get(o, 'present');
  }, function reflectGetAbsent() {
    return Reflect.get(o, 'absent');
  });
})();

function test(fn) {
  var result;
  for (var i = 0; i < N; i += 1) result = fn();
  return result;
}

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.');
}
=====================================================================

On V8 ToT we get:

=====================================================================
$ out/Release/d8 bench-reflect.js
reflectHasPresent: 340 ms.
reflectHasAbsent: 454 ms.
reflectGetPresent: 473 ms.
reflectGetAbsent: 475 ms.
=====================================================================

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

commit 4213af64b6b9c9d04be89303169562460d38df84
Author: Benedikt Meurer <bmeurer@chromium.org>
Date: Mon Oct 16 17:27:41 2017

[es2015] Optimize Reflect.has builtin.

Port the baseline version of Reflect.has to the CodeStubAssembler and
reuse the existing logic for HasProperty (i.e. the HasProperty builtin).
Also inline the Reflect.has builtin into TurboFan, by adding a check
on the target in front of a use of the JSHasProperty operator.
Technically this additional check is not necessary, because the
JSHasProperty operator already throws if the target is not a JSReceiver,
but the exception message is confusing then.

This improves the performance of the micro-benchmark from

  reflectHasPresent: 337 ms.
  reflectHasAbsent: 472 ms.

to

  reflectHasPresent: 121 ms.
  reflectHasAbsent: 216 ms.

which is a nice 2.8x improvement in the best case. It also improves the
chai test on the web-tooling-benchmark by around 1-2%, which is roughly
the expected win (since Reflect.has overall accounts for around 3-4%).

Bug: v8:5996, v8:6936,  v8:6937 
Change-Id: I856183229677a71c19936f06f2a4fc7a794a9a4a
Reviewed-on: https://chromium-review.googlesource.com/720959
Commit-Queue: Benedikt Meurer <bmeurer@chromium.org>
Reviewed-by: Jaroslav Sevcik <jarin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#48608}
[modify] https://crrev.com/4213af64b6b9c9d04be89303169562460d38df84/BUILD.gn
[modify] https://crrev.com/4213af64b6b9c9d04be89303169562460d38df84/src/builtins/builtins-definitions.h
[modify] https://crrev.com/4213af64b6b9c9d04be89303169562460d38df84/src/builtins/builtins-promise-gen.cc
[modify] https://crrev.com/4213af64b6b9c9d04be89303169562460d38df84/src/builtins/builtins-promise-gen.h
[add] https://crrev.com/4213af64b6b9c9d04be89303169562460d38df84/src/builtins/builtins-reflect-gen.cc
[modify] https://crrev.com/4213af64b6b9c9d04be89303169562460d38df84/src/builtins/builtins-reflect.cc
[modify] https://crrev.com/4213af64b6b9c9d04be89303169562460d38df84/src/code-stub-assembler.cc
[modify] https://crrev.com/4213af64b6b9c9d04be89303169562460d38df84/src/code-stub-assembler.h
[modify] https://crrev.com/4213af64b6b9c9d04be89303169562460d38df84/src/compiler/js-call-reducer.cc
[modify] https://crrev.com/4213af64b6b9c9d04be89303169562460d38df84/src/compiler/js-call-reducer.h
[modify] https://crrev.com/4213af64b6b9c9d04be89303169562460d38df84/src/v8.gyp
[add] https://crrev.com/4213af64b6b9c9d04be89303169562460d38df84/test/mjsunit/compiler/reflect-has.js

Project Member Comment 5 by bugdroid1@chromium.org, Oct 23
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/35614b7215ed56a8653787fadfba4609b1237733

commit 35614b7215ed56a8653787fadfba4609b1237733
Author: Benedikt Meurer <bmeurer@chromium.org>
Date: Mon Oct 23 17:42:38 2017

[turbofan] Optimize Reflect.get(target, key) calls.

When TurboFan sees a call to Reflect.get with exactly two parameters,
we can lower that to a direct call to the GetPropertyStub, which is
certainly faster than the general C++ builtin. This gives a nice
7-8% improvement on the chai test in the web-tooling-benchmark.

The micro-benchmark on the issue goes from

  reflectGetPresent: 461 ms.
  reflectGetAbsent: 470 ms.

to 

  reflectGetPresent: 141 ms.
  reflectGetAbsent: 245 ms.

which is an up to 3.2x improvement.

Bug: v8:5996, v8:6936,  v8:6937 
Change-Id: Ic439fccb13f1a2f84386bf9fc31b4283d101afc4
Reviewed-on: https://chromium-review.googlesource.com/732988
Commit-Queue: Benedikt Meurer <bmeurer@chromium.org>
Reviewed-by: Michael Stanton <mvstanton@chromium.org>
Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#48841}
[modify] https://crrev.com/35614b7215ed56a8653787fadfba4609b1237733/src/compiler/js-call-reducer.cc
[modify] https://crrev.com/35614b7215ed56a8653787fadfba4609b1237733/src/compiler/js-call-reducer.h
[add] https://crrev.com/35614b7215ed56a8653787fadfba4609b1237733/test/mjsunit/compiler/reflect-get.js

Status: Fixed
Sign in to add a comment