Issue 11541002: Use call counts to prevent cold calls from being inlined.

Issue 11541002: Use call counts to prevent cold calls from being inlined. (Closed)

Created:
8 years ago by srdjan

Modified:
8 years ago

Reviewers:
Vyacheslav Egorov (Google), Kevin Millikin (Google)

CC:
reviews_dartlang.org

Base URL:
http://dart.googlecode.com/svn/branches/bleeding_edge/dart/

Visibility:
Public.

More Reviews

Description

Use call counts to prevent cold calls from being inlined. Committed: https://code.google.com/p/dart/source/detail?r=16269

Patch Set 8 : #

Created: 8 years ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+71 lines, -21 lines)			Patch
M	runtime/vm/flow_graph_inliner.cc	View	1 2 3 4 5 6 7	9 chunks	+60 lines, -21 lines	0 comments	Download
M	runtime/vm/object.h	View	1 2 3 4 5 6 7	1 chunk	+1 line, -0 lines	0 comments	Download
M	runtime/vm/object.cc	View	1 2 3 4 5 6 7	1 chunk	+10 lines, -0 lines	0 comments	Download

Messages

Total messages: 6 (0 generated)

Expand Messages | Collapse Messages

Vyacheslav Egorov (Google)

https://codereview.chromium.org/11541002/diff/9001/runtime/vm/flow_graph_inliner.cc File runtime/vm/flow_graph_inliner.cc (right): https://codereview.chromium.org/11541002/diff/9001/runtime/vm/flow_graph_inliner.cc#newcode41 runtime/vm/flow_graph_inliner.cc:41: "default 20%: calls above-equal 20% of max-count are inlined."); ...

8 years ago (2012-12-17 13:16:04 UTC) #3

srdjan

8 years ago (2012-12-17 22:46:13 UTC) #4

https://codereview.chromium.org/11541002/diff/9001/runtime/vm/flow_graph_inli...
File runtime/vm/flow_graph_inliner.cc (right):

https://codereview.chromium.org/11541002/diff/9001/runtime/vm/flow_graph_inli...
runtime/vm/flow_graph_inliner.cc:41: "default 20%: calls above-equal 20% of
max-count are inlined.");
On 2012/12/17 13:16:05, Vyacheslav Egorov (Google) wrote:
> comment does not match the default value.

Done.

https://codereview.chromium.org/11541002/diff/9001/runtime/vm/flow_graph_inli...
runtime/vm/flow_graph_inliner.cc:201: const
GrowableArray<PolymorphicInstanceCallInstr*>& instance_calls() const {
On 2012/12/17 13:16:05, Vyacheslav Egorov (Google) wrote:
> I'd prefer that this array contained a two field structure instead of having
two
> disconnected arrays with an assumption.

Done.

https://codereview.chromium.org/11541002/diff/9001/runtime/vm/flow_graph_inli...
runtime/vm/flow_graph_inliner.cc:675: calls[i]->ic_data().AggregateCount() *
calls_caller_count[i];
On 2012/12/17 13:16:05, Vyacheslav Egorov (Google) wrote:
> I can't fully grasp the physical meaning behind this multiplication.
> 
> If we a descending through the call chain then as we go deeper call counts in
> the callee are already in some sense including the call count of the caller.
But
> at the same time as we descend down we might encounter counters induced by
some
> other callchains to become higher.
> 
> Why do we multiply them?
> 
> Compare two call chains:
> 
> Imagine the following code:
> 
> A() { B() /* 10 */; D() /* 10 */; }
> B() { C() /* 10 */; }
> C() { /* code */ }
> D() { E() /* 60 */; }
> E() { /* code */ }
> Z() { D(); /* 50 */ }
> 
> for (var i = 0; i < 50; i++) Z();
> for (var i = 0; i < 10; i++) A();
> 
> According to this formula when compiling A and inlining both B and D we'll get
> callsite C with count 100 and callsite E with count 600. This does not make
much
> sense to me. It contradicts the fact that call paths A => B => C and A => D =>
E
> is equally hot.


You are right. Changed to use a ratio per one scope.

Vyacheslav Egorov (Google)

lgtm https://codereview.chromium.org/11541002/diff/21003/runtime/vm/flow_graph_inliner.cc File runtime/vm/flow_graph_inliner.cc (right): https://codereview.chromium.org/11541002/diff/21003/runtime/vm/flow_graph_inliner.cc#newcode250 runtime/vm/flow_graph_inliner.cc:250: call-> ic_data().AggregateCount(); remove space after ->

8 years ago (2012-12-18 11:55:51 UTC) #5

srdjan

8 years ago (2012-12-18 16:37:51 UTC) #6

Thanks!

https://codereview.chromium.org/11541002/diff/21003/runtime/vm/flow_graph_inl...
File runtime/vm/flow_graph_inliner.cc (right):

https://codereview.chromium.org/11541002/diff/21003/runtime/vm/flow_graph_inl...
runtime/vm/flow_graph_inliner.cc:250: call-> ic_data().AggregateCount();
On 2012/12/18 11:55:51, Vyacheslav Egorov (Google) wrote:
> remove space after ->

Done.

Expand Messages | Collapse Messages