Bug 160678

Summary:

JSBoundFunction should lazily generate its name string

Product:

WebKit

Reporter:

Saam Barati <saam>

Component:

JavaScriptCore

Assignee:

Saam Barati <saam>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

barraclough, benjamin, buildbot, commit-queue, fpizlo, ggaren, gskachkov, keith_miller, mark.lam, msaboff, oliver, ossy, rniwa, ryanhaddad, sukolsak, ticaiolima, webkit-bug-importer, ysuzuki

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

See Also:

https://bugs.webkit.org/show_bug.cgi?id=160738

Bug Depends on:

160737

Bug Blocks:

Attachments:

Description	Flags
patch	mark.lam: review+
patch for landing	buildbot: commit-queue-
Archive of layout-test-results from ews103 for mac-yosemite	none
Archive of layout-test-results from ews105 for mac-yosemite-wk2	none
Archive of layout-test-results from ews121 for ios-simulator-elcapitan-wk2	none
Archive of layout-test-results from ews114 for mac-yosemite	none
patch for landing	none
patch for landing	none

Saam Barati

Reported 2016-08-08 16:57:31 PDT

...

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
Show Obsolete (7) View All Add attachment proposed patch, testcase, etc.

Saam Barati

Comment 1 2016-08-08 16:59:14 PDT

<rdar://problem/27043194>

Saam Barati

Comment 2 2016-08-09 12:10:19 PDT

Created attachment 285659 [details] patch

Mark Lam

Comment 3 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.

Saam Barati

Comment 4 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.

Saam Barati

Comment 5 2016-08-09 12:37:05 PDT

Created attachment 285662 [details] patch for landing

Build Bot

Comment 6 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

Build Bot

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

Build Bot

Comment 8 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

Build Bot

Comment 9 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

Build Bot

Comment 10 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

Build Bot

Comment 11 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

Build Bot

Comment 12 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

Build Bot

Comment 13 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

Saam Barati

Comment 14 2016-08-09 14:50:29 PDT

Created attachment 285677 [details] patch for landing

Saam Barati

Comment 15 2016-08-09 15:39:49 PDT

Created attachment 285681 [details] patch for landing

WebKit Commit Bot

Comment 16 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>

WebKit Commit Bot

Comment 17 2016-08-09 20:40:40 PDT

All reviewed patches have been landed. Closing bug.

Csaba Osztrogonác

Comment 18 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.

Saam Barati

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

Mark Lam

Comment 20 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.

Mark Lam

Comment 21 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!"?

Csaba Osztrogonác

Comment 22 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

WebKit Commit Bot

Comment 23 2016-08-10 09:58:31 PDT

Re-opened since this is blocked by bug 160737

Alexey Proskuryakov

Comment 24 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

Alexey Proskuryakov

Comment 25 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.

Note You need to log in before you can comment on or make changes to this bug.