Last Comment Bug 720808 - Add nsJSEventListener and nsGlobalWindow to BBP
: Add nsJSEventListener and nsGlobalWindow to BBP
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: 12 Branch
: All All
: -- normal (vote)
: mozilla12
Assigned To: Olli Pettay [:smaug] (way behind * queues, especially ni? queue)
:
:
Mentors:
Depends on:
Blocks: 705582
  Show dependency treegraph
 
Reported: 2012-01-24 12:51 PST by Olli Pettay [:smaug] (way behind * queues, especially ni? queue)
Modified: 2012-01-26 13:19 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (11.12 KB, patch)
2012-01-24 12:51 PST, Olli Pettay [:smaug] (way behind * queues, especially ni? queue)
continuation: review+
Details | Diff | Splinter Review
patch (11.11 KB, patch)
2012-01-26 07:18 PST, Olli Pettay [:smaug] (way behind * queues, especially ni? queue)
no flags Details | Diff | Splinter Review

Description Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2012-01-24 12:51:16 PST
Created attachment 591230 [details] [diff] [review]
patch
Comment 1 Andrew McCreight [:mccr8] 2012-01-25 12:35:58 PST
Comment on attachment 591230 [details] [diff] [review]
patch

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

::: content/base/src/nsCCUncollectableMarker.h
@@ +59,4 @@
>    static bool InGeneration(nsCycleCollectionTraversalCallback &cb,
> +                           PRUint32 aGeneration)
> +  {
> +    return !cb.WantAllTraces() && InGeneration(aGeneration);

This is kind of a silly thing, but it would probably be better to put WantAllTraces second, because "!cb.WantAllTraces()" is always true in cases where we care about speed, so we should do the check that can fail first.  Not a huge deal, I'm sure, but this particular function likely gets called a lot.

::: dom/base/nsGlobalWindow.cpp
@@ +1418,5 @@
> +  }
> +NS_IMPL_CYCLE_COLLECTION_CAN_SKIP_END
> +
> +NS_IMPL_CYCLE_COLLECTION_CAN_SKIP_IN_CC_BEGIN(nsGlobalWindow)
> +  return tmp->IsBlackForCC();

This is totally fine for now, but I don't see any particular reason we can't do all the UnmarkGraying here, too.  It will cause additional work up front in the CC, but if we know all the objects it ungrays are going to be added to the CC graph anyways, it is probably going to reduce the time it takes overall.  Something to think about for a followup patch.

@@ +1548,5 @@
> +void
> +nsGlobalWindow::UnmarkGrayTimers()
> +{
> +  for (nsTimeout* timeout = FirstTimeout();
> +       timeout && IsTimeout(timeout);

So the timeout list stops being timeouts once there's any non-timeouts?

::: dom/base/nsJSEnvironment.h
@@ +197,5 @@
>  
> +  nsIScriptGlobalObject* GetCachedGlobalObject()
> +  {
> +    // Verify that we have a global so that this
> +    // does never return non-null value when GetGlobalObject() would.

The second line of the comment is a little unclear.  Maybe you mean "always returns a null when GetGlobalObject() is null"?

::: dom/src/events/nsJSEventListener.cpp
@@ +158,5 @@
> +      (!mScopeObject || !xpc_IsGrayGCThing(mScopeObject)) &&
> +      (!mHandler || !xpc_IsGrayGCThing(mHandler))) {
> +    nsIScriptGlobalObject* sgo =
> +      static_cast<nsJSContext*>(mContext.get())->GetCachedGlobalObject();
> +    return sgo && sgo->IsBlackForCC();

Why is the event listener not black when sgo is null?  Does that indicate the event listener is in some kind of shutting down state?
Comment 2 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2012-01-26 01:53:14 PST
> > +NS_IMPL_CYCLE_COLLECTION_CAN_SKIP_IN_CC_BEGIN(nsGlobalWindow)
> > +  return tmp->IsBlackForCC();
> 
> This is totally fine for now, but I don't see any particular reason we can't
> do all the UnmarkGraying here, too. 
CAN_SKIP_IN_CC runs in CC thread. unmarking js is not possible


> > +nsGlobalWindow::UnmarkGrayTimers()
> > +{
> > +  for (nsTimeout* timeout = FirstTimeout();
> > +       timeout && IsTimeout(timeout);
> 
> So the timeout list stops being timeouts once there's any non-timeouts?
Timeouts are stored in a circular linked list and IsTimeout check is need to
find the end of the list.


> > +  nsIScriptGlobalObject* GetCachedGlobalObject()
> > +  {
> > +    // Verify that we have a global so that this
> > +    // does never return non-null value when GetGlobalObject() would.
> 
> The second line of the comment is a little unclear.  Maybe you mean "always
> returns a null when GetGlobalObject() is null"?
That is what I mean, yes.


> 
> ::: dom/src/events/nsJSEventListener.cpp
> @@ +158,5 @@
> > +      (!mScopeObject || !xpc_IsGrayGCThing(mScopeObject)) &&
> > +      (!mHandler || !xpc_IsGrayGCThing(mHandler))) {
> > +    nsIScriptGlobalObject* sgo =
> > +      static_cast<nsJSContext*>(mContext.get())->GetCachedGlobalObject();
> > +    return sgo && sgo->IsBlackForCC();
> 
> Why is the event listener not black when sgo is null?
Because there might be non-cached-gray global
Comment 3 Andrew McCreight [:mccr8] 2012-01-26 06:46:35 PST
Thanks for the explanations!

(In reply to Olli Pettay [:smaug] from comment #2)
> > > +NS_IMPL_CYCLE_COLLECTION_CAN_SKIP_IN_CC_BEGIN(nsGlobalWindow)
> > > +  return tmp->IsBlackForCC();
> > 
> > This is totally fine for now, but I don't see any particular reason we can't
> > do all the UnmarkGraying here, too. 
> CAN_SKIP_IN_CC runs in CC thread. unmarking js is not possible

Okay, I didn't realize that, but it isn't too surprising.
Comment 4 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2012-01-26 07:18:47 PST
Created attachment 591789 [details] [diff] [review]
patch
Comment 5 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2012-01-26 12:30:02 PST
https://hg.mozilla.org/mozilla-central/rev/7aef3dcb28d0

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