491579 - Make strings not require finalization

Status: RESOLVED WORKSFORME

(Core :: JavaScript Engine, defect)

Description

[Bruce Hoult]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10_5_6; en-us) AppleWebKit/528.16 (KHTML, like Gecko) Version/4.0 Safari/528.16
Build Identifier: 

Strings currently have an expensive finalizer. This is very bad in such a common object. If possible, the GC should simply be able to GC the string and anything related to it, in particular any ASCII version of the string that has been cached.

Reproducible: Always

Comment 1

[Brendan Eich [:brendan]]

What problem is being reported by this bug? That string chars are typically allocated with malloc (but not necessarily external strings)? Or that a deflated string needs to be purged from js_FinalizeStringRT if it has been computed and associated with string header's address?

Does "GC the string" mean GC-allocate and collect the deflated bytes or vector of jschars for a string? The GC currently has few size classes (10, from 8 to 80), a histogram of string lengths from a live Firefox gmail+other-tabs run would be helpful. Been a long time since anyone has used printJSStringStats.

Any profile data measuring the performance problem this bug reports?

/be

Comment 2

[Brendan Eich [:brendan]]

JS_NewString hands off API-client malloc'ed memory to the deflated string cache; this is old API, still used in many embeddings AFAIK (including Gecko). The primal API sin is JS_GetStringBytes(str), which is why the deflated string cache exists at all. It's also used all over.

/be

Comment 3

[Bruce Hoult]

I have no specific problem or stats at present and I don't yet understand the code. Sayrer asked me to make a bug before I started to investigate.

Comment 4

[Brendan Eich [:brendan]]

Ok -- there's a bug on making small strings allocate chars contiguously from GC'ed space after the string header: bug 402614. Hey, it has histograms! That bug is on Igor's list but he could give it to you.

/be

Comment 5

[Brendan Eich [:brendan]]

Still not sure which costs are being targeted here, but we can find out. The API breaking costs are bigger because scaled across more embedders, but with C++ we could make sweeter API even than a call-once/zero-cleanup JS_GetStringBytes.

So maybe there are two bugs here, one about better API to replace (some day; have to deprecate first then obsolete; could happen in a single release cycle with some betas, if our embedders would just catch up to trunk) JS_GetStringBytes; and the other about GC'ing the chars vector.

/be

Comment 6

[Bruce Hoult]

Attachment: prelim: allow JSString to not be 8 bytes

There turn out to be some places in the existing code that assume that JSString and JSGCThing are the same size, and also that all objects are a multiple of JSGCThing size.

This patch simply adds an unused field to JSString and then fixes the problems that are exposed.  Firefox seems to run just fine with this patch applied.

This patch has some debugging aid and other temporary code in it.

Comment 7

[Bruce Hoult]

Attachment: prelim: point to deflated strings directly from the string

This builds on the other patch, using the added field to replace the deflatedStringCache.

It compiles but crashes before startup is complete.

Suspects:
- may not be handling free() properly
- may need an explicit length of the deflated string
- may not have removed the recent "I am deflated" bit in the length cleanly enough.

Current plan:
- debug it
- compare memory use and speed vs current system

Comment 8

[Brendan Eich [:brendan]]

Yes, the GC assumes everything is a multiple of 8 bytes (the 32-bit targets).

What fraction of all strings are deflated? Over what fraction (mean/variance) of their lifetimes?

If deflation is sparse then why increase JSString size? Fixing this bug could be purely deoptimizing (on average).

/be

Comment 9

[Brendan Eich [:brendan]]

Suspect bug 402614 is going to yield real benchmark and memory profile wins.

Also, bug 501186 is a fat target. Slab allocation for pre-initialized JSObjects by shape, better locality off-page of gc-thing flags, thereby avoiding that expensive divide, look like winners.

It still is not clear to me how this bug as stated would be fixed without actually regressing performance. Measurements showing deflated string frequence and lifetime still more than welcome -- measurements are necessary in order to make progress.

/be

Comment 10

[David Mandelin [:dmandelin]]

Oops, I could have cleared a couple of things up if I had seen this sooner. I suspect this bug is partially a follow-on of bug 487546, in which it was found string finalizers could be incredibly and unnecessarily expensive. Also, it seems to be commonly believed that it is best to avoid finalizers. So this bug seems to represent an attempt to avoid a certain finalizer entirely, and a first pass at eliminating finalizers as part of a general plan to do more in the GC and less in finalizers.

Given that in the browser, 99% or so of strings don't have a cache entry, it may not be a win to put the pointer in JSString and GC it. For practical purposes, it is probably better to deprecate the deflated string API. In particular, it should be possible to support the basic functionality (returning a C string) without the deflated string cache by altering the lifetime guarantee. This simplest option is to return a malloc'd string and make the caller responsible for cleaning it up.

Comment 11

[Brendan Eich [:brendan]]

JS_GetStringBytes is ~14 years old. Don't change it or you will create leaks a plenty, in the engine, in Firefox, and in many other embeddings.

The way to deprecate cleanly is to provide a better API and then remove the old one at some point.

A new API, JS_DecodeBytes, was added as part of the JS_CStringsAreUTF8 work. But it is frankly a pain to use, and I doubt many people will switch to it from JS_GetStringBytes.

A JS_DeflateString API could be added that always mallocs, and obligates the caller to JS_free (or just free; we don't require balance; some argue we should but without C++ helpers or a stronger type system, historically this is asking for trouble), could be added.

/be

Comment 12

[Brendan Eich [:brendan]]

I had two verbs in that last sentence, sorry.

Thinking about JS_GetStringBytes more, if the "lifetime tied to the JSString whence the bytes came" guarantee is too strong, could we keep the deflated bytes until next GC? In general, JS_GetStringBytes users do not expect the bytes to live as long as the JSString they pass in, at a glance.

Then the deflated string cache would become a deflated string pool, to be reset by the GC -- it might not need to be keyed by JSString address. It might be ok to index by the string chars (decimated to bytes, or not). It might even be ok to create a new deflated string in the pool for every JS_GetStringBytes call.

/be

Comment 13

[Brendan Eich [:brendan]]

A C++ RAII-pattern API that managed the copy like an auto_ptr does, freeing the byte vector in its destructor -- seems like the carrot to use.

New bug, or morph this bug. If the root problem is the ancient JS_GetStringBytes API, then the fix must include better API, not (only, or even) a GC-level change (and not one that increases overhead for the rare case of deflating a JSString), yet which leaves the current API unchanged. So I say morph if there's no better use for this bug.

/be

Comment 14

[Andreas Gal :gal]

I would like to look into the API change post FF4.

Comment 15

[Luke Wagner [:luke]]

String finalization has been optimized in the interim.