908301 - Allow saving source when parsing scripts off thread

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

Attachment: patch

Currently off thread parsing is only compatible with scripts that don't require source saving/compression.  This works for scripts in XUL documents but won't work for scripts loaded from HTML, and needs to be fixed.  The attached patch updates the source compressor thread so that it will work properly if compression is triggered from multiple threads, by avoiding races when updating |tok| and |stop| and allowing for the case when one thread triggers a new compression before another thread has completed the earlier token.

Comment 1

[:Benjamin Peterson]

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

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

A first pass reveals at least one issue in |abort()|.

Since the purpose is to allow off-the-main-thread parsing to source compression, wouldn't it make more sense to simply allow source compression to run in the parsing thread? An initial version could even just save the uncompressed source. It would also be bad if the main thread had to block on off-the-main-thread parsing's compressions jobs.

::: js/src/jsscript.cpp
@@ +1188,5 @@
>  {
> +    PR_Lock(lock);
> +    if (userTok == tok)
> +        stop = true;
> +    PR_Unlock(lock);

This isn't going to work, since the compression loop never releases the lock.

Comment 2

[Brian Hackett [Laid off!]]

I was initially just planning to do compression on the parse thread, but since the main motivation for off thread HTML script parsing is to deal with large games, and since zlib is so slow I think parse time would really suffer as a result.  Also because of the games, keeping source uncompressed seems bad.  The |stop| issue should be simple to fix, change the type to SourceCompressionToken* to indicate the token to stop on (away from computer right now).

Another option would be to rip out the source compressor thread and use the jsworkers threads for doing compression.  I want to do that eventually anyways, but doing it now would let multiple threads compress source at once and avoid contention issues.

Comment 3

[:Benjamin Peterson]

I would support ripping about the source compressor. :)

Comment 4

[Brian Hackett [Laid off!]]

Attachment: patch

OK, this patch removes the dedicated source compressor thread and uses the jsworkers machinery instead.

Comment 5

[Brian Hackett [Laid off!]]

Attachment: patch

Oops, wrong patch.

Comment 6

[:Benjamin Peterson]

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

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

Nice to see that extra code go away.

::: js/src/jsscript.h
@@ +1341,4 @@
>      ScriptSource *ss;
>      const jschar *chars;
>      bool oom;
> +    volatile size_t abort_;

This is size_t and not bool to get word alignment for volatile, right? Deserves a comment.

::: js/src/jsworkers.h
@@ +175,4 @@
>      ParseTask *parseTask;
>  
> +    /* Any source being compressed on this thread. */
> +    SourceCompressionToken *compressionToken;

Seeing ParseTask and AsmJSParallelTask makes me think SourceCompressionToken should be renamed to SourceCompressionTask and moved to jsworkers.h/jsworders.cpp.

Comment 7

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ca06d27f049f

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ca06d27f049f