1008032 - Clean up ScriptSource

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

Attachment: patch

As part of investigating whether to fix the lazy script cache so that it actually does something (bug 988353), I'd like to play around with modifying ScriptSources so that we can keep track of which ones are identical to each other.  This is kind of tricky though because ScriptSource is pretty messy and not really friendly to poking around in.  There are two main issues which the attached patch addresses.

- Several critical fields in the class are modified off thread during source compression.  This isn't really necessary though and the source compression thread can keep track of its state using SourceCompressionTask (a much simpler muultithreaded data structure).  Fixing this also has the benefit of making it explicit whether a ScriptSource owns its characters.

- Clean up some union field accesses where it's not obvious which field of the union is active at any given time.

Comment 1

[Luke Wagner [:luke]]

Comment on attachment 8419859 [details] [diff] [review]
patch

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

Nice!

::: js/src/jsscript.cpp
@@ +1597,5 @@
>      argumentsNotIncluded_ = argumentsNotIncluded;
>  
> +    setSource(srcBuf.ownsChars() ? srcBuf.take() : srcBuf.get(),
> +              srcBuf.length(),
> +              srcBuf.ownsChars());

If srcBuf.ownsChars(), then srcBuf.take() will change srcBuf.ownsChars_ to false, so the third argument will evaluate to false which I think is a leak (I guess argument evaluation order is unspecified, so it might not).

@@ +1694,4 @@
>  #endif
>  
> +    // Shrink the buffer to the size of the compressed data. Shouldn't fail.
> +    compressed = js_realloc(compressed, compressedBytes);

Prexisting, but technically, I don't think we're able to rely on shrinking-realloc never failing.

@@ +1699,4 @@
>  }
>  
>  void
>  ScriptSource::destroy()

Preexisting, but since you're cleaning up, can you change ScriptSource::destroy() to be ScriptSource::~ScriptSource() and have decref() just call js_delete(this) (don't forget to remove the js_free(this) at the end)?

@@ +1772,5 @@
> +                js_free(p);
> +                return false;
> +            }
> +            length_ = length;
> +            argumentsNotIncluded_ = argumentsNotIncluded;

Pre-existing, but this whole area is all scrambled up.  Since you're cleaning this, can you make it more canonical by:
 - length_ is already a uint32_t, so you should be able to just have xdr->codeUint32(&length_) w/o the local variable.
 - argumentsNotIncluded needs a local, but it could be hoisted up belown length and handled in the canonical way:

uint8_t argumentsNotIncluded;
if (mode == XDR_ENCODE)
    argumentsNotIncluded = argumentsNotIncluded_;
if (!xdr->codeUint8(&argumentsNotIncluded))
    return false;
if (mode == XDR_DECODE)
    argumentsNotIncluded_ = argumentsNotIncluded;

::: js/src/jsscript.h
@@ +388,5 @@
>  
> +    uint32_t refs;
> +
> +    // Note: while ScriptSources may be compressed off thread, they are only
> +    // modified by the main thread, and all members are always save to access

s/save/safe/

::: js/src/jsworkers.cpp
@@ -977,5 @@
>      return nullptr;
>  }
>  
> -const jschar *
> -ScriptSource::getOffThreadCompressionChars(ExclusiveContext *cx)

Cool.  Is the reason this can simply be removed that we already block at the end of compilation for the task to complete so there's no point in ScriptSource::chars doing any of this?

@@ +1085,5 @@
>      return true;
>  }
>  
>  const jschar *
>  ScriptSource::getOffThreadCompressionChars(ExclusiveContext *cx)

I think you need to delete this function?

Comment 2

[Brian Hackett [Laid off!]]

(In reply to Luke Wagner [:luke] (on partial PTO) from comment #1)
> ::: js/src/jsworkers.cpp
> @@ -977,5 @@
> >      return nullptr;
> >  }
> >  
> > -const jschar *
> > -ScriptSource::getOffThreadCompressionChars(ExclusiveContext *cx)
> 
> Cool.  Is the reason this can simply be removed that we already block at the
> end of compilation for the task to complete so there's no point in
> ScriptSource::chars doing any of this?

Previously, during off thread compression the compression task owned the uncompressed characters, so we had to go through all this to get them during the off thread compression.  Now, the script source always owns the uncompressed chars.  When the compression task finishes the uncompressed chars are replaced with the compressed ones, but since this happens on the main thread there isn't any need for ScriptSource::chars to worry about what the compression thread is doing.

Comment 3

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/99a6ee6466f5

Comment 4

[Hannes Verschore [:h4writer]]

This gives a 5% improvement on Windows 8 (browser). Interestingly no net change on the mac shell builds? I.e. 1.4% improvement on kraken-crypto-ccm, 1.7% improvement on kraken-crypto-pbkdf, 6.8% decrease on kraken-darkroom and 4.1% decrease on kraken-crypto-aes.

Since we have a net improvement on Win 8, it doesn't matter much. But maybe there is some easy fallout from this patch on the few decreases that would make this a net gain (on shells)?

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/99a6ee6466f5

Comment 6

[Nicholas Nethercote [inactive]]

> This gives a 5% improvement on Windows 8 (browser).

Was that expected? This reminds me of bug 972657 comment 6 where I saw some unexpected Talos wins from a tiny tweak to the processing of the compression worklist. I wonder if we have some significant inefficiencies in the source compression code.

Comment 7

[Brian Hackett [Laid off!]]

I don't think this patch should make a big difference, it doesn't change the point at which we allocate the various buffers involved or finish the off thread characters.  It does make it much easier to get the characters while off thread compression is in progress, which earlier required locking the worker thread state so the main thread could poke around at the worker threads.  That's gone now but a 5% improvement seems pretty big unless we were hitting pathological lock contention somehow (seems unlikely in kraken).

Comment 8

[Phil Ringnalda (:philor)]

https://tbpl.mozilla.org/php/getParsedLog.php?id=39420166&tree=Mozilla-Inbound - this also made the 32-bit Linux warnings-as-errors shell builds hit one of your MOZ_CRASHes.

Comment 9

[Hannes Verschore [:h4writer]]

I'm also seeing these MOZ_CRASH'es:

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0xf507eb40 (LWP 4556)]
js::SourceCompressionTask::work (this=this@entry=0xffffbf24) at /home/h4writer/Build/mozilla-inbound/js/src/jsscript.cpp:1692
1692        MOZ_CRASH();
(gdb) bt
#0  js::SourceCompressionTask::work (this=this@entry=0xffffbf24) at /home/h4writer/Build/mozilla-inbound/js/src/jsscript.cpp:1692
#1  0x083ef0ed in handleCompressionWorkload (this=0x872af00) at /home/h4writer/Build/mozilla-inbound/js/src/jsworkers.cpp:897
#2  js::WorkerThread::threadLoop (this=0x872af00) at /home/h4writer/Build/mozilla-inbound/js/src/jsworkers.cpp:1031
#3  0xf7f8d683 in ?? () from /usr/lib/i386-linux-gnu/libnspr4.so
#4  0xf7faad78 in start_thread () from /lib/i386-linux-gnu/libpthread.so.0
#5  0xf7d5afee in clone () from /lib/i386-linux-gnu/libc.so.6
(gdb)

Comment 10

[Douglas Crosher [:dougc]]

I'm seeing crashes too on ARM Linux shells.  The worker appears to be started even if USE_ZLIB is not defined.  A workaround here is to change the '#ifdef JS_THREADSAFE' in ScriptSource::setSourceCopy() to '#if defined(JS_THREADSAFE) && defined(USE_ZLIB)'. Not yet sure why it's not using a zlib, but that seems a separate configuration issue.

Comment 11

[Brian Hackett [Laid off!]]

Thanks, here's a push with that #if fix:

https://hg.mozilla.org/integration/mozilla-inbound/rev/5c9119729bbf

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5c9119729bbf