Bug 131083 – Rewrite Function.bind as a builtin

• Status: REOPENED
• Component: New Bugs

[Oliver Hunt]

Rewrite Function.bind as a builtin

[Oliver Hunt]

Created attachment 228343 [details]
Patch

[WebKit Commit Bot]

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.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Oliver Hunt]

Created attachment 228365 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Oliver Hunt]

Comment on attachment 228365 [details]
Patch

This was for build testing

[Oliver Hunt]

Created attachment 228879 [details]
Patch

[Geoffrey Garen]

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

[Oliver Hunt]

Created attachment 228926 [details]
Patch

[Geoffrey Garen]

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__".

[Geoffrey Garen]

Please fix Windows build before landing.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Oliver Hunt]

Committed r167020: <http://trac.webkit.org/changeset/167020>

[Gavin Barraclough]

Rolled out in r167165 due to DYEBench performance regression.

[Oliver Hunt]

Created attachment 229211 [details]
Patch

[WebKit Commit Bot]

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.

[Geoffrey Garen]

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.

[Oliver Hunt]

Committed r167199: <http://trac.webkit.org/changeset/167199>

[Ryosuke Niwa]

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

[Oliver Hunt]

(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

[Ryosuke Niwa]

./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

[Csaba Osztrogonác]

(In reply to comment #35)
> Committed r167199: <http://trac.webkit.org/changeset/167199>

It broke the bindings tests too, could you possibly fix it?

[Geoffrey Garen]

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?

[Oliver Hunt]

(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

[Ryosuke Niwa]

!?  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.

[Ryosuke Niwa]

(In reply to comment #42)
> !?  The bot is consistent with our null hypothesis that this patch caused a regression.

s/null hypothesis/hypothesis/.

[Oliver Hunt]

(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 :)

[Geoffrey Garen]

> > 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?

[Oliver Hunt]

(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.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 131671

[Ryosuke Niwa]

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.