nsContentPolicy::CheckPolicy() is a bit malloc heavy

RESOLVED FIXED in Firefox 55

Status

()

P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

2 years ago
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?
Flags: needinfo?(nfroyd)
(Assignee)

Updated

2 years ago
Blocks: 1347376
(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?
Flags: needinfo?(nfroyd)
(Assignee)

Comment 2

2 years ago
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.
Flags: needinfo?(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.
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 4

2 years ago
Makes sense.

Nathan, do you have any preferences how the XPCOM side of things get done here?
Flags: needinfo?(nfroyd)
(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.
Flags: needinfo?(nfroyd)
Priority: -- → P1
(Assignee)

Comment 6

2 years ago
Yeah, that's what I had in mind basically...
(Assignee)

Updated

2 years ago
Assignee: nobody → ehsan
(Assignee)

Comment 7

2 years ago
Created attachment 8865268 [details] [diff] [review]
Part 1: Add the nsCategoryCache<T>::GetCachedEntries() API

This provides a more efficient way to read the entries out of an
XPCOM category cache.
Attachment #8865268 - Flags: review?(nfroyd)
(Assignee)

Comment 8

2 years ago
Created attachment 8865269 [details] [diff] [review]
Part 2: Avoid allocation overhead for reading the content policies in nsContentPolicy::CheckPolicy()
Attachment #8865269 - Flags: review?(michael)
Attachment #8865268 - Flags: review?(nfroyd) → review+

Updated

2 years ago
Attachment #8865269 - Flags: review?(michael) → review+

Comment 9

2 years ago
Pushed by eakhgari@mozilla.com:
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8799d30a7dc4
https://hg.mozilla.org/mozilla-central/rev/9f981516fb36
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.