983486 - Collect parallel arenas between iterations if data cannot escape

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Niko Matsakis [:nmatsakis]]

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.

Comment 1

[Niko Matsakis [:nmatsakis]]

I got the bug number wrong of course, it is bug 966567.

Comment 2

[Niko Matsakis [: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.

Comment 3

[Niko Matsakis [:nmatsakis]]

Attachment: Bug983486.diff

Cleaned up version of earlier patch, with feedback applied.

Comment 4

[Niko Matsakis [:nmatsakis]]

Try run: https://tbpl.mozilla.org/?tree=Try&rev=e453e32af4c1

Comment 5

[Niko Matsakis [:nmatsakis]]

Attachment: Bug983486.diff

Comment 6

[Terrence Cole [: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.

Comment 7

[Niko Matsakis [:nmatsakis]]

Comment on attachment 8391935 [details] [diff] [review]
Bug983486.diff

Requesting fuzzing.

Comment 8

[Niko Matsakis [:nmatsakis]]

Attachment: Bug983486.diff

Actually, here is a revised patch with nits applied and fully rebased.

Comment 9

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 8392333 [details] [diff] [review]
Bug983486.diff

Nothing bad found after one overnight run, tested on a 64-bit Linux debug shell.

Comment 10

[Niko Matsakis [:nmatsakis]]

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

Comment 11

[Niko Matsakis [:nmatsakis]]

Comment on attachment 8392333 [details] [diff] [review]
Bug983486.diff

Cancelling fuzz request due to limited resources.

Comment 12

[Carsten Book [:Tomcat]]

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