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

RESOLVED FIXED in mozilla30

Status

()

Core
JavaScript Engine
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla30
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

4 years ago
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.
(Assignee)

Comment 1

4 years ago
Created attachment 8375922 [details] [diff] [review]
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.
Attachment #8375922 - Flags: review?(benjamin)

Comment 2

4 years ago
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.
Attachment #8375922 - Flags: review?(benjamin) → review+
(Assignee)

Comment 3

4 years ago
>> 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.
https://hg.mozilla.org/mozilla-central/rev/27777c768b68
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
(Assignee)

Comment 6

4 years ago
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.
You need to log in before you can comment on or make changes to this bug.