New issue
Advanced search Search tips
Starred by 2 users
Status: Fixed
Owner:
Closed: Oct 24
Cc:
Components:
HW: All
OS: All
Priority: 1
Type: Bug

Blocking:
issue 6936



Sign in to add a comment
Lot's of %SetProperty calls for dictionaries
Project Member Reported by bmeu...@chromium.org, Oct 23 Back to list
In the profile for the chai benchmark we see lots of calls to the %SetProperty runtime function. Almost all of these come from the generic KeyedStoreIC. It seems that the logic to decide whether the prototype chain is fine has a bug where it would go to the "bailout" label if the prototype is null, which was supposed to be the fast-path. That means that for example the transferFlags module always hits the slow-path:

================================================================================================
module.exports = function transferFlags(assertion, object, includeAll) {
  var flags = assertion.__flags || (assertion.__flags = Object.create(null));

  if (!object.__flags) {
    object.__flags = Object.create(null);
  }

  includeAll = arguments.length === 3 ? includeAll : true;

  for (var flag in flags) {
    if (includeAll ||
        (flag !== 'object' && flag !== 'ssfi' && flag !== 'lockSsfi' && flag != 'message')) {
      object.__flags[flag] = flags[flag];
    }
  }
};
================================================================================================

In addition to that, we also seem to call %SetProperty when we need to rehash/grow, although there's already a proper runtime function %AddDictionaryProperty for this. It looks like picking these two low hanging fruits should make it possible to considerably reduce the 6-8% overhead of %SetProperty in the chai test.
 
chai.json
7.0 MB View Download
wtb-chai.js
1.1 MB View Download
wtb-chai.png
696 KB View Download
Cc: jarin@chromium.org
This micro-benchmark illustrates the problem of storing to DICTIONARY vs FAST mode objects, which goes against the advice to use Object.create(null) for dictionaries (instead of using empty object literal).

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

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

(function() {
  const KEYS = ["a", "b", "c", "d"];

  function storeToDictionary() {
    const dict = Object.create(null);
    for (const key of KEYS) dict[key] = key;
    return dict;
  }

  function storeToFast() {
    const fast = {};
    for (const key of KEYS) fast[key] = key;
    return fast;
  }

  TESTS.push(
      storeToDictionary,
      storeToFast);
})();

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

Running this on V8 ToT we get:

====================================================================
$ ./d8-master bench-dictionaries.js
storeToDictionary: 559 ms.
storeToFast: 95 ms.
====================================================================

So initializing a simple dictionary is almost 6x slower than initializing a fast mode object via keyed stores. In both cases the KeyedStoreIC is MEGAMORPHIC. The fast mode benchmark is always hitting the megamorphic stub cache.
Project Member Comment 2 by bugdroid1@chromium.org, Oct 24
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/a9b098013d18fbf285c45edb3d29b0fa646633ac

commit a9b098013d18fbf285c45edb3d29b0fa646633ac
Author: Benedikt Meurer <bmeurer@chromium.org>
Date: Tue Oct 24 08:42:32 2017

[ic] Improve KeyedStoreIC performance for dictionaries.

Once the KeyedStoreIC was in MEGAMORPHIC state storing to dictionary
mode objects, we'd constantly hit the slow-path implemented via the
%SetProperty runtime function, if the dictionary was created with a
null prototype, i.e. via Object.create(null). This goes against the
advice of using Object.create(null) for dictionaries (compared to
using empty object literal), which is unfortunate.

This CL addresses two issues, starting with

- adding support for null prototypes to LookupPropertyOnPrototypeChain,
  which was always hitting the slow path for null prototypes, and
- using the dedicated %AddDictionaryProperty runtime call when we
  have to grow the backing store.

These changes combined improve the micro-benchmark from

  storeToDictionary: 559 ms.
  storeToFast: 95 ms.

to

  storeToDictionary: 201 ms.
  storeToFast: 94 ms.

which reduces overhead by about 65%. This overall improves the chai test
on the web-tooling-benchmark by about 4%, which still leaves some room
for improvement.

Bug: v8:6936,  v8:6985 
Change-Id: I97b78961f51edb3a3e198bdb31457fd78bed947f
Reviewed-on: https://chromium-review.googlesource.com/735139
Reviewed-by: Igor Sheludko <ishell@chromium.org>
Commit-Queue: Benedikt Meurer <bmeurer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#48858}
[modify] https://crrev.com/a9b098013d18fbf285c45edb3d29b0fa646633ac/src/ic/keyed-store-generic.cc

Status: Fixed
Sign in to add a comment