Bug 120564 – Converting StackIterator to a callback interface

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

[Mark Lam]

It was proposed that using a callback interface for the StackIterator will reduce the complexity of code for decoding inlined frames from the various JIT tiers.  Secondly, it was also proposed that the increase in complexity for the clients of the StackIterator (as a result of adopting a callback interface) will not be too significant, and that this cost will only be incurred rarely because we seldom need to add new clients of the StackIterator.  This bug tracks the work for converting the StackIterator to a callback interface.

[Mark Lam]

Created attachment 210202 [details]
the patch.

This patch has run all javascriptcore and layout tests with no regressions.  Benchmark details to follow shortly.

[Mark Lam]

Created attachment 210203 [details]
jsc benchmark run 3

[Mark Lam]

Created attachment 210204 [details]
jsc benchmark run 4

[Mark Lam]

Created attachment 210205 [details]
jsc benchmark run 5

[Mark Lam]

Created attachment 210206 [details]
jsc benchmark run 6

[Mark Lam]

Created attachment 210207 [details]
Benchmark component results from the runs that are consistently different between the baseline and this patch.

[Mark Lam]

Created attachment 210208 [details]
Summary of performance differences between the baseline and this patch.

If you look at the total or overall benchmark scores, there is no significant differences.

However, if you look at the specific benchmark components, then there are 5 components that are consistently slower with the patch, and 2 that are consistently faster.  These (also shown the summary file) are:

Patch is slower than baseline:
                                run 3                        run 4                        run 5                        run 6
 regexp                       ! definitely 1.0069x slower  ! definitely 1.0180x slower  ! definitely 1.0196x slower  ! definitely 1.0054x slower
 HashMap-put-get-iterate      ! definitely 1.0424x slower  ! definitely 1.0300x slower  ! definitely 1.0492x slower  ! definitely 1.0568x slower
 Int16Array-to-Int32Array-set ! definitely 1.0615x slower  ! definitely 1.0492x slower  ! definitely 1.0414x slower  ! definitely 1.0586x slower
 string-long-ident-equality   ! definitely 1.0419x slower  ! definitely 1.0358x slower  ! definitely 1.0454x slower  ! definitely 1.0390x slower
 string-var-equality          ! definitely 1.0251x slower  ! definitely 1.0255x slower  ! definitely 1.0254x slower  ! definitely 1.0240x slower

Patch is faster than baseline:
                                run 3                        run 4                        run 5                        run 6
 json-parse-financial         ^ definitely 1.0151x faster  ^ definitely 1.0246x faster  ^ definitely 1.0153x faster  ^ definitely 1.0193x faster
 json-stringify-tinderbox     ^ definitely 1.0176x faster  ^ definitely 1.0397x faster  ^ definitely 1.0361x faster  ^ definitely 1.0469x faster

[Filip Pizlo]

(In reply to comment #7)
> Created an attachment (id=210208) [details]
> Summary of performance differences between the baseline and this patch.
> 
> If you look at the total or overall benchmark scores, there is no significant differences.
> 
> However, if you look at the specific benchmark components, then there are 5 components that are consistently slower with the patch, and 2 that are consistently faster.  These (also shown the summary file) are:
> 
> Patch is slower than baseline:
>                                 run 3                        run 4                        run 5                        run 6
>  regexp                       ! definitely 1.0069x slower  ! definitely 1.0180x slower  ! definitely 1.0196x slower  ! definitely 1.0054x slower
>  HashMap-put-get-iterate      ! definitely 1.0424x slower  ! definitely 1.0300x slower  ! definitely 1.0492x slower  ! definitely 1.0568x slower
>  Int16Array-to-Int32Array-set ! definitely 1.0615x slower  ! definitely 1.0492x slower  ! definitely 1.0414x slower  ! definitely 1.0586x slower
>  string-long-ident-equality   ! definitely 1.0419x slower  ! definitely 1.0358x slower  ! definitely 1.0454x slower  ! definitely 1.0390x slower
>  string-var-equality          ! definitely 1.0251x slower  ! definitely 1.0255x slower  ! definitely 1.0254x slower  ! definitely 1.0240x slower
> 
> Patch is faster than baseline:
>                                 run 3                        run 4                        run 5                        run 6
>  json-parse-financial         ^ definitely 1.0151x faster  ^ definitely 1.0246x faster  ^ definitely 1.0153x faster  ^ definitely 1.0193x faster
>  json-stringify-tinderbox     ^ definitely 1.0176x faster  ^ definitely 1.0397x faster  ^ definitely 1.0361x faster  ^ definitely 1.0469x faster

It's not a good idea to get excited about individual benchmark changes, especially when they're in the 1-5% range.  What is important are the overall numbers.  You should care the individual benchmark numbers only if you have a substantial change in the benchmark suite averages (i.e. the lines marked with a  '*').

If you have as many benchmarks as we do, then anytime you breathe on the system (for example change the layout of any code in JavaScriptCore or any other framework by making an innocuous change to some function) you would expect some benchmarks to have "significant" changes.  It's best to ignore that so long as the averages don't change significantly.

[Filip Pizlo]

Comment on attachment 210202 [details]
the patch.

Any particular reason you didn't use a functor based approach, like we use in a bunch of other places in the system?  See for example MarkedSpace::forEachLiveCell() or DFG::clobberize.

[Mark Lam]

Created attachment 210232 [details]
patch 2: functor version.

To answer Filip, there's no reason why we shouldn't use functors for the callback instead.  It's just an oversight of mine.  Patch 2 applies the functor pattern and makes StackIterator::iterate() a template function.

[Filip Pizlo]

Comment on attachment 210232 [details]
patch 2: functor version.

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

I'm worried about StackIterator::find() - does this method need to exist?

> Source/JavaScriptCore/interpreter/StackIterator.h:119
> +    class Functor {
> +    public:
> +        Status operator()(StackIterator&) { return Done; }
> +    };

I don't think you need to declare this.  And you don't need the various implementations to subclass this.

> Source/JavaScriptCore/runtime/JSFunction.cpp:209
> +    RetrieveArgumentsFunctor functor;
>      StackIterator iter = exec->find(functionObj);
> -    return iter != exec->end() ? JSValue(iter->arguments()) : jsNull();
> +    iter.iterate(functor);
> +    return functor.result();

This is confusing. Wouldn't it be better to have find() be part of the functor?

[Mark Lam]

Created attachment 210393 [details]
patch 3: removed StackIterator::find().



(In reply to comment #11)
> (From update of attachment 210232 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=210232&action=review
> 
> I'm worried about StackIterator::find() - does this method need to exist?

StackIterator::find() is now removed in patch 3.

> > Source/JavaScriptCore/interpreter/StackIterator.h:119
> > +    class Functor {
> > +    public:
> > +        Status operator()(StackIterator&) { return Done; }
> > +    };
> 
> I don't think you need to declare this.  And you don't need the various implementations to subclass this.

The only reason I wanted to provide this is because it makes it clear to the implementers of Functor subclasses as to what the prototype the functor is expected to have.  Sure, it can be inferred from the implementation of StackIterator::iterate() (or from other subclasses of StackIterator::Functor), but I think it's better to state it explicitly as the expected interface.  I change the code to just be a prototype without an implementation, and added a comment to indicate that the subclass should implement that method.

[Mark Lam]

Comment on attachment 210393 [details]
patch 3: removed StackIterator::find().

I just re-read Filip's comment.  Will remove StackIterator::Functor and replace it with a comment.

[Mark Lam]

Created attachment 210405 [details]
patch 4: Removed StackIterator::Functor.

[Filip Pizlo]

Comment on attachment 210405 [details]
patch 4: Removed StackIterator::Functor.

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

r=me if you come up with a better name for that one functor.

> Source/WebCore/bindings/js/ScriptCallStackFactory.cpp:102
> +class CreateScriptCallStackFunctor2 {

Better name?  It's OK to be descriptive.

[Mark Lam]

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

[Geoffrey Garen]

Comment on attachment 210405 [details]
patch 4: Removed StackIterator::Functor.

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

> Source/JavaScriptCore/interpreter/StackIterator.cpp:481
> +    StackIterator iter = topCallFrame->begin();
> +    iter.iterate(functor);

It's pretty strange to continue calling this an iterator, and allocating it as an object.

In C++, the idiomatic way to use "begin()" is to allocate an object that represents your state, increment it, and compare to "end()". In a callback-based design, there shouldn't be any iterator object with any state of its own.

> Source/JavaScriptCore/interpreter/StackIterator.h:125
> +            gotoNextFrameWithFilter();

It's also strange to have two kinds of filter: one built into the iterator, and then the functor. In cases where we want to filter frames, the functor should do the filtering.

> Source/WebCore/bindings/js/ScriptCallStackFactory.cpp:148
> +    size_t numberOfFrames = iter.numberOfFrames();

The functor should do this calculation.