CpuProfiler: eliminate cpu-profiler dependency from heap-inl.h

CpuProfiler: eliminate cpu-profiler dependency from heap-inl.h

We accessed to cpu_profiler for tracking SharedFunctionInfo objects movements and used their addresses for generating function_id. Actually we could replace the manually generated shared_id by the pair script_id + position. In this case we can drop SharedFunctionInfo events support from cpu_profiler and remove the dependency.

BTW GetCallUid was used as an unique identifier of the function on the front-end side. Actually it is a hash which might not be unique. So I renamed GetCallUid with GetHash and implemented GetFunctionId method.

BUG=452067
LOG=n

Committed: https://crrev.com/8ba89cce6daf5be4427ba184ec5f2c25ec2a15a4
Cr-Commit-Position: refs/heads/master@{#26775}

[loislo, 2015-02-20 09:58:56]

loislo@chromium.org changed reviewers:
+ alph@chromium.org, svenpanne@chromium.org, yurys@chromium.org

[loislo, 2015-02-20 09:58:57]

PTAL

[yurys, 2015-02-20 10:17:16]

https://codereview.chromium.org/941973002/diff/20001/src/cpu-profiler.h
File src/cpu-profiler.h (right):

https://codereview.chromium.org/941973002/diff/20001/src/cpu-profiler.h#newco...
src/cpu-profiler.h:243: virtual void SharedFunctionInfoMoveEvent(Address from,
Address to) {}
Can we remove dependency of the logger on this method and remove it altogether?
I suspect the use this event for the same purpose as we do in CPU profiler.

https://codereview.chromium.org/941973002/diff/20001/src/profile-generator.cc
File src/profile-generator.cc (left):

https://codereview.chromium.org/941973002/diff/20001/src/profile-generator.cc...
src/profile-generator.cc:175: hash ^=
ComputeIntegerHash(static_cast<uint32_t>(shared_id_),
Given that we (wrongly) assume externally that GetCllUid returns ProfileNode's
unique id this change might increase probability of collision and lead to some
errors. Let's first rename this method to GetHash() and have GetCallUid return
unique id. After that it should be safe to change this method. Eventually we
should drop GetCallUid from external API in favor of script_id + position.

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

https://codereview.chromium.org/941973002/diff/20001/src/profile-generator.cc...
src/profile-generator.cc:221: if (shared->script()->IsScript()) {
 if (!shared->script()->IsScript()) return;

https://codereview.chromium.org/941973002/diff/20001/src/profile-generator.cc...
src/profile-generator.cc:222: DCHECK(Script::cast(shared->script()));
Remove this

[alph, 2015-02-20 10:26:35]

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

https://codereview.chromium.org/941973002/diff/20001/src/profile-generator.cc...
src/profile-generator.cc:225: set_position(shared->start_position());
Can we just add the uid to SharedFunctionInfo?

[loislo, 2015-02-20 10:36:04]

https://codereview.chromium.org/941973002/diff/20001/src/profile-generator.cc
File src/profile-generator.cc (left):

https://codereview.chromium.org/941973002/diff/20001/src/profile-generator.cc...
src/profile-generator.cc:175: hash ^=
ComputeIntegerHash(static_cast<uint32_t>(shared_id_),
On 2015/02/20 10:17:16, yurys wrote:
> Given that we (wrongly) assume externally that GetCllUid returns ProfileNode's
> unique id this change might increase probability of collision and lead to some
> errors. Let's first rename this method to GetHash() and have GetCallUid return
> unique id. After that it should be safe to change this method. Eventually we
> should drop GetCallUid from external API in favor of script_id + position.

It is not a ProfileNode unique ID but CodeEntry "UniqueID" so many ProfileNodes
could have the same CallUid. I.e. it is a function id.

[loislo, 2015-02-20 11:38:04]

https://codereview.chromium.org/941973002/diff/20001/src/cpu-profiler.h
File src/cpu-profiler.h (right):

https://codereview.chromium.org/941973002/diff/20001/src/cpu-profiler.h#newco...
src/cpu-profiler.h:243: virtual void SharedFunctionInfoMoveEvent(Address from,
Address to) {}
On 2015/02/20 10:17:16, yurys wrote:
> Can we remove dependency of the logger on this method and remove it
altogether?
> I suspect the use this event for the same purpose as we do in CPU profiler.

I'd do this in another patch

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

https://codereview.chromium.org/941973002/diff/20001/src/profile-generator.cc...
src/profile-generator.cc:221: if (shared->script()->IsScript()) {
On 2015/02/20 10:17:16, yurys wrote:
>  if (!shared->script()->IsScript()) return;

Done.

https://codereview.chromium.org/941973002/diff/20001/src/profile-generator.cc...
src/profile-generator.cc:222: DCHECK(Script::cast(shared->script()));
On 2015/02/20 10:17:16, yurys wrote:
> Remove this

Done.

https://codereview.chromium.org/941973002/diff/20001/src/profile-generator.cc...
src/profile-generator.cc:225: set_position(shared->start_position());
On 2015/02/20 10:26:35, alph wrote:
> Can we just add the uid to SharedFunctionInfo?

Done.

[loislo, 2015-02-20 11:50:22]

Patchset #3 (id:40001) has been deleted

[alph, 2015-02-20 12:03:42]

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

https://codereview.chromium.org/941973002/diff/60001/src/profile-generator.cc...
src/profile-generator.cc:198: (script_id_ != v8::UnboundScript::kNoScriptId ||
won't it just match all the functions with same script_id?
there should be a check against position as well.

[loislo, 2015-02-20 12:21:25]

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

https://codereview.chromium.org/941973002/diff/60001/src/profile-generator.cc...
src/profile-generator.cc:198: (script_id_ != v8::UnboundScript::kNoScriptId ||
On 2015/02/20 12:03:42, alph wrote:
> won't it just match all the functions with same script_id?
> there should be a check against position as well.

Done.

[loislo, 2015-02-20 12:22:31]

Patchset #4 (id:80001) has been deleted

[alph, 2015-02-20 12:25:31]

lgtm

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

https://codereview.chromium.org/941973002/diff/100001/src/profile-generator.c...
src/profile-generator.cc:198: if (script_id_ == entry->script_id_ && position_
== entry->position_) {
return script_id_ == entry->script_id_ && position_ == entry->position_;

https://codereview.chromium.org/941973002/diff/100001/src/profile-generator.c...
src/profile-generator.cc:203: if (name_prefix_ == entry->name_prefix_ && name_
== entry->name_ &&
ditto

[loislo, 2015-02-20 12:27:19]

New patchsets have been uploaded after l-g-t-m from alph@chromium.org

[loislo, 2015-02-20 12:28:53]

On 2015/02/20 12:25:31, alph wrote:
> lgtm
> 
> https://codereview.chromium.org/941973002/diff/100001/src/profile-generator.cc
> File src/profile-generator.cc (right):
> 
>
https://codereview.chromium.org/941973002/diff/100001/src/profile-generator.c...
> src/profile-generator.cc:198: if (script_id_ == entry->script_id_ && position_
> == entry->position_) {
> return script_id_ == entry->script_id_ && position_ == entry->position_;
> 
>
https://codereview.chromium.org/941973002/diff/100001/src/profile-generator.c...
> src/profile-generator.cc:203: if (name_prefix_ == entry->name_prefix_ && name_
> == entry->name_ &&
> ditto

done

[yurys, 2015-02-20 12:29:29]

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

https://codereview.chromium.org/941973002/diff/100001/src/profile-generator.c...
src/profile-generator.cc:351: function_ids_.Lookup(code_entry,
code_entry->GetHash(), true);
Would it make sense to store the map on ProfileGenerator next to CodeMap?
Otherwise we'll have it duplicated in each ProfileTree.

[yurys, 2015-02-20 12:34:25]

Also please update issue's description as its main purpose is to make CallUid
actually unique and eliminate SharedFunctionInfo motion events.

[loislo, 2015-02-20 13:02:16]

On 2015/02/20 12:29:29, yurys wrote:
> https://codereview.chromium.org/941973002/diff/100001/src/profile-generator.cc
> File src/profile-generator.cc (right):
> 
>
https://codereview.chromium.org/941973002/diff/100001/src/profile-generator.c...
> src/profile-generator.cc:351: function_ids_.Lookup(code_entry,
> code_entry->GetHash(), true);
> Would it make sense to store the map on ProfileGenerator next to CodeMap?
> Otherwise we'll have it duplicated in each ProfileTree.

We delete profile immediately after taking profile.
When we get a new CodeEntry we delete old entries from CodeMap if they overlap
by address range with the new one.
If we put function_ids map into profile generator then it will continuously
grow.
In this case we need to clean it manually so we will loose the persistency.

As a result we will get no benefits from this and additional code.

[loislo, 2015-02-20 13:02:31]

On 2015/02/20 12:34:25, yurys wrote:
> Also please update issue's description as its main purpose is to make CallUid
> actually unique and eliminate SharedFunctionInfo motion events.

done

[Sven Panne, 2015-02-20 13:18:02]

The CQ bit was checked by svenpanne@chromium.org

[Sven Panne, 2015-02-20 13:18:03]

lgtm

LGTM (rubber-stamped)

[commit-bot: I haz the power, 2015-02-20 13:19:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/941973002/120001

[commit-bot: I haz the power, 2015-02-20 13:28:48]

Committed patchset #5 (id:120001)

[commit-bot: I haz the power, 2015-02-20 13:28:49]

Failed to apply the patch.

[commit-bot: I haz the power, 2015-02-20 13:29:25]

Patchset 5 (id:??) landed as
https://crrev.com/8ba89cce6daf5be4427ba184ec5f2c25ec2a15a4
Cr-Commit-Position: refs/heads/master@{#26775}

[Michael Achenbach, 2015-02-20 13:54:16]

Side note: Better start tryjobs with "git cl try" as this selects the most
important trybots only which are also used by the CQ. Unless you need the others
for a reason...

[Sven Panne, 2015-02-23 07:15:58]

On 2015/02/20 13:54:16, Michael Achenbach wrote:
> Side note: Better start tryjobs with "git cl try" as this selects the most
> important trybots only which are also used by the CQ. Unless you need the
others
> for a reason...

IIRC, I checked the bots (as a reviewer, so I can't simply do a "git cl try"),
and I always do it that way: Check everything, because I don't know what's
relevant and I've got no clue whatsoever which bots "git cl try" triggers. Can
we have a meta-checkbox which triggers exactly the "git cl try" bots? If not,
I'll probably continue my way... :-)