New issue
Advanced search Search tips
Starred by 1 user
Status: Fixed
Owner:
Closed: Sep 26
Cc:
Components:
HW: All
OS: All
Priority: 1
Type: Bug



Sign in to add a comment
Literals not properly optimized in certain inlined functions
Project Member Reported by bmeu...@chromium.org, Sep 24 Back to list
When a function is inlined via SharedFunctionInfo based inlining instead of JSFunction based inlining, we don't (always) properly optimize literals inside the inlinee. I.e. in this simple example:

============================================
function foo() {
  function bar() { return { x: 1 }; };
  return bar().x;
}
============================================

The optimized code contains a call to the FastCloneShallowObject builtin instead of just escape analyzing away the unused literal. Changing the code slightly to

============================================
function bar() {
  return { x: 1 };
}

function foo() {
  return bar().x;
}
============================================

so that TurboFan inlines based on JSFunction, we don't emit any code for the literal, but just return 1 as expected.
 
Here's a simple micro-benchmark to illustrate the impact:

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

const createEmptyArrayLiteral = (function() {
  return function createEmptyArrayLiteral() {
    function foo() { return []; }
    return foo().length;
  }
})();

const createShallowArrayLiteral = (function() {
  return function createShallowArrayLiteral() {
    function foo() { return [1, 2, 3]; }
    return foo().length;
  }
})();

const createShallowObjectLiteral = (function() {
  return function createShallowObjectLiteral() {
    function foo() { return {length: 1}; }
    return foo().length;
  }
})();

var TESTS = [
    createEmptyArrayLiteral,
    createShallowArrayLiteral,
    createShallowObjectLiteral
];
var n = 1e8;

function test(fn) {
  var result;
  for (var i = 0; i < n; ++i) 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.');
}
====================================================================

In all cases the literals should be escape analyzed away. On ToT we currently get

====================================================================
createEmptyArrayLiteral: 1846 ms.
createShallowArrayLiteral: 1868 ms.
createShallowObjectLiteral: 2246 ms.
====================================================================

while properly removing the literals, we can achieve

====================================================================
createEmptyArrayLiteral: 1175 ms.
createShallowArrayLiteral: 1187 ms.
createShallowObjectLiteral: 1195 ms.
====================================================================

and properly a lot more once the closure allocation is inlined.
Project Member Comment 2 by bugdroid1@chromium.org, Sep 25
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/855b88ae5a569b236cab0211a5702ed2ad4ee0c3

commit 855b88ae5a569b236cab0211a5702ed2ad4ee0c3
Author: Benedikt Meurer <bmeurer@chromium.org>
Date: Mon Sep 25 13:05:16 2017

[turbofan] Properly optimize literals in inlined functions.

When inlining based on SharedFunctionInfo rather than based on concrete
JSFunction, we weren't able to properly optimize array, object and
regexp literals inside the inlinee, because we didn't know the concrete
FeedbackVector for the inlinee inside JSCreateLowering. This was because
JSCreateLowering wasn't properly updated after the literals moved to the
FeedbackVector. Now with this CL we also have the VectorSlotPair on the
literal creation operators, just like we do for property accesses and
calls, and are thus able to always access the appropriate FeedbackVector
and optimize the literal creation.

The impact is illustrated by the micro-benchmark on the tracking bug,
which goes from

  createEmptyArrayLiteral: 1846 ms.
  createShallowArrayLiteral: 1868 ms.
  createShallowObjectLiteral: 2246 ms.

to

  createEmptyArrayLiteral: 1175 ms.
  createShallowArrayLiteral: 1187 ms.
  createShallowObjectLiteral: 1195 ms.

with this CL, so up to 2x faster now.

Drive-by-fix: Also remove the unused CreateEmptyObjectLiteral builtin
and cleanup the names of the other builtins to be consistent with the
names of the TurboFan operators and Ignition bytecodes.

Bug:  v8:6856 
Change-Id: I453828d019b27c9aa1344edac0dd84e91a457097
Reviewed-on: https://chromium-review.googlesource.com/680656
Commit-Queue: Benedikt Meurer <bmeurer@chromium.org>
Reviewed-by: Yang Guo <yangguo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#48140}
[modify] https://crrev.com/855b88ae5a569b236cab0211a5702ed2ad4ee0c3/src/arm/interface-descriptors-arm.cc
[modify] https://crrev.com/855b88ae5a569b236cab0211a5702ed2ad4ee0c3/src/arm64/interface-descriptors-arm64.cc
[modify] https://crrev.com/855b88ae5a569b236cab0211a5702ed2ad4ee0c3/src/ast/ast.cc
[modify] https://crrev.com/855b88ae5a569b236cab0211a5702ed2ad4ee0c3/src/ast/ast.h
[modify] https://crrev.com/855b88ae5a569b236cab0211a5702ed2ad4ee0c3/src/builtins/builtins-constructor-gen.cc
[modify] https://crrev.com/855b88ae5a569b236cab0211a5702ed2ad4ee0c3/src/builtins/builtins-constructor-gen.h
[modify] https://crrev.com/855b88ae5a569b236cab0211a5702ed2ad4ee0c3/src/builtins/builtins-definitions.h
[modify] https://crrev.com/855b88ae5a569b236cab0211a5702ed2ad4ee0c3/src/builtins/builtins.cc
[modify] https://crrev.com/855b88ae5a569b236cab0211a5702ed2ad4ee0c3/src/builtins/builtins.h
[modify] https://crrev.com/855b88ae5a569b236cab0211a5702ed2ad4ee0c3/src/code-factory.cc
[modify] https://crrev.com/855b88ae5a569b236cab0211a5702ed2ad4ee0c3/src/code-factory.h
[modify] https://crrev.com/855b88ae5a569b236cab0211a5702ed2ad4ee0c3/src/compiler/bytecode-graph-builder.cc
[modify] https://crrev.com/855b88ae5a569b236cab0211a5702ed2ad4ee0c3/src/compiler/js-create-lowering.cc
[modify] https://crrev.com/855b88ae5a569b236cab0211a5702ed2ad4ee0c3/src/compiler/js-create-lowering.h
[modify] https://crrev.com/855b88ae5a569b236cab0211a5702ed2ad4ee0c3/src/compiler/js-generic-lowering.cc
[modify] https://crrev.com/855b88ae5a569b236cab0211a5702ed2ad4ee0c3/src/compiler/js-operator.cc
[modify] https://crrev.com/855b88ae5a569b236cab0211a5702ed2ad4ee0c3/src/compiler/js-operator.h
[modify] https://crrev.com/855b88ae5a569b236cab0211a5702ed2ad4ee0c3/src/compiler/pipeline.cc
[modify] https://crrev.com/855b88ae5a569b236cab0211a5702ed2ad4ee0c3/src/ia32/interface-descriptors-ia32.cc
[modify] https://crrev.com/855b88ae5a569b236cab0211a5702ed2ad4ee0c3/src/interface-descriptors.cc
[modify] https://crrev.com/855b88ae5a569b236cab0211a5702ed2ad4ee0c3/src/interface-descriptors.h
[modify] https://crrev.com/855b88ae5a569b236cab0211a5702ed2ad4ee0c3/src/interpreter/bytecode-generator.cc
[modify] https://crrev.com/855b88ae5a569b236cab0211a5702ed2ad4ee0c3/src/interpreter/interpreter-generator.cc
[modify] https://crrev.com/855b88ae5a569b236cab0211a5702ed2ad4ee0c3/src/mips/interface-descriptors-mips.cc
[modify] https://crrev.com/855b88ae5a569b236cab0211a5702ed2ad4ee0c3/src/mips64/interface-descriptors-mips64.cc
[modify] https://crrev.com/855b88ae5a569b236cab0211a5702ed2ad4ee0c3/src/ppc/interface-descriptors-ppc.cc
[modify] https://crrev.com/855b88ae5a569b236cab0211a5702ed2ad4ee0c3/src/runtime/runtime-literals.cc
[modify] https://crrev.com/855b88ae5a569b236cab0211a5702ed2ad4ee0c3/src/s390/interface-descriptors-s390.cc
[modify] https://crrev.com/855b88ae5a569b236cab0211a5702ed2ad4ee0c3/src/x64/interface-descriptors-x64.cc
[modify] https://crrev.com/855b88ae5a569b236cab0211a5702ed2ad4ee0c3/test/unittests/compiler/js-create-lowering-unittest.cc

Status: Fixed
Sign in to add a comment