1424816 - The Document state cache is not great, and doesn't look very useful

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Emilio Cobos Álvarez (:emilio)]

See bug 1422633, there are assertions missing, and servo doesn't assert at all anymore.

I don't think it's worth optimizing / lazily resolving it, each time the document state changes we usually just restyle the world anyway, and the changes that it's optimizing (nsWindow::SetActive() and xul <browser> attribute changes) aren't common enough to warrant the complexity I'd say.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1424816: Remove the document state cache.

See bug 1422633, there are assertions missing, and servo doesn't assert at all
anymore.

I don't think it's worth optimizing / lazily resolving it, each time the
document state changes.

We usually just restyle the world anyway (which requires recomputing it), and
the changes that it's optimizing (nsWindow::SetActive() and XUL root element
localedir attribute changes) aren't common enough to warrant the complexity I'd
say.

Review commit: https://reviewboard.mozilla.org/r/207062/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/207062/

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8936330 [details]
Bug 1424816: Remove the document state cache.

Err, this fails browser_windowactivation.js, but I bet it's because of the method living in nsPresContext...

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8936330 [details]
Bug 1424816: Remove the document state cache.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/207062/diff/1-2/

Comment 4

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

Comment on attachment 8936330 [details]
Bug 1424816: Remove the document state cache.

Manish added ThreadSafeGetDocumentState(), perhaps he recalls why it was done that way.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

(In reply to Olli Pettay [:smaug] (won't be in Austin) from comment #4)
> Comment on attachment 8936330 [details]
> Bug 1424816: Remove the document state cache.
> 
> Manish added ThreadSafeGetDocumentState(), perhaps he recalls why it was
> done that way.

The cache was added in bug 508482, fwiw, and there doesn't seem to be any reason for that.

Comment 6

[Manish Goregaokar [:manishearth]]

> Manish added ThreadSafeGetDocumentState(), perhaps he recalls why it was done that way.

That was just to bypass the cache so that Stylo can safely use this. If you remove the cache you can just remove all the threadsafe stuff.

Comment 7

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

I still don't understand what ThreadSafeGetDocumentState is about. Nothing there seems to be threadsafe as such. No atomic being used or mutexes or such.

Comment 8

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

Comment on attachment 8936330 [details]
Bug 1424816: Remove the document state cache.

I think I should not review this, since I really don't understand what ThreadSafeGetDocumentState() was trying to capture and why the patch removing it is ok.

Comment 9

[Manish Goregaokar [:manishearth]]

It's threadsafe because it *doesn't* hit the cache. It does more work as a tradeoff.

Comment 10

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

ThreadSafeGetDocumentState doesn't do much work. It just returns mDocumentState.
And I don't even understand why accessing that is threadsafe. The underlying code doesn't seem to use atomic or such for the value.

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8936330 [details]
Bug 1424816: Remove the document state cache.

(Talked with Olli on IRC)

Comment 12

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

It was name ThreadSafeGetDocumentState which is confusing. That method as such isn't thread safe. It is safe only if no one sets the value while that is being called.
So, it is safe in Stylo case, but not in general.

Comment 13

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

Comment on attachment 8936330 [details]
Bug 1424816: Remove the document state cache.

https://reviewboard.mozilla.org/r/207062/#review213400

::: dom/base/nsIDocument.h:1567
(Diff revision 2)
>    {
>      nsPIDOMWindowInner* window = GetInnerWindow();
>      return window ? window->WindowID() : 0;
>    }
>  
> +  bool IsTopLevelWindowInactive() const;

Rather oddly named method for nsIDocument, but I see we use similar name in PresContext too, so fine.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b18567e1737
Remove the document state cache. r=smaug

Comment 15

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/5b18567e1737