Dramatically speed up detailed heap snapshot generation.

Dramatically speed up detailed heap snapshot generation.

I've replaced calls to HashMap::Clear with instance re-creation.
This has sped up taking a heap snapshot of GMail from 33s -> 3s!

R=vitalyr@chromium.org
BUG=none
TEST=none

Committed: http://code.google.com/p/v8/source/detail?r=7259

[mnaganov (inactive), 2011-03-16 19:37:01]

On 2011/03/16 18:36:06, Mikhail Naganov (Chromium) wrote:

As we have discussed offline with Vitaly, stressing malloc with frequent
allocs/frees isn't an optimal way. Alternative approaches:

  - only re-allocate hashmap in case it had grown over some limit, otherwise,
clearing it with memset will be fast enough;

  - get rid of hash map at all, and instead mark visited references in heap
object directly, then clean up mark when extracting all remaining references; in
this case, the object itself replaces the hash map, avoiding memory allocations
at all.

[Vitaly Repeshko, 2011-03-16 19:37:28]

Nice! I do like the speedup, but I'm worried that this stresses the malloc
implementation. We discussed with Mikhail offline some other methods to achieve
the same if not better speedup.


-- Vitaly

[mnaganov (inactive), 2011-03-17 16:33:39]

On 2011/03/16 19:37:28, Vitaly Repeshko wrote:
> Nice! I do like the speedup, but I'm worried that this stresses the malloc
> implementation. We discussed with Mikhail offline some other methods to
achieve
> the same if not better speedup.
> 
> 
> -- Vitaly

So I've chosen the second approach. By the way I discovered that in fact I was
over-zealous in adding references to the map, as many of the references added
were not stored directly in objects, but in its dependents.

[Vitaly Repeshko, 2011-03-17 16:52:00]

http://codereview.chromium.org/6665038/diff/4001/src/objects.h
File src/objects.h (right):

http://codereview.chromium.org/6665038/diff/4001/src/objects.h#newcode1125
src/objects.h:1125: inline Address GetFieldAddress(int offset);
This is "obj->address() + offset". If a helper for this is really need, please
move it to the profiler code.

http://codereview.chromium.org/6665038/diff/4001/src/objects.h#newcode1593
src/objects.h:1593: inline Address GetInternalFieldOffset(int index);
Since it's called "...Offset" it should return an offset. Then a generic helper
can turn it into an address.

http://codereview.chromium.org/6665038/diff/4001/src/objects.h#newcode1707
src/objects.h:1707: inline Address GetFastPropertyAddress(int index);
I think this also should return an offset.

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

http://codereview.chromium.org/6665038/diff/4001/src/profile-generator.cc#new...
src/profile-generator.cc:1838: if (process_failure_tags_ && (*p)->IsFailure()) {
Please introduce a helper to test for the mark and a helper to clear it.

http://codereview.chromium.org/6665038/diff/4001/src/profile-generator.cc#new...
src/profile-generator.cc:1851: bool process_failure_tags_;
Rename to "process_marks_"?

http://codereview.chromium.org/6665038/diff/4001/src/profile-generator.cc#new...
src/profile-generator.cc:1902: SetInternalReference(obj, entry, 1, cs->first());
Won't this leave a failure object in the heap?

http://codereview.chromium.org/6665038/diff/4001/src/profile-generator.cc#new...
src/profile-generator.cc:2108: Address field) {
You can pass field offset instead of its address and do the "obj->address() +
offset" computation once in MarkAsFailure instead of doing it in every caller of
SetInternalReference.

http://codereview.chromium.org/6665038/diff/4001/src/profile-generator.h
File src/profile-generator.h (right):

http://codereview.chromium.org/6665038/diff/4001/src/profile-generator.h#newc...
src/profile-generator.h:1022: inline void MarkAsFailure(Address field);
Usually such functions hide the exact machinery they use to mark stuff. Call it
"MarkAsVisited"?

[Vitaly Repeshko, 2011-03-17 16:53:13]

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

http://codereview.chromium.org/6665038/diff/4001/src/profile-generator.cc#new...
src/profile-generator.cc:1902: SetInternalReference(obj, entry, 1, cs->first());
On 2011/03/17 16:52:00, Vitaly Repeshko wrote:
> Won't this leave a failure object in the heap?

Ah, I see. I won't because it doesn't take a field parameter.

[mnaganov (inactive), 2011-03-17 17:37:18]

http://codereview.chromium.org/6665038/diff/4001/src/objects.h
File src/objects.h (right):

http://codereview.chromium.org/6665038/diff/4001/src/objects.h#newcode1125
src/objects.h:1125: inline Address GetFieldAddress(int offset);
On 2011/03/17 16:52:00, Vitaly Repeshko wrote:
> This is "obj->address() + offset". If a helper for this is really need, please
> move it to the profiler code.

Removed.

http://codereview.chromium.org/6665038/diff/4001/src/objects.h#newcode1593
src/objects.h:1593: inline Address GetInternalFieldOffset(int index);
On 2011/03/17 16:52:00, Vitaly Repeshko wrote:
> Since it's called "...Offset" it should return an offset. Then a generic
helper
> can turn it into an address.

Fixed.

http://codereview.chromium.org/6665038/diff/4001/src/objects.h#newcode1707
src/objects.h:1707: inline Address GetFastPropertyAddress(int index);
On 2011/03/17 16:52:00, Vitaly Repeshko wrote:
> I think this also should return an offset.

Done.

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

http://codereview.chromium.org/6665038/diff/4001/src/profile-generator.cc#new...
src/profile-generator.cc:1838: if (process_failure_tags_ && (*p)->IsFailure()) {
On 2011/03/17 16:52:00, Vitaly Repeshko wrote:
> Please introduce a helper to test for the mark and a helper to clear it.

Done.

http://codereview.chromium.org/6665038/diff/4001/src/profile-generator.cc#new...
src/profile-generator.cc:1851: bool process_failure_tags_;
On 2011/03/17 16:52:00, Vitaly Repeshko wrote:
> Rename to "process_marks_"?

Renamed to process_field_marks_

http://codereview.chromium.org/6665038/diff/4001/src/profile-generator.cc#new...
src/profile-generator.cc:2108: Address field) {
On 2011/03/17 16:52:00, Vitaly Repeshko wrote:
> You can pass field offset instead of its address and do the "obj->address() +
> offset" computation once in MarkAsFailure instead of doing it in every caller
of
> SetInternalReference.

Done.

http://codereview.chromium.org/6665038/diff/4001/src/profile-generator.h
File src/profile-generator.h (right):

http://codereview.chromium.org/6665038/diff/4001/src/profile-generator.h#newc...
src/profile-generator.h:1022: inline void MarkAsFailure(Address field);
On 2011/03/17 16:52:00, Vitaly Repeshko wrote:
> Usually such functions hide the exact machinery they use to mark stuff. Call
it
> "MarkAsVisited"?

I've moved it to IndexedReferencesExtractor and renamed to MarkVisitedField.

[Vitaly Repeshko, 2011-03-18 12:16:36]

http://codereview.chromium.org/6665038/diff/8002/src/objects-inl.h
File src/objects-inl.h (right):

http://codereview.chromium.org/6665038/diff/8002/src/objects-inl.h#newcode1345
src/objects-inl.h:1345: int JSObject::GetFastPropertyOffset(int index) {
This should be called "GetInObjectPropertyOffset". It should assert the index
does not exceed the number of inobject properties and its callers should make
sure not to trigger the assert.

http://codereview.chromium.org/6665038/diff/8002/src/profile-generator.cc
File src/profile-generator.cc (right):

http://codereview.chromium.org/6665038/diff/8002/src/profile-generator.cc#new...
src/profile-generator.cc:1854: reinterpret_cast<intptr_t>(*field) & unmark_tag);
Hmm, I think the correct way is:
(reinterpret_cast<intptr_t>(*field) & ~kFailureTagMask) | kHeapObjectTag.

[mnaganov (inactive), 2011-03-18 12:31:01]

http://codereview.chromium.org/6665038/diff/8002/src/objects-inl.h
File src/objects-inl.h (right):

http://codereview.chromium.org/6665038/diff/8002/src/objects-inl.h#newcode1345
src/objects-inl.h:1345: int JSObject::GetFastPropertyOffset(int index) {
On 2011/03/18 12:16:36, Vitaly Repeshko wrote:
> This should be called "GetInObjectPropertyOffset". It should assert the index
> does not exceed the number of inobject properties and its callers should make
> sure not to trigger the assert.

Done.

http://codereview.chromium.org/6665038/diff/8002/src/profile-generator.cc
File src/profile-generator.cc (right):

http://codereview.chromium.org/6665038/diff/8002/src/profile-generator.cc#new...
src/profile-generator.cc:1854: reinterpret_cast<intptr_t>(*field) & unmark_tag);
On 2011/03/18 12:16:36, Vitaly Repeshko wrote:
> Hmm, I think the correct way is:
> (reinterpret_cast<intptr_t>(*field) & ~kFailureTagMask) | kHeapObjectTag.

Done.

[Vitaly Repeshko, 2011-03-18 12:36:02]

LGTM

http://codereview.chromium.org/6665038/diff/11001/src/profile-generator.cc
File src/profile-generator.cc (right):

http://codereview.chromium.org/6665038/diff/11001/src/profile-generator.cc#ne...
src/profile-generator.cc:1952: if (index - js_obj->map()->inobject_properties()
< 0) {
nit: It's probably more natural to write "index <
js_obj->map()->inobject_properties()" instead.

[mnaganov (inactive), 2011-03-18 12:46:34]

http://codereview.chromium.org/6665038/diff/11001/src/profile-generator.cc
File src/profile-generator.cc (right):

http://codereview.chromium.org/6665038/diff/11001/src/profile-generator.cc#ne...
src/profile-generator.cc:1952: if (index - js_obj->map()->inobject_properties()
< 0) {
On 2011/03/18 12:36:03, Vitaly Repeshko wrote:
> nit: It's probably more natural to write "index <
> js_obj->map()->inobject_properties()" instead.

Sure. I also have noted this afterwards.