CPUProfiler: propagate scriptId to the front-end

CPUProfiler: propagate scriptId to the front-end

Each CpuProfileNode has resource_name string property.
It cost us N * strlen(resource_name) where N is number of functions in the collected profile.
We could transfer script_id instead of resource_name so it would reduce transfer
size and help us to solve the problem with evals and sourceURL.

BUG=none
TEST=test-cpu-profiler/CollectCpuProfile
R=jkummerow@chromium.org, yurys@chromium.org

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

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

[yurys, 2013-06-25 09:16:54]

https://codereview.chromium.org/17642009/diff/4001/include/v8-profiler.h
File include/v8-profiler.h (right):

https://codereview.chromium.org/17642009/diff/4001/include/v8-profiler.h#newc...
include/v8-profiler.h:79: int GetScriptId() const;
In v8.h Script::Id returns v8::Local<v8::Value> and internally it seems to be
stored as i::String:
https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/objects-inl....
I think we should be consistent with v8::Script::Id() type here and use
Local<Value> as the return type.

Or if it is always int internally we could deprecate v8::Local<v8::Value>
v8::Script::Id() and introduce int v8::Script::GetId() instead.

https://codereview.chromium.org/17642009/diff/4001/src/cpu-profiler.cc
File src/cpu-profiler.cc (right):

https://codereview.chromium.org/17642009/diff/4001/src/cpu-profiler.cc#newcode96
src/cpu-profiler.cc:96: }
In case when there are no |info| you may still be able to retrieve Script from
SharedFunctionInfo (here it is already passed as Address but the signature can
be changed to accept SharedFunctionInfo* instead).

https://codereview.chromium.org/17642009/diff/4001/src/profile-generator.cc
File src/profile-generator.cc (right):

https://codereview.chromium.org/17642009/diff/4001/src/profile-generator.cc#n...
src/profile-generator.cc:275: entry_->script_id());
I'd move this before the node id in the output.

https://codereview.chromium.org/17642009/diff/4001/test/cctest/test-cpu-profi...
File test/cctest/test-cpu-profiler.cc (right):

https://codereview.chromium.org/17642009/diff/4001/test/cctest/test-cpu-profi...
test/cctest/test-cpu-profiler.cc:473: if (function_name.length() &&
(*function_name)[0] != '(')
Missing {}

https://codereview.chromium.org/17642009/diff/4001/test/cctest/test-cpu-profi...
test/cctest/test-cpu-profiler.cc:580:
CheckScriptIds(script->Id()->ToInt32()->Value(), root);
Would be nice to have additional test for a profile where nodes have different
script ids.

[loislo, 2013-07-01 12:22:05]

On 2013/06/25 09:16:54, Yury Semikhatsky wrote:
> https://codereview.chromium.org/17642009/diff/4001/include/v8-profiler.h
> File include/v8-profiler.h (right):
> 
>
https://codereview.chromium.org/17642009/diff/4001/include/v8-profiler.h#newc...
> include/v8-profiler.h:79: int GetScriptId() const;
> In v8.h Script::Id returns v8::Local<v8::Value> and internally it seems to be
> stored as i::String:
>
https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/objects-inl....
> I think we should be consistent with v8::Script::Id() type here and use
> Local<Value> as the return type.
> 
> Or if it is always int internally we could deprecate v8::Local<v8::Value>
> v8::Script::Id() and introduce int v8::Script::GetId() instead.
> 
> https://codereview.chromium.org/17642009/diff/4001/src/cpu-profiler.cc
> File src/cpu-profiler.cc (right):
> 
>
https://codereview.chromium.org/17642009/diff/4001/src/cpu-profiler.cc#newcode96
> src/cpu-profiler.cc:96: }
> In case when there are no |info| you may still be able to retrieve Script from
> SharedFunctionInfo (here it is already passed as Address but the signature can
> be changed to accept SharedFunctionInfo* instead).
> 
> https://codereview.chromium.org/17642009/diff/4001/src/profile-generator.cc
> File src/profile-generator.cc (right):
> 
>
https://codereview.chromium.org/17642009/diff/4001/src/profile-generator.cc#n...
> src/profile-generator.cc:275: entry_->script_id());
> I'd move this before the node id in the output.
> 
>
https://codereview.chromium.org/17642009/diff/4001/test/cctest/test-cpu-profi...
> File test/cctest/test-cpu-profiler.cc (right):
> 
>
https://codereview.chromium.org/17642009/diff/4001/test/cctest/test-cpu-profi...
> test/cctest/test-cpu-profiler.cc:473: if (function_name.length() &&
> (*function_name)[0] != '(')
> Missing {}
> 
>
https://codereview.chromium.org/17642009/diff/4001/test/cctest/test-cpu-profi...
> test/cctest/test-cpu-profiler.cc:580:
> CheckScriptIds(script->Id()->ToInt32()->Value(), root);
> Would be nice to have additional test for a profile where nodes have different
> script ids.

completely reworked

[Jakob Kummerow, 2013-07-01 12:35:22]

Superficially LGTM; I'll let Yury decide if this is the right approach.

[yurys, 2013-07-01 14:50:00]

lgtm

https://codereview.chromium.org/17642009/diff/11001/test/cctest/test-profile-...
File test/cctest/test-profile-generator.cc (right):

https://codereview.chromium.org/17642009/diff/11001/test/cctest/test-profile-...
test/cctest/test-profile-generator.cc:912: // don't appear in the stack trace.
I think we should be testing with inlining on as well to make sure script id is
correct after inlining.

https://codereview.chromium.org/17642009/diff/11001/test/cctest/test-profile-...
test/cctest/test-profile-generator.cc:920: context->Enter();
Please use LocalContext env(config); instead, it would save some typing and
would be more in line with other profiler tests.

https://codereview.chromium.org/17642009/diff/11001/test/cctest/test-profile-...
test/cctest/test-profile-generator.cc:933: CpuProfile* profile =
profiler->GetProfile(NULL, 0);
Why don't we use public API here?

https://codereview.chromium.org/17642009/diff/11001/test/cctest/test-profile-...
test/cctest/test-profile-generator.cc:945: current = PickChild(current,
"(anonymous function)");
Please use i::ProfilerGenerator::kAnonymousFunction instead/

[loislo, 2013-07-01 15:16:09]

Committed patchset #5 manually as r15429 (presubmit successful).

[loislo, 2013-07-02 06:14:11]

Committed patchset #6 manually as r15433 (presubmit successful).