504478 - Shrink slots during GC only, split ReallocSlots into Alloc/Grow/ShrinkSlots

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Comment 1

[Andreas Gal :gal]

Attachment: patch

Comment 2

[Andreas Gal :gal]

This patch postpones shrinking for ->dslots until the GC is marking objects. This allows eliminating some of the rather brittle oscillation avoidance heuristics in js_ReallocSlots. The patch also splits js_ReallocSlots into js_AllocSlots and js_GrowSlots, which do what their names suggest. js_ShrinkSlots is only used by the GC to shrink or free slots.

This patch is a 6ms speedup on SS.

Comment 3

[Andreas Gal :gal]

TEST                   COMPARISON            FROM                 TO             DETAILS

=============================================================================

** TOTAL **:           1.007x as fast    907.1ms +/- 0.1%   901.2ms +/- 0.1%     significant

=============================================================================

  3d:                  1.013x as fast    137.6ms +/- 0.4%   135.9ms +/- 0.3%     significant
    cube:              1.026x as fast     39.0ms +/- 1.1%    38.1ms +/- 0.8%     significant
    morph:             1.017x as fast     28.0ms +/- 0.4%    27.6ms +/- 0.5%     significant
    raytrace:          1.004x as fast     70.5ms +/- 0.2%    70.2ms +/- 0.2%     significant

  access:              1.002x as fast    130.4ms +/- 0.1%   130.0ms +/- 0.2%     significant
    binary-trees:      1.014x as fast     37.4ms +/- 0.4%    36.9ms +/- 0.3%     significant
    fannkuch:          *1.005x as slow*   55.1ms +/- 0.2%    55.4ms +/- 0.3%     significant
    nbody:             1.008x as fast     24.6ms +/- 0.6%    24.4ms +/- 0.6%     significant
    nsieve:            ??                 13.2ms +/- 0.9%    13.3ms +/- 1.0%     not conclusive: might be *1.008x as slow*

  bitops:              ??                 34.9ms +/- 0.6%    35.1ms +/- 0.6%     not conclusive: might be *1.006x as slow*
    3bit-bits-in-byte: -                   1.6ms +/- 9.0%     1.6ms +/- 9.1% 
    bits-in-byte:      -                   8.0ms +/- 0.5%     8.0ms +/- 0.5% 
    bitwise-and:       ??                  2.4ms +/- 5.8%     2.4ms +/- 5.9%     not conclusive: might be *1.025x as slow*
    nsieve-bits:       *1.008x as slow*   23.0ms +/- 0.3%    23.2ms +/- 0.5%     significant

  controlflow:         -                  34.2ms +/- 0.3%    34.1ms +/- 0.3% 
    recursive:         -                  34.2ms +/- 0.3%    34.1ms +/- 0.3% 

  crypto:              -                  52.4ms +/- 0.8%    52.1ms +/- 0.9% 
    aes:               -                  29.8ms +/- 1.2%    29.7ms +/- 1.4% 
    md5:               -                  14.3ms +/- 1.2%    14.2ms +/- 0.9% 
    sha1:              -                   8.2ms +/- 1.5%     8.2ms +/- 1.3% 

  date:                1.008x as fast    129.0ms +/- 0.2%   127.9ms +/- 0.2%     significant
    format-tofte:      1.015x as fast     60.7ms +/- 0.3%    59.8ms +/- 0.3%     significant
    format-xparb:      -                  68.3ms +/- 0.2%    68.1ms +/- 0.2% 

  math:                ??                 29.6ms +/- 0.6%    29.6ms +/- 0.8%     not conclusive: might be *1.001x as slow*
    cordic:            -                  10.6ms +/- 1.3%    10.5ms +/- 1.4% 
    partial-sums:      -                  12.9ms +/- 0.5%    12.9ms +/- 0.7% 
    spectral-norm:     *1.023x as slow*    6.0ms +/- 0.0%     6.1ms +/- 2.8%     significant

  regexp:              1.009x as fast     44.8ms +/- 0.4%    44.4ms +/- 0.3%     significant
    dna:               1.009x as fast     44.8ms +/- 0.4%    44.4ms +/- 0.3%     significant

  string:              1.007x as fast    314.4ms +/- 0.2%   312.2ms +/- 0.2%     significant
    base64:            -                  17.1ms +/- 0.5%    17.0ms +/- 0.2% 
    fasta:             *1.019x as slow*   62.8ms +/- 0.2%    64.0ms +/- 0.2%     significant
    tagcloud:          -                  96.1ms +/- 0.3%    96.0ms +/- 0.5% 
    unpack-code:       1.028x as fast    109.2ms +/- 0.1%   106.2ms +/- 0.2%     significant
    validate-input:    1.009x as fast     29.2ms +/- 0.5%    28.9ms +/- 0.4%     significant

Comment 4

[Brendan Eich [:brendan]]

Splitting cases so the "funnel" doesn't have to == WIN.

/be

Comment 5

[Igor Bukanov]

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

>diff --git a/js/src/jsgc.cpp b/js/src/jsgc.cpp
>--- a/js/src/jsgc.cpp
>+++ b/js/src/jsgc.cpp
>@@ -2723,16 +2723,27 @@ JS_CallTracer(JSTracer *trc, void *thing
>     JS_ASSERT(rt->gcMarkingTracer == trc);
>     JS_ASSERT(rt->gcLevel > 0);
> 
>     /*
>      * Optimize for string and double as their size is known and their tracing
>      * is not recursive.
>      */
>     switch (kind) {
>+      case JSTRACE_OBJECT: {
>+        JSObject* obj = (JSObject*)thing;
>+        if (OBJ_IS_NATIVE(obj)) {
>+            JSScope* scope = OBJ_SCOPE(obj);
>+            size_t slots = scope->freeslot;
>+            if (STOBJ_NSLOTS(obj) != slots)
>+                js_ShrinkSlots(cx, obj, slots);
>+        }
>+        break;
>+      }

The code must not shrink slots when object does not own the scope. Plus it should go to js_TraceObject. Here the code will be executed any time the object is reached through the object graph, not only when it is marked.

>+JSBool
>+js_AllocSlots(JSContext *cx, JSObject *obj, size_t nslots)

Nit: This is a new code. Use bool/true/false.

>+{
>+    JS_ASSERT(!obj->dslots);
>+
>+    /* If we are allocating fslots, there is nothing to do. */
>+    if (nslots <= JS_INITIAL_NSLOTS)
>+        return JS_TRUE;

Is this function ever called with nslots <= JS_INITIAL_NSLOTS? If not, just assert that nslots > JS_INITIAL_NSLOTS here.

Comment 6

[Andreas Gal :gal]

Attachment: patch v2

Can you briefly explain why the object must own its scope to shrink the slots?

Comment 7

[Brendan Eich [:brendan]]

(In reply to comment #6)
> Created an attachment (id=388884) [details]
> patch v2
> 
> Can you briefly explain why the object must own its scope to shrink the slots?

See InitScopeForObject and js_GetMutableScope. That scope->freeslot member does not tell you about the free slot for any object other than scope->object, meaning all the unmutated newborns.

/be

Comment 8

[Igor Bukanov]

Comment on attachment 388884 [details] [diff] [review]
patch v2

>@@ -5738,16 +5714,21 @@ js_TraceObject(JSTracer *trc, JSObject *
...
>     if (traceScope) {
>+        /* Check whether we should shrink the object's slots. */

This needs to be protected with IS_GC_MARKING_TRACER(trc) to shrink slots only during the real GC. r+ with that fixed.

Comment 9

[Andreas Gal :gal]

http://hg.mozilla.org/tracemonkey/rev/1440f40669a7

Comment 10

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/1440f40669a7