New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 3354 link

Starred by 19 users

Issue metadata

Status: WontFix
Owner:
Closed: Oct 2015
Cc:
HW: All
NextAction: ----
OS: All
Priority: 2
Type: Bug



Sign in to add a comment

Inlining heuristics still take source size into account

Project Member Reported by vegorov@chromium.org, May 28 2014

Issue description

InliningAstSize contains the following code:

  // Do a quick check on source code length to avoid parsing large
  // inlining candidates.
  if (target_shared->SourceSize() >
      Min(FLAG_max_inlined_source_size, kUnlimitedMaxInlinedSourceSize)) {
    TraceInline(target, caller, "target text too big");
    return kNotInlinable;
  }

https://code.google.com/p/v8/source/browse/trunk/src/hydrogen.cc?r=21543#7280

This check historically made sense because V8 originally did not persist AST counts on the SharedFunctionInfo. These days however we have ast_nodes() information directly on the SFI and this heuristic does not make much sense.

Taking raw source size into account penalizes well-commented short function in the environments which run unminified code by default (e.g. node.js).
 

Comment 1 Deleted

Labels: OS-All HW-All
Owner: bmeu...@chromium.org
Status: Assigned
Cc: danno@chromium.org svenpanne@chromium.org mstarzinger@chromium.org
We looked into that some time ago, it's actually pretty easy (CL that does this https://codereview.chromium.org/306883002), but it tanks Octane (Richards and DeltaBlue actually, although it improves EarleyBoyer).
I see, indeed I can repro somewhat lower scores with --max_inlined_source_size=800.

It kinda strange that if I say just minify identifiers and remove whitespace in DeltaBlue without changing any code then it will become a bit slower.

Maybe inlining heuristics need to be tweaked in some other place to offset more aggressive inlining? Why does more aggressive inlining lead to worse scores in the first place?
From what I can tell, the aforementioned CL that tanked Octane didn't *replace* the inline source-size check with an AST-size check, it just **removed the size limit altogether** - not too surprising that inlining all functions regardless of size would cause a slowdown.

Has *actually using the size of the AST* been tried yet?
#5: Look closer. When you have two limits, and you remove one, that leaves the other.
Status: WontFix
I don't think we will touch this part of Crankshaft again. This will likely be very different in TurboFan.
#6: How does the patch have two limits? The only checks I see apart from the removed source size limit are:

- IsInlineable (which, from what I can tell, only checks if the function is stack-allocated, ie. if it's a parameter or local)
- dont_inline / dont_optimize (which appear to check whether the target function use unsupported/unoptimizable syntax)
Project Member

Comment 10 by bugdroid1@chromium.org, Sep 23 2016

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

commit 1b33028607637ba8cb2d60433811f825b00a0cc1
Author: Benedikt Meurer <bmeurer@chromium.org>
Date: Fri Sep 23 11:47:07 2016

[turbofan] Don't take into account source size for inlining heuristics.

The source size is not a real indicator for whether or not to inline a
certain function.

R=ishell@chromium.org, jarin@chromium.org
BUG= v8:3354 ,v8:5267

Review URL: https://codereview.chromium.org/2361813002 .

Cr-Commit-Position: refs/heads/master@{#39659}

[modify] https://crrev.com/1b33028607637ba8cb2d60433811f825b00a0cc1/src/compiler/js-inlining-heuristic.cc
[modify] https://crrev.com/1b33028607637ba8cb2d60433811f825b00a0cc1/test/cctest/test-heap-profiler.cc
[modify] https://crrev.com/1b33028607637ba8cb2d60433811f825b00a0cc1/test/mjsunit/es6/tail-call-megatest.js

Project Member

Comment 11 by bugdroid1@chromium.org, Sep 23 2016

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

commit 99160dc1c124d39c5d9e1eeb14bb363ab554ce6c
Author: machenbach <machenbach@chromium.org>
Date: Fri Sep 23 12:43:23 2016

Revert of [turbofan] Don't take into account source size for inlining heuristics. (patchset #4 id:60001 of https://codereview.chromium.org/2361813002/ )

Reason for revert:
timeouts on windows:
https://build.chromium.org/p/client.v8/builders/V8%20Win64%20-%20debug/builds/12504

Original issue's description:
> [turbofan] Don't take into account source size for inlining heuristics.
>
> The source size is not a real indicator for whether or not to inline a
> certain function.
>
> R=ishell@chromium.org, jarin@chromium.org
> BUG= v8:3354 ,v8:5267
>
> Committed: https://chromium.googlesource.com/v8/v8/+/1b33028607637ba8cb2d60433811f825b00a0cc1

TBR=ishell@chromium.org,jarin@chromium.org,bmeurer@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= v8:3354 ,v8:5267

Review-Url: https://codereview.chromium.org/2362853003
Cr-Commit-Position: refs/heads/master@{#39661}

[modify] https://crrev.com/99160dc1c124d39c5d9e1eeb14bb363ab554ce6c/src/compiler/js-inlining-heuristic.cc
[modify] https://crrev.com/99160dc1c124d39c5d9e1eeb14bb363ab554ce6c/test/cctest/test-heap-profiler.cc
[modify] https://crrev.com/99160dc1c124d39c5d9e1eeb14bb363ab554ce6c/test/mjsunit/es6/tail-call-megatest.js

Comment 13 Deleted

Labels: Priority-2

Sign in to add a comment