972657 - Move the "is this script too small to compress?" check to a better place.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

In ScriptSource::setSourceCopy(), we skip compression if a script's length (in
chars) is larger than HUGE_SCRIPT.

In SourceCompressionTask::work(), we skip compression if a scripts size (in
bytes) is smaller than COMPRESS_THRESHOLD.

Moving the latter check into ScriptSource::setSourceCopy() would (a) put more
of the logic in a single place, and (b) avoid putting no-op tasks on the
compression worklists.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Move the "is this script too small to compress?" check to a better place.

Note that the old threshold was in bytes, and the new one is in jschars, which
explains the change from 512 to 256.

Comment 2

[:Benjamin Peterson]

Comment on attachment 8375922 [details] [diff] [review]
Move the "is this script too small to compress?" check to a better place.

Review of attachment 8375922 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsscript.cpp
@@ +1287,5 @@
>      //  - If the script is enormous, then decompression can take seconds. With
>      //    lazy parsing, decompression is not uncommon, so this can significantly
>      //    increase latency.
>      //  - If there is only one core, then compression will contend with JS
>      //    execution (which hurts benchmarketing).

I can't tell if this is a typo or a clever coinage.

Comment 3

[Nicholas Nethercote [inactive]]

>> benchmarketing).
>  
> I can't tell if this is a typo or a clever coinage.

Neither! It's an old phrase used to describe optimizations that mostly help benchmarks, not real code. E.g. daylight savings offset caches, things like that.

Comment 4

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/27777c768b68

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/27777c768b68

Comment 6

[Nicholas Nethercote [inactive]]

Talos liked this patch a lot:

> Improvement: Mozilla-Inbound-Non-PGO - Tab Animation Test - Ubuntu HW 12.04 x64 - 5.92% decrease
> Improvement: Mozilla-Inbound-Non-PGO - Tab Animation Test - WINNT 6.2 x64 - 4.41% decrease
> Improvement: Mozilla-Inbound-Non-PGO - Paint - WINNT 6.2 x64 - 10.1% decrease
> Improvement: Mozilla-Inbound-Non-PGO - Tab Animation Test - WINNT 5.1 (ix) - 3.36% decrease
> Improvement: Mozilla-Inbound-Non-PGO - Tab Animation Test - WINNT 6.1 (ix) - 3.54% decrease
> Improvement: Mozilla-Inbound-Non-PGO - Tab Animation Test - Ubuntu HW 12.04 - 3.26% decrease
> Improvement: Mozilla-Inbound-Non-PGO - Paint - WINNT 6.1 (ix) - 5.73% decrease

Which was surprising... I thought this patch would have miniscule performance effects, because it merely avoided a small amount of worklist processing.