attach shareable portion of JSScript to necko cache

REOPENED
Unassigned

Status

()

Core
JavaScript Engine
REOPENED
6 years ago
3 years ago

People

(Reporter: luke, Unassigned)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink:P2])

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
Created attachment 553958 [details] [diff] [review]
measurement patch

bug 665707 proposes adding the ability to attach the "compiled representations" of a resource to its necko cache entry.  The shareable portion of a JSScript would be a prime candidate for this (see bug 665707 comment 8 to 13).

To get a feeling for how much this could save, I instrumented a browser.  On each full GC, the patch maintains a hash set where the key is the tuple (script->filename, script->lineno, script->length, script->code[0..script->length]).  When a script is traced, its key is extracted and it is added to the set.  If there is already a script in the set with the same key, I naively assume the cache would hit (and we'd share the script) and add script->totalSize() to a running sum that is reset/printed on each GC.  Lastly, in view of bug 678037, I only count scripts which have been run once.  This is pretty rough, but I think it gives a nice estimate of how much we could potentially win:

                      # scripts                 totalSize sum
                    total / share / %       total / share  / %
Startup:             1504 /  63   / 4%      765KB / 16.0KB / 2%
news.google->14 tabs  29K / 18K   / 63%    16.8MB / 10.4MB / 62%
facebook->5 tabs       7K /  4K   / 64%     3.2MB /  2.0MB / 63%
yahoo->7 tabs         11K /  3K   / 26%     6.3MB /  1.5MB / 24%
wikipedia->20 tabs    10K /  8K   / 85%     6.1MB /  5.3MB / 86%

Here, "Site->N tabs" means "go to Site, then ctrl-click on N things".  news.google is interesting because all the tabs are different domains; all the other examples were from the same domain.  I should note that, since not all of JSScript::totalSize() can be shared (some of it needs to stay per-compartment) and, as discussed in bug 678037, most scripts are pretty short, only maybe 60% of the "share" column could actually be saved by caching.  Still, that's a lot.

Also, the compile time for these scripts is a major part of page load time.  On the subject of compile time, bug 288473 has already attempted a caching strategy.  A key difference is that bug 288473 caches XDR (which incurs an encoding penalty) and decodes a new script on each hit (so there is no memory savings).  This mostly results from the use of the existing cache interface instead of the fancy new hotness proposed in bug 665707.

Measurement patch attached.
Whiteboard: [MemShrink]
Whiteboard: [MemShrink] → [MemShrink:P1]

Comment 1

5 years ago
Luke, would be interested to test a version of 10 or nightly with this change implemented, theres a massive memory usage in facebook compartments when multiple share or like buttons are active across several tabs
(Reporter)

Comment 2

5 years ago
I'm afraid this isn't close to being implemented.  At the least, it needs support from Necko.
Blocks: 716323
Blocks: 690229
Whiteboard: [MemShrink:P1] → [MemShrink:P2]
Is this still relevant now that bug 679940 has been done?
(Reporter)

Comment 4

4 years ago
Probably not, I'll WONTFIX for now.
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → WONTFIX
In the current setup, do we just save memory by sharing, or also save compilation time?
Flags: needinfo?(luke)
> In the current setup, do we just save memory by sharing, or also save
> compilation time?

The former. Given the results in bug 813324 comment 19, I'm not convinced we should WONTFIX this. There's clearly some value in not only saving memory but also compilation time.

Having said that, other changes might make this moot to some extend. Lazy parsing of scripts will substantially decrease the time needed for parsing in general. Additionally, we might do things like clone scripts from other compartments if they have already been parsed in the current session. That would require at least the hashing part of this task, however.

Given that there's no clearly best path forward here, I think WONTFIXING is premature.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
(Reporter)

Comment 7

4 years ago
Another concern that has come up since filing this: with Till's patch, we always save memory (but never time; we lose a bit of time from the hashing).  If we start caching the bytecode for scripts that aren't being used then we might win some time but we potentially waste a ton of memory.  If we start thinking about saving bytecode to disk, Taras's general feedback on I/O perf indicates that we might end up going slower since we'd be performing more I/O.  That plus lazy parsing is what has made me lose interest in this bug.

OTOH, bug 883154 sounds promising (saves parse time and doesn't waste memory).
Flags: needinfo?(luke)
Depends on: 900255
(Assignee)

Updated

3 years ago
Assignee: general → nobody
You need to log in before you can comment on or make changes to this bug.