[webkit-reviews] review granted: [Bug 119657] JSC: Change StackIterator to not require writes to the JS stack : [Attachment 209946] patch 3: applied Geoff's refinement suggestions.

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has granted Mark Lam <mark.lam at apple.com>'s
request for review:
Bug 119657: JSC: Change StackIterator to not require writes to the JS stack
https://bugs.webkit.org/show_bug.cgi?id=119657

Attachment 209946: patch 3: applied Geoff's refinement suggestions.
https://bugs.webkit.org/attachment.cgi?id=209946&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=209946&action=review


r=me with two changes below.

> Source/JavaScriptCore/interpreter/StackIterator.cpp:98
> +const StackIterator& StackIterator::end()
> +{
> +    DEFINE_STATIC_LOCAL(StackIterator, endIter, (0, 0));
> +    return endIter;

There are lots of problems with using a static for an iterator end value.

(1) Fragments the heap.

(2) Not thread-safe.

(3) Prevents the compiler from optimizing, because it turns a constant into a
load from the heap.

Please turn this into a normal return of a value before landing.

> Source/JavaScriptCore/interpreter/StackIterator.h:147
> +    if (this == &other)
> +	   return true;

This extra branch is a pessimization, since it's false until the very last
iteration. Please remove it before landing.