1499808 - change nsUrlClassifierPrefixSet/VariableLengthPrefixSet to use async memory reporting

Status: NEW

(Toolkit :: Safe Browsing, defect, P5)

Description

[Nathan Froyd [:froydnj]]

nsUrlClassifierPrefixSet has the following (VariableLengthPrefixSet has the same issues, but apparently not quite as severe):

  // Lock to prevent races between the url-classifier thread (which does most
  // of the operations) and the main thread (which does memory reporting).
  // It should be held for all operations between Init() and destruction that
  // touch this class's data members.
  mutable mozilla::Mutex mLock;

Bug 1498643 comment 4 shows that we acquire this lock a non-trivial number of times:

Mutex nsUrlClassifierPrefixSet.mLock: 0/19263 ( 0.000%)
Mutex nsUrlClassifierPrefixSet.mLock: 0/19263 ( 0.000%)
Mutex nsUrlClassifierPrefixSet.mLock: 0/19263 ( 0.000%)
Mutex nsUrlClassifierPrefixSet.mLock: 0/19263 ( 0.000%)
Mutex nsUrlClassifierPrefixSet.mLock: 0/19263 ( 0.000%)
Mutex nsUrlClassifierPrefixSet.mLock: 0/19299 ( 0.000%)
Mutex nsUrlClassifierPrefixSet.mLock: 0/19300 ( 0.000%)
Mutex nsUrlClassifierPrefixSet.mLock: 0/19300 ( 0.000%)
Mutex nsUrlClassifierPrefixSet.mLock: 0/19302 ( 0.000%)
Mutex nsUrlClassifierPrefixSet.mLock: 0/22053 ( 0.000%)
Mutex nsUrlClassifierPrefixSet.mLock: 0/22074 ( 0.000%)

for a short-ish session watching things on CNN.  Each line above is a separate prefix set instance and the 0/19263 show contended acquisitions/total acquisitions.  There are other instances that don't show nearly as many acquisitions as well.

All of these acquisitions (and their corresponding releases) are essentially wasted work: we're putting the burden of correctness on the common case (using the prefix set) rather than the uncommon case (when memory reporting happens).  We should make the uncommon case bear the burden of correctness instead, and make the common case as fast as we can.

The problem is that we have a bunch of data that's only safe to touch from the url-classifier thread.  Fortunately, memory reporting provides async memory reporters that can, presumably, collect data on whatever thread they like, and then forward things back to the main thread for processing.  We have a couple examples in the tree:

https://searchfox.org/mozilla-central/search?q=Register.*Async.*Reporter&case=false&regexp=true&path=

and we could presumably use the same techniques here.

Nick, do you have cycles to do this kind of conversion?

Comment 1

[Nicholas Nethercote [inactive]]

Not right now, unfortunately. I agree this would be a good thing to fix, though.