Regression(r191815): 5.3% regression on Dromaeo JS Library Benchmark
Created attachment 264960 [details] Patch
Created attachment 264962 [details] Patch
Created attachment 264963 [details] Benchmark results
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
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
Can you add a regress performance test?
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
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
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
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
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
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
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
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.
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.
(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.
(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.
(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.
Created attachment 265195 [details] Patch
Created attachment 265221 [details] Patch
Re-uploaded the patch since I forgot an expect and needed to rebase. Other than that it is the same however.
Created attachment 265248 [details] Patch
Added a new patch to check CMake build... :|
(In reply to comment #23) > Added a new patch to check CMake build... :| Ready to commit?
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
Guess I need to do it by hand.
Committed r192321: <http://trac.webkit.org/changeset/192321>