1360971 - nsContentPolicy::CheckPolicy() is a bit malloc heavy

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P1)

Description

[(no longer active)]

The free() function from the nsCOMArray destructor used here <http://searchfox.org/mozilla-central/rev/3dc6ceb42746ab40f1441e1e659ffb8f62ae78e3/dom/base/nsContentPolicy.cpp#128> and <http://searchfox.org/mozilla-central/rev/3dc6ceb42746ab40f1441e1e659ffb8f62ae78e3/dom/base/nsContentPolicy.cpp#190> shows up in profiles of bug 1347376.

We shouldn't really recompute the list of these categories every time since they almost never change.  The nsCategoryObserver object knows about these changes, and it seems like we can either listen to those notifications and invalidate a cache ourselves or add an API to nsCategoryCache to keep an nsCOMArray updated with changes to the underlying category observer or something similar.

Nathan, do you prefer a specific solution on the XPCOM side?

Comment 1

[Nathan Froyd [:froydnj]]

(In reply to :Ehsan Akhgari (super long backlog, slow to respond) from comment #0)
> Nathan, do you prefer a specific solution on the XPCOM side?

I think the real question is whether we have to enumerate the hashtable entries that nsCategoryObserver (via nsCategoryCache) has into a separate array, and then iterate the array--because we have to worry that calling out to the things contained therein will register/deregister services/content policies while we're trying to iterate over them (??)--or whether we could just iterate over the hashtable directly.  Because if it's the former, then it seems like we're doomed to take a snapshot of the data no matter what, right?  And then the only feasible thing to do is to use nsAutoTArray<nsCOMPtr<T>> or something like that for our data structure.  But if it's the latter, then we should indeed come up with some API that doesn't require this temporary data structure.

I don't know the nsContentPolicy code well enough to say which case we're handling.  But does that analysis seem right?

Comment 2

[(no longer active)]

Yes, your analysis is right on, thanks Nathan!

This code dates back to bug 246092.  I'm not really following the actual reason why this was added but it looks like that bug is talking about caching, so I gather the motivation there was speed.  At any rate, the content-policy category registrations are mostly static now (or they should be at least) so I think we shouldn't care much about the case where one content policy adds/removes one as CheckLoad/CheckPolicy is being called.

Boris, what do you think?  This is technically a breaking change to the API, I guess.

We can also not break the API and just cache the nsCOMArray properly as I suggested earlier with a more sophisticated implementation that just keeps the nsCOMAarray updated as the categories change, if the existing semantics is worth preserving.

Comment 3

[Boris Zbarsky [:bzbarsky]]

I would rather just keep the existing semantics; it doesn't seem like it would be hard to just cache the array and I really don't want to worry about the things people do in this API impl.

Comment 4

[(no longer active)]

Makes sense.

Nathan, do you have any preferences how the XPCOM side of things get done here?

Comment 5

[Nathan Froyd [:froydnj]]

(In reply to :Ehsan Akhgari (super long backlog, slow to respond) from comment #4)
> Nathan, do you have any preferences how the XPCOM side of things get done
> here?

Um, not sure?  In comment 0:

(In reply to :Ehsan Akhgari (super long backlog, slow to respond) from comment #0)
> We shouldn't really recompute the list of these categories every time since
> they almost never change.  The nsCategoryObserver object knows about these
> changes, and it seems like we can either listen to those notifications and
> invalidate a cache ourselves or add an API to nsCategoryCache to keep an
> nsCOMArray updated with changes to the underlying category observer or
> something similar.

How would this cached nsCOMArray work?  Would we have something like, in nsCategoryCache:

nsCOMArray& GetCachedEntries()
{
  if (HasDirtyCache()) {
    mCachedEntries.Clear();
    GetEntries(mCachedEntries);
  }
  return mCachedEntries;
}

where HasDirtyCache would probably be implemented by some cooperation between nsCategoryCache and nsCacheObserver?

If so, that would be OK, maybe with some NS_IsMainThread() asserts in the appropriate functions and an explanation of why we have this cache in the first place.

Comment 6

[(no longer active)]

Yeah, that's what I had in mind basically...

Comment 7

[(no longer active)]

Attachment: Part 1: Add the nsCategoryCache<T>::GetCachedEntries() API

This provides a more efficient way to read the entries out of an
XPCOM category cache.

Comment 8

[(no longer active)]

Attachment: Part 2: Avoid allocation overhead for reading the content policies in nsContentPolicy::CheckPolicy()

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8799d30a7dc4
Part 1: Add the nsCategoryCache<T>::GetCachedEntries() API; r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f981516fb36
Part 2: Avoid allocation overhead for reading the content policies in nsContentPolicy::CheckPolicy(); r=mystor

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8799d30a7dc4
https://hg.mozilla.org/mozilla-central/rev/9f981516fb36