737365 - stop using the cx during finalization

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

+++ This bug was initially created as a clone of Bug #737364 +++

This is a sub-bug for the bug 737364 with the focus on removing the cx usage during finalization/sweeping. In addition this bug is useful for the incremental/background sweeping implementation in the bug 729760. For that bug it is necessary to pass an extra parameters to the finalizers about the way they are invoked and that requires to change the finalizer signatures.

Comment 1

[Igor Bukanov]

Attachment: part 1 - changing the finalizer signature

The first part replaces the JSContext argument of the finalizers with the JSFreeOp/FreeOp structs. I named this struct FreeOp and not, for example, FinalizeOp, as the intent here is to deal with free/delete. Also I use structure on the part 3 to deal with code that is shared between the finalizers and non-finalizers.

Comment 2

[Igor Bukanov]

Attachment: part 2 - stop using the cx in dom workers during finalization.

In part 1 the cx is still passed to the finalizers indirectly via FreeOp structure so the callers can access it if necessary. This part removes that cx usage in dom workers.

This patch is untested.

Comment 3

[Igor Bukanov]

Attachment: part 3 - eliminating remaining uses of cx during finalization.

This is completely untested patch and it is here only for references.

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 607505 [details] [diff] [review]
part 1 - changing the finalizer signature

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

Great patch. I've wanted to do this for a while, but I always got bogged down part way through. I really like the new FreeOp structure.

One major request: I don't really like that FreeOp is passed around const. The only other place we use const a lot is with Shapes, and I think that was a mistake. There are const_casts on shapes everywhere. Is there any particular reason you used const?

::: js/ipc/ObjectWrapperChild.cpp
@@ +410,5 @@
>  static const PRUint32 sNextIdIndexSlot = 0;
>  static const PRUint32 sNumNewEnumerateStateSlots = 1;
>  
>  static void
> +CPOW_NewEnumerateState_freeIds(JSObject* state)

It seems like CPOW_NewEnumerate_FreeIds would be more consistent, maybe.

::: js/src/jsapi.cpp
@@ +2232,5 @@
>      return cx->free_(p);
>  }
>  
>  JS_PUBLIC_API(void)
> +JS_Free(const JSFreeOp *fop, void *p)

This is likely to cause some confusion :-). I guess it's okay as long as we deprecate JS_free pretty soon, which seems likely.

::: js/src/jsfriendapi.h
@@ +306,5 @@
>  
>  extern JS_FRIEND_API(bool)
>  GCThingIsMarkedGray(void *thing);
>  
> +class FreeOp : public JSFreeOp {

Is it necessary to expose this in jsfriendapi.h? Maybe I missed something, but it looks like the only uses of these definitions outside the JS engine are to fetch the context in a few places. Could we just expose the declaration of js::FreeOp but not the definition? And then provide a jsfriendapi function to fetch the context from a js::FreeOp?

@@ +309,5 @@
>  
> +class FreeOp : public JSFreeOp {
> +  public:
> +    bool        shouldFreeLater;
> +    bool        backgroundThread;

Could you rename this to something like onBackgroundThread so it's clear that it's a bool and not a thread?

::: js/xpconnect/idl/nsIXPCScriptable.idl
@@ +160,5 @@
>                     in JSContextPtr cx, in JSObjectPtr obj,
>                     in PRUint32 type, in JSValPtr vp);
>  
>      void   finalize(in nsIXPConnectWrappedNative wrapper,
> +                    in JSFreeOpPtr cx, in JSObjectPtr obj);

Probably should be called fop, rather than cx.

Comment 5

[Igor Bukanov]

(In reply to Bill McCloskey (:billm) from comment #4)
> One major request: I don't really like that FreeOp is passed around const.
> The only other place we use const a lot is with Shapes, and I think that was
> a mistake. There are const_casts on shapes everywhere. Is there any
> particular reason you used const?

I just wanted to make sure that the callee can not modify FreeOp. As JSFreeOp is declared as a C structure the only way to make JSFreeOp::runtime is immutable is via const. But you are right, the patch adds way too much those const declarations. So I will update it with const removed and field in FreeOp made private. I will also add #ifdef C++ to JSFreeOp definition so only for C code it will be a plain struct. C++ will have JSFreeOp::runtime().

> >  JS_PUBLIC_API(void)
> > +JS_Free(const JSFreeOp *fop, void *p)
> 
> This is likely to cause some confusion :-). I guess it's okay as long as we
> deprecate JS_free pretty soon, which seems likely.

That was my initial thinking. On the second thought, if JS_free(cx, p) disappears, it is rather likely that we may end up with JS_free(rt, p). So we better have a good name here from the beginning. Does JS_FreeOp(JSFreeOp *fop, void *p) look better? Or should it be JS_freeOp(fop, p)?


> Is it necessary to expose this in jsfriendapi.h? Maybe I missed something,
> but it looks like the only uses of these definitions outside the JS engine
> are to fetch the context in a few places. Could we just expose the
> declaration of js::FreeOp but not the definition? And then provide a
> jsfriendapi function to fetch the context from a js::FreeOp?

FreeOp::context is just a temporary solution to split the patch into separated parts that can be tested on its own. It will be removed in the part 3. But then to compile I indeed may well just add a friend access function to get the cx.

Comment 6

[Igor Bukanov]

Attachment: part 1 - changing the finalizer signature

The new version drops const and stops exposing FreeOp via jsfriendapi.h. Temporary I added JSContext * to JSFreeOp to allow to compile and test this part independently.

Comment 7

[Igor Bukanov]

Attachment: interdiff for part1 changes

For references here is the interdiff between a previous version of the patch with const removed and other changes.

Comment 8

[Igor Bukanov]

Attachment: part 2 - stop using the cx in dom workers during finalization.

This patch removes the usage of cx during finalization outside the JS engine. Currently that cx is extensively used in the dom/workers code where in many places finalizer share the code with explicit destruction.

To support that the patch adds a default JSFreeOp that allows to call JS_freeop() outside a finalizer. Also in dom/workers I replaced in few method signatures the current (JSContext *aCx, bool aFromFinilizer) arguments with (JSContext *aCx, JSFreeOp *aFinalizerFop) where exactly one of aCx and aFinalizerFop must be given.

Comment 9

[Igor Bukanov]

Comment on attachment 607922 [details] [diff] [review]
part 1 - changing the finalizer signature

Bent, could you also look at dom/workers changes here?

Comment 10

[Igor Bukanov]

Attachment: part 3 - eliminating remaining uses of cx during finalization.

Here is another backup of part 3 - it is no yet ready for a review.

Comment 11

[Igor Bukanov]

Attachment: part 3 - eliminating remaining uses of cx during finalization.

This part removes the usage of JSContext* during the finalization and
sweeping. That required to change quite a few methods in type inference,
jit and debugger implementation to take a FreeOp rather than JSContext
pointer. In turn that also often required to replace cx->compartment
usage with extracting the compartment from the passed objects or pass
the compartment explicitly. On the plus side it allowed to remove
fallible compartment enter code in methods that could be called during
finalization.

Comment 12

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 607922 [details] [diff] [review]
part 1 - changing the finalizer signature

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

r=me on worker things!

Comment 13

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 608890 [details] [diff] [review]
part 2 - stop using the cx in dom workers during finalization.

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

r=me on the worker changes!

Comment 14

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 609149 [details] [diff] [review]
part 3 - eliminating remaining uses of cx during finalization.

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

Thanks again, Igor. This looks great.

Jason, can you look over the debugger change that I've pointed out below?

::: js/src/jsinfer.cpp
@@ +2117,5 @@
>      }
>  }
>  
>  void
> +TypeCompartment::setPendingNukeTypesNoReport()

Is it possible to assert that this one is only called from GC, and that setPendingNukeTypes is never called from GC? Also, it would be nice to have a comment about when one or the other should be used (maybe in the header file).

::: js/src/vm/Debugger.cpp
@@ -292,5 @@
> -        if (!forTrap) {
> -            ac.construct(cx, ScriptGlobal(cx, script, scriptGlobal));
> -            if (!ac.ref().enter())
> -                return false;
> -        }

Removing this basically makes sense to me, since the two functions below it won't take a context. It still scares me a little, though. Why were we only entering the compartment if forTrap was true? Would we have been in the right compartment anyway if !forTrap?

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +250,5 @@
>      MOZ_COUNT_DTOR(xpc::CompartmentPrivate);
>  }
>  
> +static void
> +CompartmentCallback(JSFreeOp *fop, JSCompartment *compartment)

Could you call this CompartmentDestroyedCallback?

Comment 15

[Igor Bukanov]

(In reply to Bill McCloskey (:billm) from comment #14)
> >  void
> > +TypeCompartment::setPendingNukeTypesNoReport()
> 
> Is it possible to assert that this one is only called from GC, and that
> setPendingNukeTypes is never called from GC? Also, it would be nice to have
> a comment about when one or the other should be used (maybe in the header
> file).

I will try to do that, but do we want to report OOM if we can completely recover from it? I.e. in case the malloc fails during type inference we still AFAIK can compete the evaluation albeit slowly. Clearly, most likely we will fail later with OOM again, but then perhaps we should report OOM only if it changes the evaluation results.


> 
> ::: js/src/vm/Debugger.cpp
> @@ -292,5 @@
> > -        if (!forTrap) {
> > -            ac.construct(cx, ScriptGlobal(cx, script, scriptGlobal));
> > -            if (!ac.ref().enter())
> > -                return false;
> > -        }
> 
> Removing this basically makes sense to me, since the two functions below it
> won't take a context. It still scares me a little, though. Why were we only
> entering the compartment if forTrap was true? Would we have been in the
> right compartment anyway if !forTrap?

forTrap is false during the GC we could be in an arbitrary compartment when cx->compartment is a wrong one. So without the patch due to reliance of cx->compartment it was necessary to do the enter cal during the GC.

Comment 16

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Igor Bukanov from comment #15)
> > Is it possible to assert that this one is only called from GC, and that
> > setPendingNukeTypes is never called from GC? Also, it would be nice to have
> > a comment about when one or the other should be used (maybe in the header
> > file).
> 
> I will try to do that, but do we want to report OOM if we can completely
> recover from it? I.e. in case the malloc fails during type inference we
> still AFAIK can compete the evaluation albeit slowly. Clearly, most likely
> we will fail later with OOM again, but then perhaps we should report OOM
> only if it changes the evaluation results.

So you're saying we should use the NoReport version even when a GC is not running? I guess that makes sense, although I'd rather do that in a separate bug.

> > Removing this basically makes sense to me, since the two functions below it
> > won't take a context. It still scares me a little, though. Why were we only
> > entering the compartment if forTrap was true? Would we have been in the
> > right compartment anyway if !forTrap?
> 
> forTrap is false during the GC we could be in an arbitrary compartment when
> cx->compartment is a wrong one. So without the patch due to reliance of
> cx->compartment it was necessary to do the enter cal during the GC.

OK, maybe I'm just being paranoid.

Comment 17

[Igor Bukanov]

feedback ping - Jason, could you comment on the comment 14?

Comment 18

[Jason Orendorff [:jorendorff]]

Comment on attachment 609149 [details] [diff] [review]
part 3 - eliminating remaining uses of cx during finalization.

That code makes no sense to me. Go ahead and delete it.

Comment 19

[Igor Bukanov]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d5057ff02ffb
https://hg.mozilla.org/integration/mozilla-inbound/rev/74053b148a3c
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8c5316f513a

Comment 20

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

This seems to break non-threadsafe shell builds.

Comment 21

[Igor Bukanov]

https://hg.mozilla.org/integration/mozilla-inbound/rev/06b921a803ac - follow up to fix !JS_THREADSAFE

Comment 22

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/d5057ff02ffb
https://hg.mozilla.org/mozilla-central/rev/74053b148a3c
https://hg.mozilla.org/mozilla-central/rev/d8c5316f513a

Comment 23

[(no longer active)]

Followup: https://hg.mozilla.org/mozilla-central/rev/06b921a803ac

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

Is it possible that this could have affected to gc behavior?
I'm trying to figure out what has caused Bug 743178, and I *think*
it is something after http://hg.mozilla.org/mozilla-central/rev/9894cd999781

Comment 25

[Igor Bukanov]

(In reply to Olli Pettay [:smaug] from comment #24)
> Is it possible that this could have affected to gc behavior?
> I'm trying to figure out what has caused Bug 743178, and I *think*
> it is something after http://hg.mozilla.org/mozilla-central/rev/9894cd999781

This bug should not affect the GC as it just replaces the way the finalizers are called. I tried to avoid any changes to the substance in this relatively big refactoring patch delegating the real changes to the bug 737364.