Collect parallel arenas between iterations if data cannot escape

RESOLVED FIXED in mozilla31

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: nmatsakis, Assigned: nmatsakis)

Tracking

unspecified
mozilla31
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

During parallel execution the only way that objects can escape a parallel callback is through the return value. Therefore, if the return value is known to have scalar type, no objects can escape, and hence it is reasonable to just completely free all objects in the parallel arenas in between iterations.

Given that requesting a GC causes bailouts *and* our allocation routines are not especially parallelized, this turns out to be crucial for performance for applications where the parallel kernel does a lot of allocation.

An example is the test case in bug 977567.
Blocks: 966567
I got the bug number wrong of course, it is bug 966567.
Assignee: nobody → nmatsakis
Note: a preliminary patch was posted in bug 966567 but I thought it'd be better to just create a separate bug for this work, since there are still other improvements needed for that bug.
Posted patch Bug983486.diff (obsolete) — Splinter Review
Cleaned up version of earlier patch, with feedback applied.
Posted patch Bug983486.diff (obsolete) — Splinter Review
Attachment #8390932 - Attachment is obsolete: true
Attachment #8391935 - Flags: review?(terrence)
Comment on attachment 8391935 [details] [diff] [review]
Bug983486.diff

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

Great work! The GC's heap structures and management are a nightmare to understand, much less tweak; this seems pretty reasonable. r=me

::: js/src/jsgc.cpp
@@ +545,5 @@
>       */
>  
> +    // during parallel sections, we sometimes finalize the parallel arenas,
> +    // but in that case, we want to hold on to the memory in our arena
> +    // lists, not offer it up for reuse

Start with a capital and end with a period. Also, be sure to change the comment style of the entire method if you want to use a differnet style comment here.

::: js/src/vm/ForkJoin.cpp
@@ +1237,5 @@
>  
>      bailouts += 1;
>      determineBailoutCause();
>  
> +    fprintf(stderr, "recoverFromBailout: %d\n", bailoutCause);

I guess you probably want to remove this from the final patch or turn it into SPEW.
Attachment #8391935 - Flags: review?(terrence) → review+
Comment on attachment 8391935 [details] [diff] [review]
Bug983486.diff

Requesting fuzzing.
Attachment #8391935 - Flags: feedback?(gary)
Attachment #8391935 - Flags: feedback?(choller)
Actually, here is a revised patch with nits applied and fully rebased.
Attachment #8391935 - Attachment is obsolete: true
Attachment #8391935 - Flags: feedback?(gary)
Attachment #8391935 - Flags: feedback?(choller)
Attachment #8392333 - Flags: review+
Attachment #8392333 - Flags: feedback?(gary)
Attachment #8392333 - Flags: feedback?(choller)
Depends on: 984676
Comment on attachment 8392333 [details] [diff] [review]
Bug983486.diff

Nothing bad found after one overnight run, tested on a 64-bit Linux debug shell.
Attachment #8392333 - Flags: feedback?(gary) → feedback+
Comment on attachment 8392333 [details] [diff] [review]
Bug983486.diff

Cancelling fuzz request due to limited resources.
Attachment #8392333 - Flags: feedback?(choller)
https://hg.mozilla.org/mozilla-central/rev/c02a4b7778cb
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.