Bug 160678 – JSBoundFunction should lazily generate its name string

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Saam Barati]

...

[Saam Barati]

<rdar://problem/27043194>

[Saam Barati]

Created attachment 285659 [details]
patch

[Mark Lam]

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.

[Saam Barati]

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.

[Saam Barati]

Created attachment 285662 [details]
patch for landing

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Saam Barati]

Created attachment 285677 [details]
patch for landing

[Saam Barati]

Created attachment 285681 [details]
patch for landing

[WebKit Commit Bot]

Comment on attachment 285681 [details]
patch for landing

Clearing flags on attachment: 285681

Committed r204321: <http://trac.webkit.org/changeset/204321>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

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

[Saam Barati]

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

[Mark Lam]

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

[Mark Lam]

(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!"?

[Csaba Osztrogonác]

(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

[WebKit Commit Bot]

Re-opened since this is blocked by bug 160737

[Alexey Proskuryakov]

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

[Alexey Proskuryakov]

Back to RESOLVED/FIXED, as we didn't end up rolling out the patch, as far as I can tell.