938390 - don't create a string to hold chars in ScriptSource::chars

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

If I understand correctly, we aren't really taking advantage of the JSString created in ScriptSource::chars anywhere.  We just hold it in a HashMap that gets flushed on every GC.  The JSString poses a problem because JSString::MAX_LENGTH limits the max script length to 2^28 chars which we actually hit in un-optimized un-minified Emscripten builds.  I think we should be able to store the malloc'd chars in the map directly and FreeOp::free_ them in purge().

Comment 1

[Luke Wagner [:luke]]

Attachment: fix-decompress-bug

Seems to be a simple enough fix.

Comment 2

[:Benjamin Peterson]

Comment on attachment 831875 [details] [diff] [review]
fix-decompress-bug

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

Hmm, I'm not sure why the source was ever stored in a string. Perhaps for the convenience of the debugger at some point. It is rather scary to have to such large source files that this is a problem!

::: js/src/jsscript.cpp
@@ +1049,5 @@
> +    if (!map_)
> +        return;
> +
> +    for (Map::Range r = map_->all(); !r.empty(); r.popFront())
> +        js_delete(const_cast<jschar*>(r.front().value));

Should probably be "jschar *" (space added).

@@ +1066,5 @@
>  #ifdef USE_ZLIB
>      if (compressed()) {
> +        if (const jschar *decompressed = cx->runtime()->sourceDataCache.lookup(this))
> +            return decompressed;
> +      

Trailing ws, oh noes!!!

@@ +1070,5 @@
> +      
> +        const size_t nbytes = sizeof(jschar) * (length_ + 1);
> +        jschar *decompressed = static_cast<jschar *>(js_malloc(nbytes));
> +        if (!decompressed)
> +            return nullptr;

This is not your problem, but I suspect we need a JS_ReportOutOfMemory(cx) here.

Comment 3

[:Benjamin Peterson]

Now that I'm looking at this again, I see the memory ownership behavior of ScriptSource::chars() is insane. It basically gives you a pointer that is valid only until the next full gc, which means you might not even be able to do something useful with it before it gets freed under you. I don't actually see how you could write safe code that uses it. I don't think your patch makes this worse, but we should definitely make chars() less terrible as an API.

Comment 4

[Luke Wagner [:luke]]

Oh good point, I hadn't been considering the context of its usage!  So if this is for lazy parsing, is there anything that inhibits GC?

Comment 5

[Luke Wagner [:luke]]

We might want some simple flag on the sourceDataCache to prevent GC

Comment 6

[Luke Wagner [:luke]]

Err, s/GC/purging during GC/

Comment 7

[Luke Wagner [:luke]]

Oh, hah, all the significant callers have an AutoSupressGC that mentions preserving the chars.  Safe for now, but we should clean this up by having SourceDataCache::lookup/put take a PurgeGuard (is what I was calling it my half-done patch) that increments a counter in SourceDataCache that inhibits purging when non-zero.

Comment 8

[Luke Wagner [:luke]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6d579cd22053

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6d579cd22053