WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
REOPENED
131083
Rewrite Function.bind as a builtin
https://bugs.webkit.org/show_bug.cgi?id=131083
Summary
Rewrite Function.bind as a builtin
Oliver Hunt
Reported
2014-04-01 16:51:26 PDT
Rewrite Function.bind as a builtin
Attachments
Patch
(51.17 KB, patch)
2014-04-01 17:00 PDT
,
Oliver Hunt
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2
(575.51 KB, application/zip)
2014-04-01 18:04 PDT
,
Build Bot
no flags
Details
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion
(634.79 KB, application/zip)
2014-04-01 18:27 PDT
,
Build Bot
no flags
Details
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2
(580.01 KB, application/zip)
2014-04-01 19:18 PDT
,
Build Bot
no flags
Details
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion
(581.14 KB, application/zip)
2014-04-01 19:38 PDT
,
Build Bot
no flags
Details
Patch
(52.55 KB, patch)
2014-04-01 23:10 PDT
,
Oliver Hunt
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion
(521.06 KB, application/zip)
2014-04-02 01:04 PDT
,
Build Bot
no flags
Details
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2
(665.19 KB, application/zip)
2014-04-02 01:19 PDT
,
Build Bot
no flags
Details
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion
(521.64 KB, application/zip)
2014-04-02 02:04 PDT
,
Build Bot
no flags
Details
Patch
(56.25 KB, patch)
2014-04-08 13:44 PDT
,
Oliver Hunt
no flags
Details
Formatted Diff
Diff
Patch
(97.28 KB, patch)
2014-04-08 19:47 PDT
,
Oliver Hunt
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion
(493.38 KB, application/zip)
2014-04-08 21:31 PDT
,
Build Bot
no flags
Details
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion
(489.97 KB, application/zip)
2014-04-08 22:36 PDT
,
Build Bot
no flags
Details
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2
(469.87 KB, application/zip)
2014-04-09 05:18 PDT
,
Build Bot
no flags
Details
Patch
(111.00 KB, patch)
2014-04-12 14:06 PDT
,
Oliver Hunt
ggaren
: review+
Details
Formatted Diff
Diff
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
Oliver Hunt
Comment 1
2014-04-01 17:00:28 PDT
Created
attachment 228343
[details]
Patch
WebKit Commit Bot
Comment 2
2014-04-01 17:02:59 PDT
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 3
2014-04-01 18:04:42 PDT
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
Comment 4
2014-04-01 18:04:45 PDT
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 5
2014-04-01 18:27:35 PDT
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
Comment 6
2014-04-01 18:27:37 PDT
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 7
2014-04-01 19:18:56 PDT
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
Comment 8
2014-04-01 19:18:58 PDT
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 9
2014-04-01 19:38:07 PDT
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
Comment 10
2014-04-01 19:38:09 PDT
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
Comment 11
2014-04-01 23:10:02 PDT
Created
attachment 228365
[details]
Patch
Build Bot
Comment 12
2014-04-02 01:04:21 PDT
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
Comment 13
2014-04-02 01:04:26 PDT
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 14
2014-04-02 01:19:10 PDT
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
Comment 15
2014-04-02 01:19:12 PDT
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 16
2014-04-02 02:04:40 PDT
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
Comment 17
2014-04-02 02:04:43 PDT
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 18
2014-04-08 10:44:08 PDT
Comment on
attachment 228365
[details]
Patch This was for build testing
Oliver Hunt
Comment 19
2014-04-08 13:44:36 PDT
Created
attachment 228879
[details]
Patch
Geoffrey Garen
Comment 20
2014-04-08 14:36:35 PDT
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
Comment 21
2014-04-08 19:47:39 PDT
Created
attachment 228926
[details]
Patch
Geoffrey Garen
Comment 22
2014-04-08 20:14:07 PDT
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
Comment 23
2014-04-08 20:15:19 PDT
Please fix Windows build before landing.
Build Bot
Comment 24
2014-04-08 21:31:31 PDT
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
Comment 25
2014-04-08 21:31:36 PDT
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 26
2014-04-08 22:36:26 PDT
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
Comment 27
2014-04-08 22:36:35 PDT
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 28
2014-04-09 05:18:20 PDT
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
Comment 29
2014-04-09 05:18:32 PDT
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
Comment 30
2014-04-09 10:29:12 PDT
Committed
r167020
: <
http://trac.webkit.org/changeset/167020
>
Gavin Barraclough
Comment 31
2014-04-11 15:39:26 PDT
Rolled out in
r167165
due to DYEBench performance regression.
Oliver Hunt
Comment 32
2014-04-12 14:06:15 PDT
Created
attachment 229211
[details]
Patch
WebKit Commit Bot
Comment 33
2014-04-12 14:10:03 PDT
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 34
2014-04-12 14:17:28 PDT
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
Comment 35
2014-04-13 11:02:26 PDT
Committed
r167199
: <
http://trac.webkit.org/changeset/167199
>
Ryosuke Niwa
Comment 36
2014-04-13 19:15:11 PDT
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
Comment 37
2014-04-13 23:48:23 PDT
(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
Comment 38
2014-04-14 01:19:01 PDT
./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
Comment 39
2014-04-14 02:09:01 PDT
(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
Comment 40
2014-04-14 09:34:32 PDT
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
Comment 41
2014-04-14 09:44:10 PDT
(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
Comment 42
2014-04-14 09:54:07 PDT
!? 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
Comment 43
2014-04-14 09:59:49 PDT
(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
Comment 44
2014-04-14 10:14:22 PDT
(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
Comment 45
2014-04-14 10:16:43 PDT
> > 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
Comment 46
2014-04-14 10:24:01 PDT
(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
Comment 47
2014-04-15 00:58:19 PDT
Re-opened since this is blocked by
bug 131671
Ryosuke Niwa
Comment 48
2014-04-15 09:23:48 PDT
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.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug