Issue 9866030: Move profiler_ticks to Code object, don't walk the stack when patching ICs

Issue 9866030: Move profiler_ticks to Code object, don't walk the stack when patching ICs (Closed)

Created:
8 years, 9 months ago by Jakob Kummerow

Modified:
8 years, 9 months ago

Reviewers:
ulan

CC:
v8-dev

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

Visibility:
Public.

More Reviews

Description

Move profiler_ticks to Code object, don't walk the stack when patching ICs Committed: https://code.google.com/p/v8/source/detail?r=11162

Patch Set 2 : addressed comments #

Created: 8 years, 9 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+90 lines, -92 lines)			Patch
M	src/full-codegen.cc	View		1 chunk	+1 line, -0 lines	0 comments	Download
M	src/heap.cc	View	1	2 chunks	+1 line, -2 lines	0 comments	Download
M	src/ic.cc	View	1	1 chunk	+32 lines, -46 lines	0 comments	Download
M	src/objects.h	View	1	8 chunks	+16 lines, -15 lines	0 comments	Download
M	src/objects.cc	View		1 chunk	+1 line, -1 line	0 comments	Download
M	src/objects-inl.h	View	1	5 chunks	+15 lines, -8 lines	0 comments	Download
M	src/objects-printer.cc	View	1	1 chunk	+2 lines, -2 lines	0 comments	Download
M	src/runtime.cc	View		2 chunks	+1 line, -2 lines	0 comments	Download
M	src/runtime-profiler.cc	View	1	7 chunks	+21 lines, -16 lines	0 comments	Download

Messages

Total messages: 3 (0 generated)

Expand Messages | Collapse Messages

ulan

LGTM with comments. https://chromiumcodereview.appspot.com/9866030/diff/1/src/full-codegen.cc File src/full-codegen.cc (right): https://chromiumcodereview.appspot.com/9866030/diff/1/src/full-codegen.cc#newcode330 src/full-codegen.cc:330: code->set_profiler_ticks(0); Shouldn't it be set in ...

8 years, 9 months ago (2012-03-27 11:40:57 UTC) #2

Jakob Kummerow

8 years, 9 months ago (2012-03-27 12:19:10 UTC) #3

Thanks for the review. Comments addressed, landing.

https://chromiumcodereview.appspot.com/9866030/diff/1/src/full-codegen.cc
File src/full-codegen.cc (right):

https://chromiumcodereview.appspot.com/9866030/diff/1/src/full-codegen.cc#new...
src/full-codegen.cc:330: code->set_profiler_ticks(0);
On 2012/03/27 11:40:58, ulan wrote:
> Shouldn't it be set in Heap::CreateCode ?

No, because the field only exists for Code objects with kind() == FUNCTION. All
FUNCTION Code objects pass through here, whereas Heap::CreateCode is used for
all sorts of Code objects.

https://chromiumcodereview.appspot.com/9866030/diff/1/src/ic.cc
File src/ic.cc (right):

https://chromiumcodereview.appspot.com/9866030/diff/1/src/ic.cc#newcode299
src/ic.cc:299: static int ComputeTypeinfoCountDelta(IC::State old_state,
IC::State new_state) {
On 2012/03/27 11:40:58, ulan wrote:
> Uppercase i in TypeInfo is better.

Done.

https://chromiumcodereview.appspot.com/9866030/diff/1/src/ic.cc#newcode304
src/ic.cc:304: if (was_uninitialized && !is_uninitialized) {
On 2012/03/27 11:40:58, ulan wrote:
> You may disagree, but I like:
> return (was_uninitialized && !is_uninitialized) ? 1 :
>        (!was_uninitialized && is_uninitialized) ? -1 : 0

Done.

https://chromiumcodereview.appspot.com/9866030/diff/1/src/ic.cc#newcode314
src/ic.cc:314: if (FLAG_type_info_threshold == 0 &&
On 2012/03/27 11:40:58, ulan wrote:
> Doesn't it fit into one line?

Done.

https://chromiumcodereview.appspot.com/9866030/diff/1/src/ic.cc#newcode320
src/ic.cc:320: if (host->kind() != Code::FUNCTION) return;
On 2012/03/27 11:40:58, ulan wrote:
> I am not sure here. Is it safe to ignore other kinds like builtins?

Yes, because only Code objects with kind() == FUNCTION can be optimized (see
Code::optimizable()), and we're only interested in optimizable code here.

https://chromiumcodereview.appspot.com/9866030/diff/1/src/objects-inl.h
File src/objects-inl.h (right):

https://chromiumcodereview.appspot.com/9866030/diff/1/src/objects-inl.h#newco...
src/objects-inl.h:3093: return READ_BYTE_FIELD(this, kProfilerTicksOffset);
On 2012/03/27 11:40:58, ulan wrote:
> Now profiler ticks have to fit into one byte, and we should be careful to
avoid
> overflow. Can we put a comment in runtime-profiler that the line "else if
(ticks
> >= 100) {" is important.

Done.

https://chromiumcodereview.appspot.com/9866030/diff/1/src/objects-inl.h#newco...
src/objects-inl.h:3097: void Code::set_profiler_ticks(int ticks) {
On 2012/03/27 11:40:58, ulan wrote:
> ASSERT(ticks < 256);

Done.

https://chromiumcodereview.appspot.com/9866030/diff/1/src/runtime-profiler.cc
File src/runtime-profiler.cc (right):

https://chromiumcodereview.appspot.com/9866030/diff/1/src/runtime-profiler.cc...
src/runtime-profiler.cc:298: } else if (ticks >= 100) {
On 2012/03/27 11:40:58, ulan wrote:
> I think 100 should be a named constant with a static assert that the constant
<
> 256.

Done.

Expand Messages | Collapse Messages