Bug 229019 – [JSC] Enable ThinLTO

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

[JSC] Enable ThinLTO

[Yusuke Suzuki]

Created attachment 435392 [details]
Patch

[Yusuke Suzuki]

Created attachment 435393 [details]
Patch

[Alexey Proskuryakov]

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 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 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 on attachment 435393 [details]
Patch

r-, this is unacceptable to land when the trade off isn’t even measured.

[Keith Miller]

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]

(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 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 on attachment 435393 [details]
Patch

This is still not ready for review, as discussed online.

[Radar WebKit Bug Importer]

<rdar://problem/82107543>

[Saam Barati]

Created attachment 454122 [details]
patch

[Mark Lam]

Comment on attachment 454122 [details]
patch

r=me

[EWS]

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