Bug 150945 – Regression(r191815): 5.3% regression on Dromaeo JS Library Benchmark

• Component: New Bugs
• Status: RESOLVED FIXED

[Keith Miller]

Regression(r191815): 5.3% regression on Dromaeo JS Library Benchmark

[Keith Miller]

Created attachment 264960 [details]
Patch

[Keith Miller]

Created attachment 264962 [details]
Patch

[Keith Miller]

Created attachment 264963 [details]
Benchmark results

[Keith Miller]

                                  WithoutTag                MyChanges

json-stringify-tinderbox        23.494+-3.259             23.436+-0.662

<arithmetic>                    23.494+-3.259             23.436+-0.662           might be 1.0025x faster

[Keith Miller]

Regression in benchmarks appear to be noise.

                                  WithoutTag                MyChanges

json-stringify-tinderbox        23.494+-3.259             23.436+-0.662

<arithmetic>                    23.494+-3.259             23.436+-0.662           might be 1.0025x faster

[Geoffrey Garen]

Can you add a regress performance test?

[Keith Miller]

Whoops, I forgot to upload the results of my test against tip of tree:

                               TOT                        MC

symbol-tostringtag       29.1231+-2.2000     ^      2.9094+-0.1330        ^ definitely 10.0101x faster

<geometric>              29.1231+-2.2000     ^      2.9094+-0.1330        ^ definitely 10.0101x faster

[Build Bot]

Comment on attachment 264962 [details]
Patch

Attachment 264962 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/393535

New failing tests:
http/tests/security/cross-frame-access-history-get.html
http/tests/security/cross-frame-access-object-setPrototypeOf.html
http/tests/security/cross-frame-access-custom.html
http/tests/history/cross-origin-replace-history-object-child.html

[Build Bot]

Created attachment 264975 [details]
Archive of layout-test-results from ews101 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 264962 [details]
Patch

Attachment 264962 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/393536

New failing tests:
http/tests/security/cross-frame-access-object-setPrototypeOf.html
http/tests/security/cross-frame-access-custom.html
http/tests/history/cross-origin-replace-history-object-child.html

[Build Bot]

Created attachment 264976 [details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 264962 [details]
Patch

Attachment 264962 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/393513

New failing tests:
http/tests/security/cross-frame-access-history-get.html
http/tests/security/cross-frame-access-object-setPrototypeOf.html
http/tests/security/cross-frame-access-custom.html
http/tests/history/cross-origin-replace-history-object-child.html

[Build Bot]

Created attachment 264977 [details]
Archive of layout-test-results from ews112 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Filip Pizlo]

Comment on attachment 264962 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:13
> +        This patch fixes a performance regression introduced by r191815. Before adding Symbol.toStringTag
> +        we would cache the value of Object.prototype.toString() in the rareData of the structure.
> +        In order to cache the result of Object.prototype.toString() we now need to ensure that the
> +        value stored in Symbol.toStringTag is a known constant. Thus, in order to ensure the stored Symbol.toStringTag
> +        value remains constant adaptive watchpoints have been refactored to be generalizable and new versions of
> +        them that clear out the toString value cache when fired have been added.

Why did you generalize adaptive watchpoints?

I think that this patch takes something that is very simple - the adaptive watchpoint code in the DFG - and makes it a lot more complicated just so that it can reuse a tiny part of that functionality for something that is only peripherally related.

Did you consider if maybe the code change would be smaller, and the result easier to understand, if you just wrote a specialized watchpoint for the toStringTag case?  I believe that you aren't actually saving any code by doing this kind of generalization.  Instead, you're making the code more complicated because now I have to search through multiple files to figure out what the firing of one of those watchpoints does.

> Source/JavaScriptCore/bytecode/AdaptiveInferredPropertyValueWatchpointBase.h:36
> +template<typename T>

What is T?

> Source/JavaScriptCore/bytecode/AdaptiveInferredPropertyValueWatchpointBaseInlines.h:68
> +    T::handleFire(static_cast<T*>(this), detail);

This is really weird.  Why isn't this a virtual method?

> Source/JavaScriptCore/dfg/DFGAdaptiveInferredPropertyValueWatchpoint.h:35
> +class DFGAdaptiveInferredPropertyValueWatchpoint : public AdaptiveInferredPropertyValueWatchpointBase<DFGAdaptiveInferredPropertyValueWatchpoint> {

This is an odd choice of name.  "DFG" is the name of a namespace.  It's strange to have something with a leading "DFG" in the "DFG" namespace.

[Keith Miller]

Comment on attachment 264962 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:13
>> +        them that clear out the toString value cache when fired have been added.
> 
> Why did you generalize adaptive watchpoints?
> 
> I think that this patch takes something that is very simple - the adaptive watchpoint code in the DFG - and makes it a lot more complicated just so that it can reuse a tiny part of that functionality for something that is only peripherally related.
> 
> Did you consider if maybe the code change would be smaller, and the result easier to understand, if you just wrote a specialized watchpoint for the toStringTag case?  I believe that you aren't actually saving any code by doing this kind of generalization.  Instead, you're making the code more complicated because now I have to search through multiple files to figure out what the firing of one of those watchpoints does.

I generalized adaptive watchpoints because in order to make the toStringTag optimization we need to know that the value stored in the Symbol.toStringTag slot is a constant. If I were to implement a specialized watchpoint the code would be essentially identical to the adaptive watchpoint code. While, I agree that the code might be smaller and slightly less complex if wrote a specialized watchpoint the code maintenance overhead from having two roughly identical implementations is probably not worthwhile.

>> Source/JavaScriptCore/bytecode/AdaptiveInferredPropertyValueWatchpointBaseInlines.h:68
>> +    T::handleFire(static_cast<T*>(this), detail);
> 
> This is really weird.  Why isn't this a virtual method?

In hindsight, this should definitely be a virtual method. Fixed.

>> Source/JavaScriptCore/dfg/DFGAdaptiveInferredPropertyValueWatchpoint.h:35
>> +class DFGAdaptiveInferredPropertyValueWatchpoint : public AdaptiveInferredPropertyValueWatchpointBase<DFGAdaptiveInferredPropertyValueWatchpoint> {
> 
> This is an odd choice of name.  "DFG" is the name of a namespace.  It's strange to have something with a leading "DFG" in the "DFG" namespace.

I started the name with DFG because that was the name of the file. Perhaps I should start it with CodeJettisoning instead.

[Filip Pizlo]

(In reply to comment #15)
> Comment on attachment 264962 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=264962&action=review
> 
> >> Source/JavaScriptCore/ChangeLog:13
> >> +        them that clear out the toString value cache when fired have been added.
> > 
> > Why did you generalize adaptive watchpoints?
> > 
> > I think that this patch takes something that is very simple - the adaptive watchpoint code in the DFG - and makes it a lot more complicated just so that it can reuse a tiny part of that functionality for something that is only peripherally related.
> > 
> > Did you consider if maybe the code change would be smaller, and the result easier to understand, if you just wrote a specialized watchpoint for the toStringTag case?  I believe that you aren't actually saving any code by doing this kind of generalization.  Instead, you're making the code more complicated because now I have to search through multiple files to figure out what the firing of one of those watchpoints does.
> 
> I generalized adaptive watchpoints because in order to make the toStringTag
> optimization we need to know that the value stored in the Symbol.toStringTag
> slot is a constant. If I were to implement a specialized watchpoint the code
> would be essentially identical to the adaptive watchpoint code. While, I
> agree that the code might be smaller and slightly less complex if wrote a
> specialized watchpoint the code maintenance overhead from having two roughly
> identical implementations is probably not worthwhile.

There is a non-trivial cost to any code that uses templates in this manner.  We have to weigh that cost against the cost of slight code duplication.

Note that we already have multiple watchpoints that essentially duplicate this pattern.  It's a simple pattern to work with, so I don't think it's bad to duplicate it.

> 
> >> Source/JavaScriptCore/bytecode/AdaptiveInferredPropertyValueWatchpointBaseInlines.h:68
> >> +    T::handleFire(static_cast<T*>(this), detail);
> > 
> > This is really weird.  Why isn't this a virtual method?
> 
> In hindsight, this should definitely be a virtual method. Fixed.

Does that allow you to get rid of the template stuff?

> 
> >> Source/JavaScriptCore/dfg/DFGAdaptiveInferredPropertyValueWatchpoint.h:35
> >> +class DFGAdaptiveInferredPropertyValueWatchpoint : public AdaptiveInferredPropertyValueWatchpointBase<DFGAdaptiveInferredPropertyValueWatchpoint> {
> > 
> > This is an odd choice of name.  "DFG" is the name of a namespace.  It's strange to have something with a leading "DFG" in the "DFG" namespace.
> 
> I started the name with DFG because that was the name of the file. Perhaps I
> should start it with CodeJettisoning instead.

[Keith Miller]

(In reply to comment #16)
> (In reply to comment #15)
> > Comment on attachment 264962 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=264962&action=review
> > 
> > >> Source/JavaScriptCore/ChangeLog:13
> > >> +        them that clear out the toString value cache when fired have been added.
> > > 
> > > Why did you generalize adaptive watchpoints?
> > > 
> > > I think that this patch takes something that is very simple - the adaptive watchpoint code in the DFG - and makes it a lot more complicated just so that it can reuse a tiny part of that functionality for something that is only peripherally related.
> > > 
> > > Did you consider if maybe the code change would be smaller, and the result easier to understand, if you just wrote a specialized watchpoint for the toStringTag case?  I believe that you aren't actually saving any code by doing this kind of generalization.  Instead, you're making the code more complicated because now I have to search through multiple files to figure out what the firing of one of those watchpoints does.
> > 
> > I generalized adaptive watchpoints because in order to make the toStringTag
> > optimization we need to know that the value stored in the Symbol.toStringTag
> > slot is a constant. If I were to implement a specialized watchpoint the code
> > would be essentially identical to the adaptive watchpoint code. While, I
> > agree that the code might be smaller and slightly less complex if wrote a
> > specialized watchpoint the code maintenance overhead from having two roughly
> > identical implementations is probably not worthwhile.
> 
> There is a non-trivial cost to any code that uses templates in this manner. 
> We have to weigh that cost against the cost of slight code duplication.
> 
> Note that we already have multiple watchpoints that essentially duplicate
> this pattern.  It's a simple pattern to work with, so I don't think it's bad
> to duplicate it.
> 

Is "this pattern" referring to the general watchpoint pattern or the adaptive watchpoint pattern? The inferred property value watchpoint in particular seems sufficiently complex for it to be profitable to have a generalized version.

> > 
> > >> Source/JavaScriptCore/bytecode/AdaptiveInferredPropertyValueWatchpointBaseInlines.h:68
> > >> +    T::handleFire(static_cast<T*>(this), detail);
> > > 
> > > This is really weird.  Why isn't this a virtual method?
> > 
> > In hindsight, this should definitely be a virtual method. Fixed.
> 
> Does that allow you to get rid of the template stuff?
> 

Yes. It's gone now.

[Filip Pizlo]

(In reply to comment #17)
> (In reply to comment #16)
> > (In reply to comment #15)
> > > Comment on attachment 264962 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=264962&action=review
> > > 
> > > >> Source/JavaScriptCore/ChangeLog:13
> > > >> +        them that clear out the toString value cache when fired have been added.
> > > > 
> > > > Why did you generalize adaptive watchpoints?
> > > > 
> > > > I think that this patch takes something that is very simple - the adaptive watchpoint code in the DFG - and makes it a lot more complicated just so that it can reuse a tiny part of that functionality for something that is only peripherally related.
> > > > 
> > > > Did you consider if maybe the code change would be smaller, and the result easier to understand, if you just wrote a specialized watchpoint for the toStringTag case?  I believe that you aren't actually saving any code by doing this kind of generalization.  Instead, you're making the code more complicated because now I have to search through multiple files to figure out what the firing of one of those watchpoints does.
> > > 
> > > I generalized adaptive watchpoints because in order to make the toStringTag
> > > optimization we need to know that the value stored in the Symbol.toStringTag
> > > slot is a constant. If I were to implement a specialized watchpoint the code
> > > would be essentially identical to the adaptive watchpoint code. While, I
> > > agree that the code might be smaller and slightly less complex if wrote a
> > > specialized watchpoint the code maintenance overhead from having two roughly
> > > identical implementations is probably not worthwhile.
> > 
> > There is a non-trivial cost to any code that uses templates in this manner. 
> > We have to weigh that cost against the cost of slight code duplication.
> > 
> > Note that we already have multiple watchpoints that essentially duplicate
> > this pattern.  It's a simple pattern to work with, so I don't think it's bad
> > to duplicate it.
> > 
> 
> Is "this pattern" referring to the general watchpoint pattern or the
> adaptive watchpoint pattern? The inferred property value watchpoint in
> particular seems sufficiently complex for it to be profitable to have a
> generalized version.

I just mean having a fire() method that does this:

    if (!m_key.isWatchable(PropertyCondition::EnsureWatchability)) {
        // jettison code, clear a stub, whatever.
        return;
    }
    
    m_key.object()->structure()->addTransitionWatchpoint(this);

But I get now that you're referring to the more complex logic involved in inferred values.  I can see wanting that to be abstract.

> 
> > > 
> > > >> Source/JavaScriptCore/bytecode/AdaptiveInferredPropertyValueWatchpointBaseInlines.h:68
> > > >> +    T::handleFire(static_cast<T*>(this), detail);
> > > > 
> > > > This is really weird.  Why isn't this a virtual method?
> > > 
> > > In hindsight, this should definitely be a virtual method. Fixed.
> > 
> > Does that allow you to get rid of the template stuff?
> > 
> 
> Yes. It's gone now.

OK - put the patch up when you're ready.

[Keith Miller]

Created attachment 265195 [details]
Patch

[Keith Miller]

Created attachment 265221 [details]
Patch

[Keith Miller]

Re-uploaded the patch since I forgot an expect and needed to rebase. Other than that it is the same however.

[Keith Miller]

Created attachment 265248 [details]
Patch

[Keith Miller]

Added a new patch to check CMake build... :|

[Chris Dumez]

(In reply to comment #23)
> Added a new patch to check CMake build... :|

Ready to commit?

[WebKit Commit Bot]

Comment on attachment 265248 [details]
Patch

Rejecting attachment 265248 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 265248, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:

patching file LayoutTests/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file LayoutTests/http/tests/history/cross-origin-replace-history-object-child-expected.txt
patching file LayoutTests/js/regress/script-tests/symbol-tostringtag.js
patching file LayoutTests/js/regress/symbol-tostringtag-expected.txt
patching file LayoutTests/js/regress/symbol-tostringtag.html

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.webkit.org/results/416073

[Keith Miller]

Guess I need to do it by hand.

[Keith Miller]

Committed r192321: <http://trac.webkit.org/changeset/192321>