|
Description
Oliver Hunt
2014-04-01 16:51:26 PDT
Created attachment 228343 [details]
Patch
Attachment 228343 [details] did not pass style-queue:
ERROR: Source/JavaScriptCore/ChangeLog:16: Need whitespace between colon and description [changelog/filechangedescriptionwhitespace] [5]
ERROR: Source/JavaScriptCore/ChangeLog:22: Need whitespace between colon and description [changelog/filechangedescriptionwhitespace] [5]
Total errors found: 2 in 21 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 228343 [details] Patch Attachment 228343 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/4665576098103296 New failing tests: platform/mac/fast/scrolling/scroll-iframe-latched-mainframe.html ietestcenter/Javascript/15.3.4.5-15-2.html platform/mac/fast/scrolling/scroll-select-latched-mainframe.html inspector-protocol/runtime/getProperties.html ietestcenter/Javascript/15.3.4.5-13.b-1.html js/dom/function-bind.html ietestcenter/Javascript/15.3.4.5-15-1.html ietestcenter/Javascript/15.3.4.5-13.b-2.html platform/mac/fast/scrolling/scroll-div-latched-mainframe.html Created attachment 228352 [details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-14 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Comment on attachment 228343 [details] Patch Attachment 228343 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5873971320848384 New failing tests: ietestcenter/Javascript/15.3.4.5-15-2.html inspector-protocol/runtime/getProperties.html ietestcenter/Javascript/15.3.4.5-13.b-1.html js/dom/function-bind.html ietestcenter/Javascript/15.3.4.5-15-1.html ietestcenter/Javascript/15.3.4.5-13.b-2.html Created attachment 228354 [details]
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-02 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Comment on attachment 228343 [details] Patch Attachment 228343 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5372149757902848 New failing tests: ietestcenter/Javascript/15.3.4.5-15-2.html inspector-protocol/runtime/getProperties.html ietestcenter/Javascript/15.3.4.5-13.b-1.html js/dom/function-bind.html ietestcenter/Javascript/15.3.4.5-15-1.html ietestcenter/Javascript/15.3.4.5-13.b-2.html Created attachment 228357 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-11 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Comment on attachment 228343 [details] Patch Attachment 228343 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5264563779928064 New failing tests: ietestcenter/Javascript/15.3.4.5-15-2.html inspector-protocol/runtime/getProperties.html ietestcenter/Javascript/15.3.4.5-13.b-1.html js/dom/function-bind.html ietestcenter/Javascript/15.3.4.5-15-1.html ietestcenter/Javascript/15.3.4.5-13.b-2.html Created attachment 228358 [details]
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-04 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Created attachment 228365 [details]
Patch
Comment on attachment 228365 [details] Patch Attachment 228365 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/4899419887501312 New failing tests: inspector-protocol/runtime/getProperties.html ietestcenter/Javascript/15.3.4.5-13.b-1.html ietestcenter/Javascript/15.3.4.5-15-2.html ietestcenter/Javascript/15.3.4.5-13.b-2.html ietestcenter/Javascript/15.3.4.5-15-1.html Created attachment 228369 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-07 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Comment on attachment 228365 [details] Patch Attachment 228365 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5532359822671872 New failing tests: inspector-protocol/runtime/getProperties.html ietestcenter/Javascript/15.3.4.5-13.b-1.html ietestcenter/Javascript/15.3.4.5-15-2.html ietestcenter/Javascript/15.3.4.5-13.b-2.html ietestcenter/Javascript/15.3.4.5-15-1.html Created attachment 228371 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Comment on attachment 228365 [details] Patch Attachment 228365 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/6390718432018432 New failing tests: inspector-protocol/runtime/getProperties.html ietestcenter/Javascript/15.3.4.5-13.b-1.html ietestcenter/Javascript/15.3.4.5-15-2.html ietestcenter/Javascript/15.3.4.5-13.b-2.html ietestcenter/Javascript/15.3.4.5-15-1.html Created attachment 228374 [details]
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-03 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Comment on attachment 228365 [details]
Patch
This was for build testing
Created attachment 228879 [details]
Patch
Comment on attachment 228879 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=228879&action=review This patch looks good, but I'd like you to code up a change below, or something like it: > Source/JavaScriptCore/ChangeLog:11 > + Instead we not just return a regular JS closure with a few "just" > Source/JavaScriptCore/runtime/JSObject.h:1212 > + if (propertyName == vm.propertyNames->prototypePrivateName) > + propertyName = vm.propertyNames->prototype; Let's try to avoid tight coupling between the details of function.bind, instanceof, and generic property access. Here's a suggestion: (1) instanceof does a getById for prototoypeForHasInstancePrivateName (2) function.bind functions respond to prototoypeForHasInstancePrivateName by forwarding to their targets' .prototype property (3) Standard functions intercept sets of .prototype, and also set prototoypeforhadinstanceprivatename -- this makes instanceof cacheable for standard functions (4) This change to general property access can be removed Created attachment 228926 [details]
Patch
Comment on attachment 228926 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=228926&action=review r=me This looks much better. Please file a follow-up bug about performance of the .bind() case when extra arguments are bound. > Source/JavaScriptCore/runtime/JSGlobalObjectFunctions.cpp:815 > + // Setting __proto__ of a primitive should have no effect. This shouldn't say "__proto__". Please fix Windows build before landing. Comment on attachment 228926 [details] Patch Attachment 228926 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/4756157897900032 New failing tests: inspector-protocol/runtime/getProperties.html Created attachment 228933 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Comment on attachment 228926 [details] Patch Attachment 228926 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5765911151640576 New failing tests: inspector-protocol/runtime/getProperties.html Created attachment 228936 [details]
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-02 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Comment on attachment 228926 [details] Patch Attachment 228926 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/6579382386688000 New failing tests: inspector-protocol/runtime/getProperties.html Created attachment 228957 [details]
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-10 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Committed r167020: <http://trac.webkit.org/changeset/167020> Rolled out in r167165 due to DYEBench performance regression. Created attachment 229211 [details]
Patch
Attachment 229211 [details] did not pass style-queue:
ERROR: Source/JavaScriptCore/parser/Nodes.h:219: This { should be at the end of the previous line [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/parser/Nodes.h:235: This { should be at the end of the previous line [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/parser/Nodes.h:254: This { should be at the end of the previous line [whitespace/braces] [4]
Total errors found: 3 in 68 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 229211 [details]
Patch
Can you add a test for the bind function that caused the DYEB regression?
Also, you need a test case that covers each of the branches inside Function.bind.
Committed r167199: <http://trac.webkit.org/changeset/167199> Looks like this patch introduced about the same amount of regression in DYEBench: https://perf.webkit.org/#mode=charts&chartList=%5B%5B%22mac-mavericks%22%2C%22DoYouEvenBench%2FFull%3ATime%3ATotal%22%5D%5D&days=13&zoom=%5B1396973745612%2C1397428098568.3108%5D (In reply to comment #36) > Looks like this patch introduced about the same amount of regression in DYEBench: > https://perf.webkit.org/#mode=charts&chartList=%5B%5B%22mac-mavericks%22%2C%22DoYouEvenBench%2FFull%3ATime%3ATotal%22%5D%5D&days=13&zoom=%5B1396973745612%2C1397428098568.3108%5D Goddammit, how is the perfbot running dye? I'm not seeing any regression ./Tools/Scripts/run-perf-tests --release DoYouEvenBench should do the trick. Perhaps, the regression only reproduces in some CPUs? Here's the machine we use for mac-mavericks perf bot: http://build.webkit.org/buildslaves/apple-mini-213 (In reply to comment #35) > Committed r167199: <http://trac.webkit.org/changeset/167199> It broke the bindings tests too, could you possibly fix it? It looks like the regression appeared, and then disappeared, and then reappeared again on the bot. Perhaps the bot has some background task making noise? (In reply to comment #40) > It looks like the regression appeared, and then disappeared, and then reappeared again on the bot. Perhaps the bot has some background task making noise? So much weirdness. investigating it still !? The bot is consistent with our null hypothesis that this patch caused a regression. The first increase in runtime was introduced by the initial patch, then the rollout of the patch fixed temporarily until the second patch was landed again at which point the runtime increased again. (In reply to comment #42) > !? The bot is consistent with our null hypothesis that this patch caused a regression. s/null hypothesis/hypothesis/. (In reply to comment #42) > !? The bot is consistent with our null hypothesis that this patch caused a regression. > > The first increase in runtime was introduced by the initial patch, then the rollout of the patch fixed temporarily until the second patch was landed again at which point the runtime increased again. Ah, i assumed geoff was aware of the rollout :) > > The first increase in runtime was introduced by the initial patch, then the rollout of the patch fixed temporarily until the second patch was landed again at which point the runtime increased again. > > Ah, i assumed geoff was aware of the rollout :) But how do we explain the return to the regressed state @ r167199, after the rollout? (In reply to comment #45) > > > The first increase in runtime was introduced by the initial patch, then the rollout of the patch fixed temporarily until the second patch was landed again at which point the runtime increased again. > > > > Ah, i assumed geoff was aware of the rollout :) > > But how do we explain the return to the regressed state @ r167199, after the rollout? r167199 was me landing it again with the belief i had fixed the regression. Alas i apparently had not. Re-opened since this is blocked by bug 131671 Since ap reverted the improvement to .bind in http://trac.webkit.org/changeset/167297, we currently have 2.5% regression from this change in trunk. |