New issue
Advanced search Search tips

Issue 6991 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Oct 2017
Cc:
Components:
HW: All
NextAction: ----
OS: All
Priority: 1
Type: Bug

Blocking:
issue 6936



Sign in to add a comment

Terrible deopt loop around property access in prettier benchmark

Project Member Reported by bmeu...@chromium.org, Oct 24 2017

Issue description

The prettier test in the web-tooling-benchmark seems to suffer from a pretty heavy deoptimization loop around the access

  args.needsSemi

where args can sometimes be a Smi. All of this is in the function printGenerically:

================================================================================
  function printGenerically(path, args) {
    const node = path.getValue();

    const shouldCache = node && typeof node === "object" && args === undefined;
    if (shouldCache && cache.has(node)) {
      return cache.get(node);
    }

    const parent = path.getParentNode(0);
    // We let JSXElement print its comments itself because it adds () around
    // UnionTypeAnnotation has to align the child without the comments
    let res;
    if (
      ((node && node.type === "JSXElement") ||
        (parent &&
          (parent.type === "UnionTypeAnnotation" ||
            parent.type === "TSUnionType" ||
            ((parent.type === "ClassDeclaration" ||
              parent.type === "ClassExpression") &&
              parent.superClass === node)))) &&
      !hasIgnoreComment(path)
    ) {
      res = genericPrint(path, options, printGenerically, args);
    } else {
      res = comments$3.printComments(
        path,
        p => genericPrint(p, options, printGenerically, args),
        options,
        args && args.needsSemi
      );
    }

    if (shouldCache) {
      cache.set(node, res);
    }

    return res;
  }
================================================================================

The problem seems to be on the TurboFan side. A simplified repro:

================================================================================
// Flags: --allow-natives-syntax --opt

function foo(o) { return o.x; }

assertEquals(undefined, foo({}));
assertEquals(undefined, foo(1));
assertEquals(undefined, foo({}));
assertEquals(undefined, foo(1));
%OptimizeFunctionOnNextCall(foo);
assertEquals(undefined, foo({}));
assertOptimized(foo);
assertEquals(undefined, foo(1));
assertOptimized(foo);
================================================================================
 
prettier.json
13.1 MB View Download
wtb-prettier.js
7.3 MB View Download
wtb-prettier.png
355 KB View Download
Project Member

Comment 2 by bugdroid1@chromium.org, Oct 24 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/a9da0ce73565a5b4357548686590cd0ae3735f68

commit a9da0ce73565a5b4357548686590cd0ae3735f68
Author: Benedikt Meurer <bmeurer@chromium.org>
Date: Tue Oct 24 09:19:47 2017

[turbofan] Properly handle smis in monomorphic loads/stores.

When lowering a monomorphic load/store, where multiple receiver maps
have been recorded, but the action to be performed is the same (i.e.
yielding undefined because the property is not found), TurboFan used
to ignore the Smi case, leading to a pretty terrible deoptimization
loop, as the LOAD_IC/STORE_IC properly recorded that state and thus
didn't change it's state.

Fixing this issue gives a 18-20% boost on the prettier test of the
web-tooling-benchmark, which was suffering a lot from this problem.

Bug: v8:6936,  v8:6991 
Change-Id: Id208ec7129a7f6b190d989bda31f936040393226
Reviewed-on: https://chromium-review.googlesource.com/735342
Reviewed-by: Michael Stanton <mvstanton@chromium.org>
Commit-Queue: Benedikt Meurer <bmeurer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#48865}
[modify] https://crrev.com/a9da0ce73565a5b4357548686590cd0ae3735f68/src/compiler/js-native-context-specialization.cc
[add] https://crrev.com/a9da0ce73565a5b4357548686590cd0ae3735f68/test/mjsunit/regress/regress-6991.js

Status: Fixed (was: Started)

Sign in to add a comment