942240 - Improve the skippability of nsGlobalWindow

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

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

(I ended up doing some bigger changes so the patch isn't quite as trivial as I was hoping,
but it will be more correct.)

Comment 1

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

Attachment: focused_windows_are_black_simple.diff

https://tbpl.mozilla.org/?tree=Try&rev=7e13c02083f7

Comment 2

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

Er, this one https://tbpl.mozilla.org/?tree=Try&rev=9c553eb0ffae

Comment 3

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

Comment on attachment 8337236 [details] [diff] [review]
focused_windows_are_black_simple.diff

This sanitizes nsGlobalWindow blackness check to be closer to normal
CCable objects, so that once we have fully webidl-fied window object, things
should just work right.

Comment 4

[Andrew McCreight [:mccr8]]

Comment on attachment 8337236 [details] [diff] [review]
focused_windows_are_black_simple.diff

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

::: content/base/src/nsCCUncollectableMarker.cpp
@@ +211,5 @@
>      }
>    }
> +  if (doc) {
> +    nsPIDOMWindow* inner = doc->GetInnerWindow();
> +    nsPIDOMWindow* outer = doc->GetWindow();

maybe move outer down to where it is used

::: dom/base/nsFocusManager.h
@@ +120,5 @@
>    static InputContextAction::Cause GetFocusMoveActionCause(uint32_t aFlags);
>  
>    static bool sMouseFocusesFormControl;
>  
> +  static bool IsKeepingWindowAlive(nsPIDOMWindow* aWindow)

Rather than calling this for every window, it would make a little more sense to me to have this be some kind of mark uncollectable method that sets the CC generation on the windows, and is called once from nsCCUncollectableMarker.  That would also make it easier to have the focus manager optimize out other things it owns later.  Do you not want to do that because the focusmanager usually doesn't hold things for long or something?

::: dom/base/nsGlobalWindow.cpp
@@ +1793,5 @@
> +  if (!nsCCUncollectableMarker::sGeneration) {
> +    return false;
> +  }
> +
> +  return (nsCCUncollectableMarker::InGeneration(GetMarkedCCGeneration()) ||

It would be a little clearer if you split out the first part of this into a predicate like isDefinitelyAlive.

::: dom/base/nsWrapperCacheInlines.h
@@ +49,5 @@
>    return false;
>  }
>  
> +inline bool
> +nsWrapperCache::HasNothingToTrace(nsISupports* aThis)

Please define IsBlackAndDoesNotNeedTracing in terms of this function rather than duplicating the code.

Comment 5

[Andrew McCreight [:mccr8]]

Hurray for making windows just a little bit less weird! :)

Comment 6

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

Attachment: v2

Indeed, marking focusmanager's windows (and other interesting member variables) to be in cc generation is better.

Comment 7

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

Attachment: v2

Comment 8

[Andrew McCreight [:mccr8]]

Comment on attachment 8337376 [details] [diff] [review]
v2

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

::: dom/base/nsFocusManager.cpp
@@ +3438,5 @@
>  
> +void
> +nsFocusManager::MarkUncollectableForCCGeneration(uint32_t aGeneration)
> +{
> +  if (sInstance) {

Maybe change this to early return?

::: dom/base/nsWrapperCacheInlines.h
@@ +36,5 @@
>    }
>  }
>  
>  inline bool
> +nsWrapperCache::HasNothingToTrace(nsISupports* aThis)

Now that I think about it, it might be nicer if this was NeedsTracing() and you could just return hasGrayObjects.  Just to avoid a negation in there.

@@ +50,4 @@
>  nsWrapperCache::IsBlackAndDoesNotNeedTracing(nsISupports* aThis)
>  {
>    if (IsBlack()) {
> +    return HasNothingToTrace(aThis);

The method body could just be |return IsBlack() && HasNothingToTrace(aThis);|

Comment 9

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

Attachment: v3

HasNothingToTrace is what the callers need, so nicer than NeedsTracing

Comment 10

[Andrew McCreight [:mccr8]]

Yeah, I guess you have to reason about a negation either way.

Comment 11

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/41937f231795

Comment 12

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/41937f231795