Bug 160678 - JSBoundFunction should lazily generate its name string
Summary: JSBoundFunction should lazily generate its name string
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Saam Barati
URL:
Keywords: InRadar
Depends on: 160737
Blocks:
  Show dependency treegraph
 
Reported: 2016-08-08 16:57 PDT by Saam Barati
Modified: 2016-08-10 13:02 PDT (History)
18 users (show)

See Also:


Attachments
patch (9.72 KB, patch)
2016-08-09 12:10 PDT, Saam Barati
mark.lam: review+
Details | Formatted Diff | Diff
patch for landing (10.75 KB, patch)
2016-08-09 12:37 PDT, Saam Barati
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-yosemite (1.16 MB, application/zip)
2016-08-09 13:29 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-yosemite-wk2 (1.05 MB, application/zip)
2016-08-09 13:33 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews121 for ios-simulator-elcapitan-wk2 (678.19 KB, application/zip)
2016-08-09 13:43 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews114 for mac-yosemite (1.58 MB, application/zip)
2016-08-09 13:44 PDT, Build Bot
no flags Details
patch for landing (13.19 KB, patch)
2016-08-09 14:50 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
patch for landing (13.75 KB, patch)
2016-08-09 15:39 PDT, Saam Barati
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Saam Barati 2016-08-08 16:57:31 PDT
...
Comment 1 Saam Barati 2016-08-08 16:59:14 PDT
<rdar://problem/27043194>
Comment 2 Saam Barati 2016-08-09 12:10:19 PDT
Created attachment 285659 [details]
patch
Comment 3 Mark Lam 2016-08-09 12:23:03 PDT
Comment on attachment 285659 [details]
patch

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

r=me

> Source/JavaScriptCore/runtime/JSBoundFunction.cpp:177
> -    function->finishCreation(vm, executable, length, makeString("bound ", name));
> +    function->finishCreation(vm, executable, length, String());

Why not remove the name string from JSBoundFunction::finishCreation() altogether?  Have it pass String() to its base instead.
Comment 4 Saam Barati 2016-08-09 12:31:16 PDT
Comment on attachment 285659 [details]
patch

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

>> Source/JavaScriptCore/runtime/JSBoundFunction.cpp:177
>> +    function->finishCreation(vm, executable, length, String());
> 
> Why not remove the name string from JSBoundFunction::finishCreation() altogether?  Have it pass String() to its base instead.

Will do. I was thinking that the finishCreation was calling JSFunction::finishCreation. I didn't realize it was calling JSBoundFunction::finishCreation.
Comment 5 Saam Barati 2016-08-09 12:37:05 PDT
Created attachment 285662 [details]
patch for landing
Comment 6 Build Bot 2016-08-09 13:29:44 PDT
Comment on attachment 285662 [details]
patch for landing

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

New failing tests:
js/regress/bound-function-construction-performance.html
inspector/runtime/getProperties.html
Comment 7 Build Bot 2016-08-09 13:29:47 PDT
Created attachment 285667 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 8 Build Bot 2016-08-09 13:33:18 PDT
Comment on attachment 285662 [details]
patch for landing

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

New failing tests:
js/regress/bound-function-construction-performance.html
inspector/runtime/getProperties.html
Comment 9 Build Bot 2016-08-09 13:33:22 PDT
Created attachment 285669 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 10 Build Bot 2016-08-09 13:43:49 PDT
Comment on attachment 285662 [details]
patch for landing

Attachment 285662 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1841585

New failing tests:
js/regress/bound-function-construction-performance.html
Comment 11 Build Bot 2016-08-09 13:43:52 PDT
Created attachment 285670 [details]
Archive of layout-test-results from ews121 for ios-simulator-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-elcapitan-wk2  Platform: Mac OS X 10.11.5
Comment 12 Build Bot 2016-08-09 13:44:19 PDT
Comment on attachment 285662 [details]
patch for landing

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

New failing tests:
js/regress/bound-function-construction-performance.html
inspector/runtime/getProperties.html
Comment 13 Build Bot 2016-08-09 13:44:23 PDT
Created attachment 285671 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 14 Saam Barati 2016-08-09 14:50:29 PDT
Created attachment 285677 [details]
patch for landing
Comment 15 Saam Barati 2016-08-09 15:39:49 PDT
Created attachment 285681 [details]
patch for landing
Comment 16 WebKit Commit Bot 2016-08-09 20:40:34 PDT
Comment on attachment 285681 [details]
patch for landing

Clearing flags on attachment: 285681

Committed r204321: <http://trac.webkit.org/changeset/204321>
Comment 17 WebKit Commit Bot 2016-08-09 20:40:40 PDT
All reviewed patches have been landed.  Closing bug.
Comment 18 Csaba Osztrogonác 2016-08-10 06:28:49 PDT
(In reply to comment #16)
> Comment on attachment 285681 [details]
> patch for landing
> 
> Clearing flags on attachment: 285681
> 
> Committed r204321: <http://trac.webkit.org/changeset/204321>

The new regress/script-tests/bound-function-construction-performance.js
times out in debug mode on all bots, see build.webkit.org for details.
Comment 19 Saam Barati 2016-08-10 09:39:53 PDT
(In reply to comment #18)
> (In reply to comment #16)
> > Comment on attachment 285681 [details]
> > patch for landing
> > 
> > Clearing flags on attachment: 285681
> > 
> > Committed r204321: <http://trac.webkit.org/changeset/204321>
> 
> The new regress/script-tests/bound-function-construction-performance.js
> times out in debug mode on all bots, see build.webkit.org for details.

We should be skipping this test in Debug mode.
Is this not happening?
Comment 20 Mark Lam 2016-08-10 09:42:28 PDT
(In reply to comment #19)
> (In reply to comment #18)
> > (In reply to comment #16)
> > > Comment on attachment 285681 [details]
> > > patch for landing
> > > 
> > > Clearing flags on attachment: 285681
> > > 
> > > Committed r204321: <http://trac.webkit.org/changeset/204321>
> > 
> > The new regress/script-tests/bound-function-construction-performance.js
> > times out in debug mode on all bots, see build.webkit.org for details.
> 
> We should be skipping this test in Debug mode.
> Is this not happening?

Looks like Ossy is talking about the Debug JSC tests.  We need to look into skipping it for the JSC tests too.
Comment 21 Mark Lam 2016-08-10 09:46:18 PDT
(In reply to comment #20)
> Looks like Ossy is talking about the Debug JSC tests.  We need to look into
> skipping it for the JSC tests too.

How about adding "//@ slow!"?
Comment 22 Csaba Osztrogonác 2016-08-10 09:52:00 PDT
(In reply to comment #21)
> (In reply to comment #20)
> > Looks like Ossy is talking about the Debug JSC tests.  We need to look into
> > skipping it for the JSC tests too.
> 
> How about adding "//@ slow!"?

slow doesn't increase the timeout, only forces to run before all normal tests
Comment 23 WebKit Commit Bot 2016-08-10 09:58:31 PDT
Re-opened since this is blocked by bug 160737
Comment 24 Alexey Proskuryakov 2016-08-10 10:02:28 PDT
Also, this broke inspector/model/remote-object-get-properties.html, as order of properties changed:

 DISPLAYABLE PROPERTIES:
-    name
-    length
-    arguments
-    caller
+    length
+    arguments
+    caller
+    name
     __proto__
     targetFunction
     boundThis
     boundArgs
 
 ALL PROPERTIES:
-    name
-    length
-    arguments
-    caller
+    length
+    arguments
+    caller
+    name
     toString
     apply
     call

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=inspector%2Fmodel%2Fremote-object-get-properties.html
Comment 25 Alexey Proskuryakov 2016-08-10 13:02:08 PDT
Back to RESOLVED/FIXED, as we didn't end up rolling out the patch, as far as I can tell.