Bug 144011 – Give the heap object iterators the ability to return early.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

JSDollarVMPrototype::isValidCell() uses a heap object iterator to validate candidate cell pointers, and, when in use, is called a lot more often than the normal way those iterators are used.  As a result, I see my instrumented VM killed with a SIGXCPU (CPU time limit exceeded).  This patch gives the callback functor the ability to tell the iterators to return early when the functor no longer needs to continue iterating.  With this, my instrumented VM is useful again for debugging.

Since heap iteration is not something that we do in a typical fast path, I don’t expect this to have any noticeable impact on performance.

[Mark Lam]

Created attachment 251261 [details]
the patch.

[Geoffrey Garen]

Comment on attachment 251261 [details]
the patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=251261&action=review

> Source/JavaScriptCore/heap/Heap.cpp:1453
> +        return false; // Not done iterating.

If an API requires a comment at every usage site, that's a good sign that the API is not readable by design.

[Mark Lam]

(In reply to comment #2)
> Comment on attachment 251261 [details]
> the patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=251261&action=review
> 
> > Source/JavaScriptCore/heap/Heap.cpp:1453
> > +        return false; // Not done iterating.
> 
> If an API requires a comment at every usage site, that's a good sign that
> the API is not readable by design.

I'm open to suggestions.

[Michael Saboff]

Comment on attachment 251261 [details]
the patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=251261&action=review

>>> Source/JavaScriptCore/heap/Heap.cpp:1453
>>> +        return false; // Not done iterating.
>> 
>> If an API requires a comment at every usage site, that's a good sign that the API is not readable by design.
> 
> I'm open to suggestions.

Make it an enum.

[Mark Lam]

(In reply to comment #4)
> Comment on attachment 251261 [details]
> the patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=251261&action=review
> 
> >>> Source/JavaScriptCore/heap/Heap.cpp:1453
> >>> +        return false; // Not done iterating.
> >> 
> >> If an API requires a comment at every usage site, that's a good sign that the API is not readable by design.
> > 
> > I'm open to suggestions.
> 
> Make it an enum.

Sounds like a good plan.  I will execute.

[Mark Lam]

Created attachment 251276 [details]
patch 2: no more comments.

[Mark Lam]

Comment on attachment 251276 [details]
patch 2: no more comments.

View in context: https://bugs.webkit.org/attachment.cgi?id=251276&action=review

> Source/JavaScriptCore/ChangeLog:17
> +        I donât expect this to have any noticeable impact on performance.

Will fix the apostrophe in “don’t” before I land.

[Michael Saboff]

Comment on attachment 251276 [details]
patch 2: no more comments.

r=me

[Mark Lam]

Thanks for the review.  Landed in r183124: <http://trac.webkit.org/r183124>.