Closed Bug 908301 Opened 6 years ago Closed 6 years ago

Allow saving source when parsing scripts off thread


(Core :: JavaScript Engine, defect)

Not set





(Reporter: bhackett, Assigned: bhackett)




(1 file, 2 obsolete files)

Attached patch patch (obsolete) — Splinter Review
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.
Attachment #794144 - Flags: review?(benjamin)
Comment on attachment 794144 [details] [diff] [review]

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.
Attachment #794144 - Flags: review?(benjamin)
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.
I would support ripping about the source compressor. :)
Attached patch patch (obsolete) — Splinter Review
OK, this patch removes the dedicated source compressor thread and uses the jsworkers machinery instead.
Attachment #794144 - Attachment is obsolete: true
Attachment #794646 - Flags: review?(benjamin)
Attached patch patchSplinter Review
Oops, wrong patch.
Attachment #794646 - Attachment is obsolete: true
Attachment #794646 - Flags: review?(benjamin)
Attachment #794647 - Flags: review?(benjamin)
Comment on attachment 794647 [details] [diff] [review]

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.
Attachment #794647 - Flags: review?(benjamin) → review+
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Assignee: general → bhackett1024
Blocks: 912168
Depends on: 916541
You need to log in before you can comment on or make changes to this bug.