Last Comment Bug 737365 - stop using the cx during finalization
: stop using the cx during finalization
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: mozilla14
Assigned To: Igor Bukanov
:
Mentors:
Depends on:
Blocks: 741115 729760 737364
  Show dependency treegraph
 
Reported: 2012-03-20 03:18 PDT by Igor Bukanov
Modified: 2012-05-08 08:42 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
part 1 - changing the finalizer signature (147.23 KB, patch)
2012-03-20 04:10 PDT, Igor Bukanov
wmccloskey: review+
Details | Diff | Review
part 2 - stop using the cx in dom workers during finalization. (28.43 KB, patch)
2012-03-20 04:16 PDT, Igor Bukanov
no flags Details | Diff | Review
part 3 - eliminating remaining uses of cx during finalization. (97.85 KB, patch)
2012-03-20 04:18 PDT, Igor Bukanov
no flags Details | Diff | Review
part 1 - changing the finalizer signature (147.62 KB, patch)
2012-03-21 06:00 PDT, Igor Bukanov
wmccloskey: review+
bent.mozilla: review+
Details | Diff | Review
interdiff for part1 changes (29.48 KB, patch)
2012-03-21 06:02 PDT, Igor Bukanov
no flags Details | Diff | Review
part 2 - stop using the cx in dom workers during finalization. (33.86 KB, patch)
2012-03-23 15:33 PDT, Igor Bukanov
wmccloskey: review+
bent.mozilla: review+
Details | Diff | Review
part 3 - eliminating remaining uses of cx during finalization. (94.96 KB, patch)
2012-03-23 15:37 PDT, Igor Bukanov
no flags Details | Diff | Review
part 3 - eliminating remaining uses of cx during finalization. (94.70 KB, patch)
2012-03-25 13:07 PDT, Igor Bukanov
wmccloskey: review+
jorendorff: feedback+
Details | Diff | Review

Description Igor Bukanov 2012-03-20 03:18:07 PDT
+++ 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 2012-03-20 04:10:08 PDT
Created attachment 607505 [details] [diff] [review]
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 2012-03-20 04:16:43 PDT
Created attachment 607506 [details] [diff] [review]
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 2012-03-20 04:18:44 PDT
Created attachment 607507 [details] [diff] [review]
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 (:billm) 2012-03-20 10:54:37 PDT
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 2012-03-20 12:48:12 PDT
(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 2012-03-21 06:00:14 PDT
Created attachment 607922 [details] [diff] [review]
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 2012-03-21 06:02:27 PDT
Created attachment 607923 [details] [diff] [review]
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 2012-03-23 15:33:14 PDT
Created attachment 608890 [details] [diff] [review]
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 2012-03-23 15:34:15 PDT
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 2012-03-23 15:37:10 PDT
Created attachment 608892 [details] [diff] [review]
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 2012-03-25 13:07:45 PDT
Created attachment 609149 [details] [diff] [review]
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!) 2012-03-26 15:37:53 PDT
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!) 2012-03-26 15:43:57 PDT
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 (:billm) 2012-03-30 20:46:46 PDT
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 2012-03-31 04:32:24 PDT
(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 (:billm) 2012-03-31 08:57:35 PDT
(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 2012-04-03 02:37:42 PDT
feedback ping - Jason, could you comment on the comment 14?
Comment 18 Jason Orendorff [:jorendorff] 2012-04-03 07:58:21 PDT
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 20 Bill McCloskey (:billm) 2012-04-04 08:54:25 PDT
This seems to break non-threadsafe shell builds.
Comment 21 Igor Bukanov 2012-04-04 09:10:21 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/06b921a803ac - follow up to fix !JS_THREADSAFE
Comment 23 :Ehsan Akhgari (out sick) 2012-04-05 10:51:06 PDT
Followup: https://hg.mozilla.org/mozilla-central/rev/06b921a803ac
Comment 24 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-04-08 04:33:59 PDT
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 2012-04-08 04:57:05 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.