Notify CPU profiler when calling native getters

Notify CPU profiler when calling native getters

This change modifies code produced by BaseLoadStubCompiler::GenerateLoadCallback so that instead of calling AccessorGetter direcly it calls InvokeAccessorGetter which changes VM state and calls the actual callback. This way CPU profiler knows which external callback is being executed in this case.

BUG=244580
R=dcarney@chromium.org, loislo@chromium.org, svenpanne@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=15116

[yurys, 2013-06-05 15:51:28]

As discussed with Took off the bug it could make sense to change
InvokeAccessorGetter/InvokeAccessorGetterCallback signatures in ic.cc to use
internal instead of public types but it would just add unnecessary conversions
and more importantly there are no internal counterparts for v8::AccessorInfo and
v8::AccessorGetter. Since it would be a mix of public and private types I'd
rather leave it as is. Here is how it would look like with internal types
https://codereview.chromium.org/16171016/diff/2001/src/ic.cc I'll update this
change if you prefer those signatures.

Another thing that we discussed was sharing common code between
JSObject::GetPropertyWithCallback that works in slow case and the new functions.
PropertyCallbackArguments::Call used to invoke external callback is generated by
a macro and I cannot change it just for AccessorGetter property as the same
macro is used for other API callbacks accepting 2 parameters. I'd rather change
PropertyCallbackArguments::Call so that it take care of creating
VMState<EXTERNAL> and ExternalCallbackScope but in a separate CL.

[loislo, 2013-06-06 06:20:12]

lgtm

https://codereview.chromium.org/16286016/diff/8015/src/arm/stub-cache-arm.cc
File src/arm/stub-cache-arm.cc (right):

https://codereview.chromium.org/16286016/diff/8015/src/arm/stub-cache-arm.cc#...
src/arm/stub-cache-arm.cc:1459: __ mov(r2,
Operand(reinterpret_cast<int32_t>(getter_address)));
nit: As a newbie for the code I'd like to see a symbolic names for the arguments
implemented the same way as for mips.

[yurys, 2013-06-06 06:35:59]

https://codereview.chromium.org/16286016/diff/8015/src/arm/stub-cache-arm.cc
File src/arm/stub-cache-arm.cc (right):

https://codereview.chromium.org/16286016/diff/8015/src/arm/stub-cache-arm.cc#...
src/arm/stub-cache-arm.cc:1459: __ mov(r2,
Operand(reinterpret_cast<int32_t>(getter_address)));
On 2013/06/06 06:20:12, loislo wrote:
> nit: As a newbie for the code I'd like to see a symbolic names for the
arguments
> implemented the same way as for mips.

This a standard calling convention on ARM: arguments are passed in r0-r3

[loislo, 2013-06-06 06:58:49]

On 2013/06/06 06:35:59, Yury Semikhatsky wrote:
> https://codereview.chromium.org/16286016/diff/8015/src/arm/stub-cache-arm.cc
> File src/arm/stub-cache-arm.cc (right):
> 
>
https://codereview.chromium.org/16286016/diff/8015/src/arm/stub-cache-arm.cc#...
> src/arm/stub-cache-arm.cc:1459: __ mov(r2,
> Operand(reinterpret_cast<int32_t>(getter_address)));
> On 2013/06/06 06:20:12, loislo wrote:
> > nit: As a newbie for the code I'd like to see a symbolic names for the
> arguments
> > implemented the same way as for mips.
> 
> This a standard calling convention on ARM: arguments are passed in r0-r3

OK. It makes sense.

[dcarney, 2013-06-06 07:20:00]

lgtm

I think you want to run the blink dromeao and bindings perf tests before doing
this though. I'm assuming they'll all tank, which would get v8 rolled back.

[danno, 2013-06-06 09:20:12]

Although I am not an API expert, the use of public instead of internal types
bugs me for some reason. I'm adding Sven to take a look, he's got more
experience with this type of stuff in the API and I'd like him to give his l g t
m before you land this.

[dcarney, 2013-06-06 09:24:33]

On 2013/06/06 09:20:12, danno wrote:
> Although I am not an API expert, the use of public instead of internal types
> bugs me for some reason. I'm adding Sven to take a look, he's got more
> experience with this type of stuff in the API and I'd like him to give his l g
t
> m before you land this.

PropertyCallbackInfo and AccessorInfo have no internal types

[danno, 2013-06-06 09:30:57]

Then is this not a more fundamental problem with how PropertyCallbackInfo and
AccessorInfo are declared. Wouldn't adding internal versions of them be more
consistent with the way other types are declared?

[dcarney, 2013-06-06 09:33:13]

On 2013/06/06 09:30:57, danno wrote:
> Then is this not a more fundamental problem with how PropertyCallbackInfo and
> AccessorInfo are declared. Wouldn't adding internal versions of them be more
> consistent with the way other types are declared?

They have no represention as a heap object, which makes them fundamentally
different. I assume that's why there's no internal version.

[yurys, 2013-06-06 10:08:39]

On Thu, Jun 6, 2013 at 1:33 PM, Sven Panne <svenpanne@chromium.org> wrote:
> On Thu, Jun 6, 2013 at 11:30 AM, <danno@chromium.org> wrote:
>>
>> Then is this not a more fundamental problem with how PropertyCallbackInfo
>> and
>> AccessorInfo are declared. Wouldn't adding internal versions of them be
>> more
>> consistent with the way other types are declared?
>>
>> https://codereview.chromium.org/16286016/
>
>
> This was my thought, too. Furthermore, I don't really like
> ExternalReference::ICUtilityType, this is an even stronger hint that we are
> doing something wrong IMHO.

I could change the code so that InvokeAccessorGetter{Callback} are declared
using DECLARE_RUNTIME_FUNCTION macro and have same signature as the rest
external calls from generated inline caches. This would require changing
arch-specific call sites and the arguments for the external callback would be
prepared inside InvokeAccessorGetter{Callback} instead of the generated code (we
would basically need to prepare PropertyCallbackArguments like it's done for
slow case in objetcs.cc). My main concern here is performance but that would
make the new code more aligned with current native calls from generated ICs and
would eliminate need for ExternalReference::ICUtilityType. This would not
resolve the issue with using external types though as we would still need to
pass and call v8::AccessorGetter which doesn't have corresponding internal type.

[yurys, 2013-06-06 10:31:39]

On 2013/06/06 09:30:57, danno wrote:
> Then is this not a more fundamental problem with how PropertyCallbackInfo and
> AccessorInfo are declared. Wouldn't adding internal versions of them be more
> consistent with the way other types are declared?

I could add v8::internal::AccessorInfo, v8::internal::PropertyCallbackInfo,
v8::internal::AccessorGetter and  v8::internal::AccessorGetterCallback and have
corresponding public classes call into the internal ones doing all the
conversion but it would cost us one more function call for each accessor
compared to what we have at the moment(because all non-template methods on
PropertyCallbackInfo would have to be moved into api.cc to be able to work with
internal types), does that performance penalty sounds ok? If so I can update the
CL.

[yurys, 2013-06-06 11:12:06]

Dromaeo test results:
http://dromaeo.com/?id=196536,196546 - diff
196536 - run without the patch
196546 - run with the patch applied

[danno, 2013-06-06 13:39:43]

I assume (hope?) the extra overhead is only in the case when the profiler is
active? In any case, I wouldn't avoid "the right thing to do" until there is
data that says it's to slow. I think having the internal types and moving the
machinery out of ic.cc to somewhere more appropriate (like api.cc/api.h?) is the
right way to go.

[yurys, 2013-06-06 13:52:39]

On 2013/06/06 13:39:43, danno wrote:
> I assume (hope?) the extra overhead is only in the case when the profiler is
> active?
The current design is that we should always update VM state when external
callback is called this is because CPU profiler may be started and stopped when
JS stack is non-empty. So it may occur that there is some native callback on the
stack but the profiler never knows about it.

After discussion with Toon my intent was to try simple approach when we always
call the interceptor no matter if profiler is active or not and add additional
checks if that turns out slow.


> In any case, I wouldn't avoid "the right thing to do" until there is
> data that says it's to slow.
Now it looks like there is about 1% regression on Dromaeo DOM tests, I'll do
several more runs of the tests to make sure the results are stable and post them
here.


> I think having the internal types and moving the
> machinery out of ic.cc to somewhere more appropriate (like api.cc/api.h?) is
the
> right way to go.
Actually I've already updated the patch and moved the new stuff to api.h/cc,
also reverted controversial changes to src/assembler.* No

[yurys, 2013-06-06 14:06:08]

Currently there is 2-3% regression on dom_attr Dromaeo test which is most
sensitive to the change:

*RESULT dom_attr: dom_attr= 696.869735456785 runs/s
*RESULT dom_attr: dom_attr= 698.5931965016348 runs/s
*RESULT dom_attr: dom_attr= 711.261470027448 runs/s
*RESULT dom_attr: dom_attr= 693.924357657484 runs/s
*RESULT dom_attr: dom_attr= 710.3259193760543 runs/s
*RESULT dom_attr: dom_attr= 698.6130043599063 runs/s
// --- with the patch: -------------
*RESULT dom_attr: dom_attr= 723.3235020216619 runs/s
*RESULT dom_attr: dom_attr= 724.0536841658352 runs/s
*RESULT dom_attr: dom_attr= 727.5528400722708 runs/s
*RESULT dom_attr: dom_attr= 715.1730294960006 runs/s
*RESULT dom_attr: dom_attr= 716.8339577417119 runs/s
*RESULT dom_attr: dom_attr= 725.9170882159016 runs/s

[danno, 2013-06-06 14:35:59]

I agree with Sven here. We've been working very hard at making the JS/C++
transitions really fast, I think setting the VM state in these cases is not a
good idea.

It was exactly due to performance concerns that we don't set VM state when
CallApiFunctionAndReturn is used elsewhere. Adding this overhead for all native
getters and setters just to support the very rare case when the sampling
profiler is active is the wrong tradeoff.

[yurys, 2013-06-06 14:44:02]

On Thu, Jun 6, 2013 at 6:16 PM, Sven Panne <svenpanne@chromium.org> wrote:
> I don't have a good solution, but always paying a penalty for a very, very
> rare case (profiler is on) seems to be the wrong approach. We desperately
> try to make transitioning between JS and C as cheap as possible, so several
> writes plus an additional frame/jump/return per transition is heading into
> the wrong direction... :-/ It would be totally OK if this was only the case
> if the profiler is on, though.

On 2013/06/06 14:35:59, danno wrote:
> I agree with Sven here. We've been working very hard at making the JS/C++
> transitions really fast, I think setting the VM state in these cases is not a
> good idea.
> 
> It was exactly due to performance concerns that we don't set VM state when
> CallApiFunctionAndReturn is used elsewhere. Adding this overhead for all
native
> getters and setters just to support the very rare case when the sampling
> profiler is active is the wrong tradeoff.

I understand that 3% is too much for such a rare case. Let me see what it would
cost to
1) Check if profiler is active in the generated code and do either direct call
or call into the runtime thunks based on that.
2) Always update VM state from the generated code my gut feeling is that it
could also be quite cheap enough or do you think based on your experience that
this second option would be too expensive?

[yurys, 2013-06-07 09:23:28]

On Thu, Jun 6, 2013 at 6:50 PM, Yang Guo <yangguo@chromium.org> wrote:
> FYI we already set the current VM state in CallApiFunctionAndReturn guarded
> by FLAG_log_timer_events. This is so that the timeline plot accounts for the
> compiled callbacks.
>
> Cheers,
>
> Yang
>
Yeah, I've seen that code but we want to be able to start profiling at any point
without requiring user to (re)start browser with special flags.

[yurys, 2013-06-07 13:29:32]

So I disabled CPU throttling as Dan suggested and ran full Dromaeo DOM Core test
suite using src/tools/perf/run_measurement three times with each of the
following builds:
1) With patch set #9: generated assembly code checks if profiler is enabled and
calls the callback directly as before if not.
2) With patch set #8: always call intermediate function that changes VM state.
3) Without any changes.

On my z620 Linux desktop I've got the following results:

// ----------- with patch set #9(check in generated code) -------------------
*RESULT dom: dom= 2288.3080499870944 runs/s
*RESULT dom: dom= 2192.0581529196734 runs/s
*RESULT dom: dom= 2298.401560011703 runs/s
// ----------- with patch set #8(always change state) -------------------
*RESULT dom: dom= 2267.9035800297943 runs/s
*RESULT dom: dom= 2267.279220577068 runs/s
*RESULT dom: dom= 2208.5243745011767 runs/s
// ----------- without patch -------------------
*RESULT dom: dom= 2238.9526837659027 runs/s
*RESULT dom: dom= 2293.108281884979 runs/s
*RESULT dom: dom= 2276.855695120652 runs/s

These results do not reveal any regression that would be noticeable with the
given level of noise. Based on that I propose to land patchset #8 and watch
golem results.

[yurys, 2013-06-10 22:00:09]

The approach when we check if profiler is active in the generated code should
have minimal perf impact of all options considered here. In offline discussion
with danno@ we decided to give it a try and be prepared to roll back in case of
noticeable regression.

I updated the CL so that CpuProfiler::is_profiling_ flag is checked before
calling callback wrapper. Also as wrapper functions have one additional
parameter compared to the signature of actual accessors I added separate
external reference types for them.

[dcarney, 2013-06-11 07:20:41]

On 2013/06/10 22:00:09, Yury Semikhatsky wrote:
> The approach when we check if profiler is active in the generated code should
> have minimal perf impact of all options considered here. In offline discussion
> with danno@ we decided to give it a try and be prepared to roll back in case
of
> noticeable regression.
> 
> I updated the CL so that CpuProfiler::is_profiling_ flag is checked before
> calling callback wrapper. Also as wrapper functions have one additional
> parameter compared to the signature of actual accessors I added separate
> external reference types for them.

You end up duplicating the entire CallApiFunctionAndReturn assembler, although
almost all the arguments are the same.  Maybe you could pass the function
address to that function in a register and do a conditional move into that
register to activate the profiling code?  A lot of callback ICs are generated
and it would be unfortunate to waste so much memory on them.

[dcarney, 2013-06-11 07:33:01]

On 2013/06/11 07:20:41, dcarney wrote:
> On 2013/06/10 22:00:09, Yury Semikhatsky wrote:
> > The approach when we check if profiler is active in the generated code
should
> > have minimal perf impact of all options considered here. In offline
discussion
> > with danno@ we decided to give it a try and be prepared to roll back in case
> of
> > noticeable regression.
> > 
> > I updated the CL so that CpuProfiler::is_profiling_ flag is checked before
> > calling callback wrapper. Also as wrapper functions have one additional
> > parameter compared to the signature of actual accessors I added separate
> > external reference types for them.
> 
> You end up duplicating the entire CallApiFunctionAndReturn assembler, although
> almost all the arguments are the same.  Maybe you could pass the function
> address to that function in a register and do a conditional move into that
> register to activate the profiling code?  A lot of callback ICs are generated
> and it would be unfortunate to waste so much memory on them.

[dcarney, 2013-06-11 07:36:20]

I looked over it more, and it seems like you could move the entirety of your new
code into CallApiFunctionAndReturn.  I don't understand why you're not profiling
the other callsite anyway.  It would only be a couple of additional instructions
there, which could be deferred to the end of CallApiFunctionAndReturn

[yurys, 2013-06-11 08:17:23]

On 2013/06/11 07:36:20, dcarney wrote:
> I looked over it more, and it seems like you could move the entirety of your
new
> code into CallApiFunctionAndReturn.  I don't understand why you're not
profiling
> the other callsite anyway.  It would only be a couple of additional
instructions
> there, which could be deferred to the end of CallApiFunctionAndReturn

I was going to do this later as part of supporting profiling of API function
calls. At the moment arguments for the call are prepared by callers of
CallApiFunctionAndReturn. If we want in some cases to pass one additional
parameter in CallApiFunctionAndReturn that means the caller should be aware of
that and adjust PrepareCallApiFunction call accordingly to allow passing one
more param, otherwise CallApiFunctionAndReturn could have to rearrange stack
values to add one more param. This can probably be solved by making
PrepareCallApiFunction provisionally allocate one more slot to fit the optional
parameter leaving its call sites unchanged, is that what you suggest?

[yurys, 2013-06-12 12:53:06]

On 2013/06/11 07:20:41, dcarney wrote:
> On 2013/06/10 22:00:09, Yury Semikhatsky wrote:
> > The approach when we check if profiler is active in the generated code
should
> > have minimal perf impact of all options considered here. In offline
discussion
> > with danno@ we decided to give it a try and be prepared to roll back in case
> of
> > noticeable regression.
> > 
> > I updated the CL so that CpuProfiler::is_profiling_ flag is checked before
> > calling callback wrapper. Also as wrapper functions have one additional
> > parameter compared to the signature of actual accessors I added separate
> > external reference types for them.
> 
> You end up duplicating the entire CallApiFunctionAndReturn assembler, although
> almost all the arguments are the same.  Maybe you could pass the function
> address to that function in a register and do a conditional move into that
> register to activate the profiling code?  A lot of callback ICs are generated
> and it would be unfortunate to waste so much memory on them.

Done. Moved is_profiling check to CallApiFunctionAndReturn.

[dcarney, 2013-06-13 06:38:13]

lgtm

you need to trigger all the cases in tests. 
specifically, two tests, test-api/FunctionTemplate and test-api/SimpleCallback
should have an outer loop that runs the contents once with the profiling flag
enabled and once without
the function CheckReturnValue should be modified to check that the vmstate is
correct when the profiling flag is on

[yurys, 2013-06-13 08:57:19]

On 2013/06/13 06:38:13, dcarney wrote:
> lgtm
> 
> you need to trigger all the cases in tests. 
> specifically, two tests, test-api/FunctionTemplate and test-api/SimpleCallback
> should have an outer loop that runs the contents once with the profiling flag
> enabled and once without
> the function CheckReturnValue should be modified to check that the vmstate is
> correct when the profiling flag is on

Done. Modified CheckReturnValue to check that VM state is set to EXTERNAL and
that isolate->external_callback() points to expected function when CPU profiler
is active.

[dcarney, 2013-06-13 09:10:11]

https://chromiumcodereview.appspot.com/16286016/diff/68001/test/cctest/test-a...
File test/cctest/test-api.cc (right):

https://chromiumcodereview.appspot.com/16286016/diff/68001/test/cctest/test-a...
test/cctest/test-api.cc:999: for (int i = 0; i < 2; i++) {
could you put these loops directly in  THREADED_TEST(FunctionTemplate)?
would reduce a lot of boilerplate and make the contents of these function more
readable

https://chromiumcodereview.appspot.com/16286016/diff/68001/test/cctest/test-a...
test/cctest/test-api.cc:1070: for (int i = 0; i < 2; i++) {
same here, might be more readable

[Sven Panne, 2013-06-13 09:14:00]

LGTM with 2 style nits when Dan's feedback is addressed.

https://chromiumcodereview.appspot.com/16286016/diff/68001/test/cctest/test-a...
File test/cctest/test-api.cc (right):

https://chromiumcodereview.appspot.com/16286016/diff/68001/test/cctest/test-a...
test/cctest/test-api.cc:1000: bool is_profiling = i;
Nit: Use 'i > 0', implicit conversions from int to bool are bad style and more
often than not an error.

https://chromiumcodereview.appspot.com/16286016/diff/68001/test/cctest/test-a...
test/cctest/test-api.cc:1071: bool is_profiling = i;
Same here.

[yurys, 2013-06-13 09:16:20]

https://chromiumcodereview.appspot.com/16286016/diff/68001/test/cctest/test-a...
File test/cctest/test-api.cc (right):

https://chromiumcodereview.appspot.com/16286016/diff/68001/test/cctest/test-a...
test/cctest/test-api.cc:999: for (int i = 0; i < 2; i++) {
On 2013/06/13 09:10:11, dcarney wrote:
> could you put these loops directly in  THREADED_TEST(FunctionTemplate)?
> would reduce a lot of boilerplate and make the contents of these function more
> readable

Moving profilier-releated stuff to THREADED_TEST(FunctionTemplate) would require
creating some temporary context to make sure isolate is initialized and
CpuProfiler is created and also to have a context for the profile name string.

[yurys, 2013-06-13 09:19:14]

https://chromiumcodereview.appspot.com/16286016/diff/68001/test/cctest/test-a...
File test/cctest/test-api.cc (right):

https://chromiumcodereview.appspot.com/16286016/diff/68001/test/cctest/test-a...
test/cctest/test-api.cc:1000: bool is_profiling = i;
On 2013/06/13 09:14:00, Sven Panne wrote:
> Nit: Use 'i > 0', implicit conversions from int to bool are bad style and more
> often than not an error.

Done.

https://chromiumcodereview.appspot.com/16286016/diff/68001/test/cctest/test-a...
test/cctest/test-api.cc:1070: for (int i = 0; i < 2; i++) {
On 2013/06/13 09:10:11, dcarney wrote:
> same here, might be more readable

Done.

https://chromiumcodereview.appspot.com/16286016/diff/68001/test/cctest/test-a...
test/cctest/test-api.cc:1071: bool is_profiling = i;
On 2013/06/13 09:14:00, Sven Panne wrote:
> Same here.

Done.

[yurys, 2013-06-13 13:46:55]

Committed patchset #18 manually as r15116 (presubmit successful).