488414 - TM: investigate if it is sound to call js_GenerateShape from the trace

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Igor Bukanov]

Currently record_SetPropHit adds to the trace code to call js_AddProperty. That in turn may call js_AddScopeProperty which in turns calls js_GenerateShape. When shape overflows, the latter calls js_GC().

Is it expected?

Comment 1

[Brendan Eich [:brendan]]

Looks like a js_LeaveTrace is needed, at the least.

/be

Comment 2

[Igor Bukanov]

I suspect a good solution to this and other cache bugs would be to change js_GenerateShape not to call js_GC. Rather on shape overflow it should just disable the cache and schedule the GC to happen at a later point.

Comment 3

[Andreas Gal :gal]

Along the same lines, we shouldn't renumber shapes at every GC. We should use a separate heuristics for that.

Comment 4

[Igor Bukanov]

I am going to implement that idea of just scheduling, not running, the GC from js_GenerateShape. It still would mean that the trace that calls js_AddProperty must guard that shape has not overflow, but that is simple.

Comment 5

[Andreas Gal :gal]

If we want this for 3.5 it has to be P1 and blocking b4 IMO. This is going to allow GC-ing during trace execution. Lots of potential fallout. Brendan thinks it might be possible to hack this short-term though.

Comment 6

[Robert Sayre]

P1 for now

Comment 7

[Igor Bukanov]

To Robert Sayre:

I have a patch that I can finish today, but it depends on 487039. If that sounds OK, I would suggest to make that bug P1 as well.

Comment 8

[Mike Beltzner [:beltzner, not reading bugmail]]

Igor: if that's the case, please re-nominated bug 487039 with a reason in that bug.

Comment 9

[Igor Bukanov]

Attachment: v1 - work in progress

The patch implements that idea of just scheduling, not calling the GC on the shape overflow. 

The most unclear part is js_AddProperty built-in changes. Currently the function returns false both to indicate OOM error and simple property mismatch. The jited code guards towards the OOM_EXIT on false. Assuming that this is the right thing to do, the patch also make function to return false on shape overflow.

Comment 10

[Igor Bukanov]

(In reply to comment #9)
> Created an attachment (id=373478) [details]
> v1 - work in progress

This patch is build on top of the patch from bug 487039 comment 19.

Comment 11

[Igor Bukanov]

Attachment: v2 - work in progress

The new version fixes the nested GC lock that v1 has and makes sure that single-threaded build compiles and avoids useless gcLocked flag in function signatures.

Comment 12

[Igor Bukanov]

Attachment: v3

Asking for comments on js_AddProperty built-in changes - is just returning false on shape overflow is the right thing there?

Comment 13

[Andreas Gal :gal]

Comment on attachment 373567 [details] [diff] [review]
v3

I like it. The "Updated by js_GenerateShape" is unclear to me. Could you extend that comment? Brendan should look at this too. Shouldn't we use ATOMIC_INC from PR for concurrent updates to shapeGen?

Comment 14

[Igor Bukanov]

Attachment: v4

The new patch updates comments to make it clear that, compared with js_GenerateShape, js_RegenerateShapeForGC does not need to do atomic increments.

It also fixed a bug where gcIsNeeded would be cleared by SET_SLOT pseudo-GC runs.

Comment 15

[Brendan Eich [:brendan]]

Comment on attachment 373577 [details] [diff] [review]
v4

>+#ifdef JS_THREADSAFE
>+# define js_TriggerAllOperationCallbacks(rt, gcLocked) \
>+    js_TriggerAllOperationCallbacks (rt, gcLocked)
>+#else
>+# define js_TriggerAllOperationCallbacks(rt, gcLocked) \
>+    js_TriggerAllOperationCallbacks (rt)
>+#endif

The space hack is a little too cute, but why use it at all #ifdef JS_THREADSAFE? Rather, define the macro only for !JS_THREADSAFE.

>+js_IsDisabledPropertyCache(JSContext *cx)

Rename to js_IsPropertyCacheDisabled.

>+#ifdef JS_THREADSAFE
>+# define js_GenerateShape(cx, gcLocked)    js_GenerateShape (cx, gcLocked)
>+#else
>+# define js_GenerateShape(cx, gcLocked)    js_GenerateShape (cx)
>+#endif

Same comment as above.

>@@ -3445,29 +3462,31 @@ js_GC(JSContext *cx, JSGCInvocationKind 
> 
>         /*
>          * We assume here that killing links to parent and prototype objects
>          * does not create garbage (such objects typically are long-lived and
>          * widely shared, e.g. global objects, Function.prototype, etc.). We
>          * collect garbage only if a racing thread attempted GC and is waiting
>          * for us to finish (gcLevel > 1) or if someone already poked us.
>          */
>-        if (rt->gcLevel == 1 && !rt->gcPoke)
>+        if (rt->gcLevel == 1 && !rt->gcPoke &&! rt->gcIsNeeded)

&&! => && !

>             goto done_running;
> 
>         rt->gcLevel = 0;
>         rt->gcPoke = JS_FALSE;
>         rt->gcRunning = JS_FALSE;
> #ifdef JS_THREADSAFE
>         rt->gcThread = NULL;
> #endif
>         gckind = GC_LOCK_HELD;
>         goto restart_at_beginning;
>     }
> 
>+    /* Clear gcIsNeeded now, when we are about to start a normal GC cycle. */
>+    rt->gcIsNeeded = JS_FALSE;
>     JS_UNLOCK_GC(rt);
> 
> #ifdef JS_TRACER
>     if (JS_ON_TRACE(cx))
>         goto out;
> #endif

Is the gcIsNeeded = false premature in light of the JS_TRACER code?

>@@ -124,18 +121,17 @@ js_FillPropertyCache(JSContext *cx, JSOb
>     jsuword vword;
>     ptrdiff_t pcoff;
>     jsuword khash;
>     JSAtom *atom;
>     JSPropCacheEntry *entry;
> 
>     JS_ASSERT(!cx->runtime->gcRunning);
>     cache = &JS_PROPERTY_CACHE(cx);
>-    pc = cx->fp->regs->pc;
>-    if (cache->disabled || (cx->fp->flags & JSFRAME_EVAL)) {
>+    if (js_IsDisabledPropertyCache(cx) || (cx->fp->flags & JSFRAME_EVAL)) {

Separate bug: turning off the cache from eval is no longer necessary, given the scripsToGC-based garbage collection of eval-compiled scripts, your fix to avoid caching non-global natives at the end of the scope chain (subsuming with statement disabling), and other fixes. Or so it seems to me! Could you file if you concur? Thanks.

>+                        if (js_IsDisabledPropertyCache(cx)) {
>+                            /*
>+                             * js_GenerateShape could not recover from shape's

s/shape's/rt->shapeGen's/

>+  sprop_changed:
>+  oom_exit:
>     JS_UNLOCK_SCOPE(cx, scope);
>     return JS_FALSE;

Looks good -- could use do-while(0) with break to avoid gotos, not sure it's any simpler.

js_AddProperty is BOOL_RETRY (please correct me if I'm wrong) so failing causes the triggered trace to exit back to the interpreter, to retry the add.

>+#ifdef JS_THREADSAFE
>+# define js_TriggerGC(cx, gcLocked)    js_TriggerGC (cx, gcLocked)
>+#else
>+# define js_TriggerGC(cx, gcLocked)    js_TriggerGC (cx)
>+#endif

Ditto early comment on space hack confinement to !JS_THREADSAFE.

Great to have the deferred trigger machinery -- I needed it during Firefox 3 propcache dev era, should have done it then for shape gen at least (sans operation callback changes for Fx3.5).

/be

Comment 16

[Igor Bukanov]

Attachment: v5

addressing all the issues from the comment 15

In js_AddProperty built-in I replaced all those different goto labels by single exit_trace. The build it is bool_retry indeed. An alternative version using do-while(0) (or rather for(;;) { ... return true; }) was uglier IMO due to excessive indentation.

Comment 17

[Mike Beltzner [:beltzner, not reading bugmail]]

Pushed to tracemonkey>

Comment 18

[Igor Bukanov]

Attachment: v6

The new patch adds missing JS_TREADSAFE ifdef around JS_ASSERT(cx->requestDepth > 0) in js_TriggerGC.

Comment 19

[Igor Bukanov]

https://hg.mozilla.org/tracemonkey/rev/7523cef76351

Comment 20

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/7523cef76351

Comment 21

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/3c718b1c7b03

Comment 22

[Boris Zbarsky [:bzbarsky]]

This failed to build with liveconnect enabled.  See http://hg.mozilla.org/releases/mozilla-1.9.1/rev/81e683c58238 and http://hg.mozilla.org/releases/mozilla-1.9.1/rev/9244eedd7d25

Comment 23

[Brendan Eich [:brendan]]

The type should be uint32 not jsuint.

/be

Comment 24

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/5b61f163f2fd