Bug 229019

Summary:

[JSC] Enable ThinLTO

Product:

WebKit

Reporter:

Yusuke Suzuki <ysuzuki>

Component:

New Bugs

Assignee:

Saam Barati <saam>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ap, ews-watchlist, keith_miller, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	ap: review-
patch	none

Yusuke Suzuki

Reported 2021-08-11 20:34:49 PDT

[JSC] Enable ThinLTO

Attachments
Patch (4.86 KB, patch) 2021-08-11 20:39 PDT, Yusuke Suzuki	no flags	Details Formatted Diff Diff
Patch (4.86 KB, patch) 2021-08-11 20:41 PDT, Yusuke Suzuki	ap: review-	Details Formatted Diff Diff
patch (1.52 KB, patch) 2022-03-08 07:58 PST, Saam Barati	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Yusuke Suzuki

Comment 1 2021-08-11 20:39:04 PDT

Created attachment 435392 [details] Patch

Yusuke Suzuki

Comment 2 2021-08-11 20:41:24 PDT

Created attachment 435393 [details] Patch

Alexey Proskuryakov

Comment 3 2021-08-11 20:44:51 PDT

Comment on attachment 435393 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=435393&action=review > Source/JavaScriptCore/ChangeLog:3 > + [JSC] Enable ThinLTO What is the build time impact? What I’m observing for the rest of WebKit is substantially slower than what was promised when we were enabling it, and we should consider disabling it everywhere :( If this adds minutes to build time, it’s likely not worth 0.3%.

Mark Lam

Comment 4 2021-08-11 20:51:18 PDT

Comment on attachment 435393 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=435393&action=review r=me > Source/JavaScriptCore/ChangeLog:14 > + We should enable ThinLTO for all WebKit components (only JSC is not using that). I think you meant to say that "We already enable ThinLTO for all WebKit components (only JSC is not using it). This patch now allows JSC to use ThinLTO too."

Yusuke Suzuki

Comment 5 2021-08-11 20:52:26 PDT

Comment on attachment 435393 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=435393&action=review >> Source/JavaScriptCore/ChangeLog:3 >> + [JSC] Enable ThinLTO > > What is the build time impact? What I’m observing for the rest of WebKit is substantially slower than what was promised when we were enabling it, and we should consider disabling it everywhere :( > > If this adds minutes to build time, it’s likely not worth 0.3%. No, we are really needing to have any sub-percent currently. 0.3% is *huge*. >> Source/JavaScriptCore/ChangeLog:14 >> + We should enable ThinLTO for all WebKit components (only JSC is not using that). > > I think you meant to say that "We already enable ThinLTO for all WebKit components (only JSC is not using it). This patch now allows JSC to use ThinLTO too." Added.

Alexey Proskuryakov

Comment 6 2021-08-11 20:54:29 PDT

Comment on attachment 435393 [details] Patch r-, this is unacceptable to land when the trade off isn’t even measured.

Keith Miller

Comment 7 2021-08-12 08:26:13 PDT

I'm not going to comment on whether .3% is or isn't worth the trade-off in build time but I think it

Keith Miller

Comment 8 2021-08-12 08:29:29 PDT

(In reply to Keith Miller from comment #7) > I'm not going to comment on whether .3% is or isn't worth the trade-off in > build time but I think it Ugh, I hate how easy it is to press tab then enter and submit before finishing typing... Anyway, I'm not going to comment on whether .3% is or isn't worth the trade-off in build time but I think it might be worth considering having a non-lto release testing build flavor and a release/production build, which enables it. We already have this to some degree for JSC, where we have a release build with debug asserts enabled. Perhaps we should consider migrating to something like that more generally?

Yusuke Suzuki

Comment 9 2021-08-12 17:40:55 PDT

Comment on attachment 435393 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=435393&action=review >>> Source/JavaScriptCore/ChangeLog:3 >>> + [JSC] Enable ThinLTO >> >> What is the build time impact? What I’m observing for the rest of WebKit is substantially slower than what was promised when we were enabling it, and we should consider disabling it everywhere :( >> >> If this adds minutes to build time, it’s likely not worth 0.3%. > > No, we are really needing to have any sub-percent currently. 0.3% is *huge*. Turned out this only adds ~1 min to builder.

Alexey Proskuryakov

Comment 10 2021-08-12 20:01:32 PDT

Comment on attachment 435393 [details] Patch This is still not ready for review, as discussed online.

Radar WebKit Bug Importer

Comment 11 2021-08-18 20:35:27 PDT

<rdar://problem/82107543>

Saam Barati

Comment 12 2022-03-08 07:58:35 PST

Created attachment 454122 [details] patch

Mark Lam

Comment 13 2022-03-08 08:04:10 PST

Comment on attachment 454122 [details] patch r=me

EWS

Comment 14 2022-03-08 08:43:41 PST

Committed r290993 (248171@main): <https://commits.webkit.org/248171@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 454122 [details].

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