Issue 11275290: Count per check hits in ICData.

Issue 11275290: Count per check hits in ICData. (Closed)

Created:
8 years, 1 month ago by Vyacheslav Egorov (Google)

Modified:
8 years ago

Reviewers:
Kevin Millikin (Google), srdjan

CC:
reviews_dartlang.org

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

Visibility:
Public.

More Reviews

Description

Count per check hits in ICData. R=srdjan@google.com BUG= Committed: https://code.google.com/p/dart/source/detail?r=14870

Patch Set 1 #

Total comments: 9

Created: 8 years, 1 month ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Stats (+101 lines, -27 lines)			Patch
M	runtime/vm/il_printer.cc	View	2 chunks	+5 lines, -1 line	0 comments	Download
M	runtime/vm/object.h	View	3 chunks	+14 lines, -1 line	5 comments	Download
M	runtime/vm/object.cc	View	6 chunks	+31 lines, -6 lines	2 comments	Download
M	runtime/vm/stub_code_ia32.cc	View	5 chunks	+25 lines, -9 lines	0 comments	Download
M	runtime/vm/stub_code_x64.cc	View	6 chunks	+26 lines, -10 lines	2 comments	Download

Messages

Total messages: 5 (0 generated)

Expand Messages | Collapse Messages

srdjan

LGTM https://codereview.chromium.org/11275290/diff/1/runtime/vm/object.h File runtime/vm/object.h (right): https://codereview.chromium.org/11275290/diff/1/runtime/vm/object.h#newcode2857 runtime/vm/object.h:2857: intptr_t* count = NULL) const; Maybe it is ...

8 years, 1 month ago (2012-11-13 20:58:14 UTC) #3

Kevin Millikin (Google)

LGTM with some simple comments. https://codereview.chromium.org/11275290/diff/1/runtime/vm/object.cc File runtime/vm/object.cc (right): https://codereview.chromium.org/11275290/diff/1/runtime/vm/object.cc#newcode7548 runtime/vm/object.cc:7548: if ((receiver_class_id == kSmiCid) ...

8 years, 1 month ago (2012-11-13 20:59:33 UTC) #4

Vyacheslav Egorov (Google)

8 years ago (2012-12-17 12:54:16 UTC) #5

Message was sent while issue was closed.

[publishing stale comments]

https://codereview.chromium.org/11275290/diff/1/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/11275290/diff/1/runtime/vm/object.cc#newcode7548
runtime/vm/object.cc:7548: if ((receiver_class_id == kSmiCid) && (data_pos > 0))
{
On 2012/11/13 20:59:33, kmillikin wrote:
> This is pretty messy.  How about:
> 
> if ((receiver_class_id == kSmiCid) && (data_pos > 0)) {
>   ASSERT(GetReceiverClassIdAt(0) != kSmiCid);  // Simple duplicate entry
check.
>   data.SetAt(data_pos, Smi::Handle(Smi::New(GetReceiverClassIdAt(0))));
>   data.SetAt(data_pos + 1, Function::Handle(GetTargetAt(0)));
>   data.SetAt(data_pos + 2, Smi::Handle(Smi::Handle(Smi::New(GetCountAt(0))));
>   data_pos = 0;
> }
> data.SetAt(data_pos, Smi::Handle(Smi::New(receiver_class_id)));
> data.SetAt(data_pos + 1, target);
> data.SetAt(data_pos + 2, Smi::Handle(Smi::New(1)));

Done.

https://codereview.chromium.org/11275290/diff/1/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/11275290/diff/1/runtime/vm/object.h#newcode2857
runtime/vm/object.h:2857: intptr_t* count = NULL) const;
On 2012/11/13 20:58:14, srdjan wrote:
> Maybe it is OK if this method remains unchanged and use GetCountAt to access
> counts?

Done.

https://codereview.chromium.org/11275290/diff/1/runtime/vm/object.h#newcode2887
runtime/vm/object.h:2887: static intptr_t TargetIndexFor(intptr_t num_args) {
On 2012/11/13 20:59:33, kmillikin wrote:
> Not an Index, but an Offset (from the beginning of the entry).

Done.

https://codereview.chromium.org/11275290/diff/1/runtime/vm/stub_code_x64.cc
File runtime/vm/stub_code_x64.cc (right):

https://codereview.chromium.org/11275290/diff/1/runtime/vm/stub_code_x64.cc#n...
runtime/vm/stub_code_x64.cc:2108: // Calls to runtime to optimized give function
On 2012/11/13 20:59:33, kmillikin wrote:
> While you're fixing ooptimized, you might as well fix "given" , make it a
> complete sentence, and add some articles ("the runtime", "the given
function"),
> etc.

Done.

Expand Messages | Collapse Messages