In some light testing (load nytimes.com, browse around a bit), the SelectorCacheKeyDeleter runnable is the 6th most frequently run task (868 times in about a minute). This isn't really a big deal, but do we need to run it so much? It seems like all it does is to deallocate a string. Runnables have a decent amount of overhead, so it seems a bit silly to do it this way. Boris, I wasn't able to understand why this needs to be in a separate runnable. In the bug [1], you said: "One other thing: reading the expiration tracker code more closely, it looks like it could expire a cached entry while we're selector-matching. We should probably make our NotifyExpired do the delete async..." I don't know anything about this code, so I don't know why that would be bad. Is it something we could handle in a different way? [1] https://bugzilla.mozilla.org/show_bug.cgi?id=863966#c32

The setup with expiration tracker is that it will call NotifyExpired on your objects at some point. That point can be pretty much any time, because it listens for memory pressure notifications, which can in theory happen at any point (e.g. on any allocation). That said, the code currently in the tree is wrong. The danger with the expiration setup is that if we're selector-matching (and hence using the nsCSSSelectorList* that the expiration tracker owns) and then we get memory pressure and expire things, it could get deleted out from under us. The idea was to do _all_ the cleanup work async, but the patch that was checked in removes from the hashtable sync (this is bad, because it will kill the value!) and then deletes the key async. That's just dumb. In terms of handling this differently, we could do something where we tell the selector cache that we're using a selector from it and have it clean things up async only if an expiration happens while a selector is in use (which should be _quite_ rare). Or something. Doing the removal from the hashtable async is a bit of a pain, really, because who's to say the hashtable won't just die altogether in the meantime? One option that would be really annoying would be to remove from the hashtable when we start using the selector and readd when we finish. Annoying because it would involves more hashtable ops....

OK, thanks. That explanation makes a lot more sense. I looked through the code because I wasn't sure if we can fire memory pressure events at any time. I don't see that happening. As best as I can tell, they run here: http://searchfox.org/mozilla-central/rev/30fcf167af036aeddf322de44a2fadd370acfd2f/xpcom/threads/nsThread.cpp#1468 There are also a few other specialized places, but I don't see anything that runs when an allocation fails. Nick, can you correct me if I'm wrong? If that's the case, then I assume we can do this all synchronously? That would be nice.

As far as I know, memory pressure events are all handled via the observer service, so they can only take place when observer notifications can be processed.

Well, observers fire right away (as soon as you call Notify), so that doesn't answer the question, unfortunately.

At the point when the code was written, the plan of record for infallible malloc was that allocation failure would fire memory pressure, then try again. It looks like we never ended up implementing that? I guess what we could do is rip this async stuff out and document very carefully in the infallible malloc machinery that if we ever start firing memory pressure events there we need to carefully audit all expiration tracker consumers (maybe with an explicit pointer to this code?).

Searching for /Notify.*"memory-pressure"/ shows up a few hits. The only one that looks like it can happen immediately after an allocation request is in XPCJSContext::CustomLargeAllocationFailureCallback(). If you follow the trail back you end up in JSRuntime::pod_callocCanGC(), which calls onOUtOfMemoryCanGC(), which ends up sending memory-pressure notifications. There is also SharedArrayRawBuffer::New(), which can call rt->largeAllcoationFailureCallback(), which follows a similar path.

Yeah, at the moment selector matching can't trigger either of those codepaths, because it can't run JS. Whether that stays true.. who knows.

I suspect a lot of code would break besides this stuff if we started running a GC/CC (as well as a lot of other stuff) in the middle of an allocation. I'll repurpose this bug to remove the runnable here and add a comment as in comment 5. Hope that's okay Boris.

The failure mode would be JS running during selector matching, not during allocation. But yes, this sounds reasonable.

Oh, and the point is that XPCJSContext::CustomLargeAllocationFailureCallback can only run on mainthread, so it's safe to assume that it can't race selector matching here... And luckily CycleCollectedJSContext::CustomLargeAllocationFailureCallback (which can run on worker threads, etc) does nothing.

I'll fix this first before labeling nsIDocument::SelectorCache in bug 1346145.

After reading all the comments above and reviewing codes in tree, I found that what would possibly causes a race is the *Remove* operation on SelectorCache::mTable instead of SelectorCacheKeyDeleter. http://searchfox.org/mozilla-central/rev/ef0b6a528e0e7b354ddf8cdce08392be8b8ca679/dom/base/nsDocument.cpp#1244 The race we talked about shall already exists in the tree only if "memory-pressure" notified from JS during selector matching is possible. It seems impossible because there is no JS allocation during selector matching. (BTW, I didn't found a "memory-pressure" notified synchronously during malloc in C/C++ but an async one via NS_DispatchMemoryPressure() instead.) Hence, it doesn't matter when the SelectorCacheKeyDeleter is done. Besides, deleting *aSelector* immediately in SelectorCache::NotifyExpired() provides better result for the "memory-pressure" notification. Hi Boris, It seems to be a false alarm to me and it's shall be safe to remove this runnable with no concern. How do you think?

Delete SelectorCacheKey without a runnable because there is no use case to race selector matching: 1. GetList(), CacheList() and NotifyExpired() of SelectorCache are done on the main thread. Note: MOZ_ASSERT(NS_IsMainThread()) is added in these methods to ensure this. 2. The infallible malloc by notifying "memory-pressure" is supported in JS but not in c/c++ yet and there is no JS allocation during seletor matching. Note: "memory-pressure" was observed by nsExpirationTracker to release these caches when memory is low. Hi Boris, I thought you are the right person for this review because you have done some discussion on this bug but your r flag is disabled currently. Could you recommend someone to review this change? Thanks!

Comment on attachment 8846522 [details] [diff] [review] Patch: Delete SelectorCacheKey synchronously since there is no race to the selector matching. The commit message is ... really confusing. There are no races here; this is all single-threaded. Adding the asserts for that is fine. Anyway, there is only reentry to consider. Also, the documentation I mentioned in comment 5 needs to happen.

1. Update the commit message for better understanding. 2. Add documentation in mozalloc_handle_oom() and SelectorCache::NotifyExpired().

Comment on attachment 8847055 [details] [diff] [review] Bug 1332491 - Delete SelectorCacheKey synchronously. > 3. Add warning in mozalloc_handle_oom() to revise SelectorCache::NotifyExpired() > if the stage 2 of handling OOM is done. This doesn't need to be in the commit message. >+ bool GetList(const nsAString& aSelector, nsCSSSelectorList** aList); Please leave this inline. This code is fairly performance-sensitive. >+ // "memory-pressure" synchronously, the revise to some use case >+ // of nsExpirationTracker such as nsIDocument::SelectorCache:: >+ // NotifyExpired() is needed to prevent the re-entry to reclaim >+ // in-used data unexpectedly. How about something like: // "memory-pressure" synchronously, please audit all // nsExpirationTrackers and ensure that the actions they take // on memory-pressure notifications (via NotifyExpired) are safe. // Note that nsIDocument::SelectorCache::NotifyExpired is _known_ // to not be safe: it will delete the selector it's caching, // which might be in use at the time under // querySelector or querySelectorAll. r=me with those changes

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #16) > Comment on attachment 8847055 [details] [diff] [review] > Bug 1332491 - Delete SelectorCacheKey synchronously. > > > 3. Add warning in mozalloc_handle_oom() to revise SelectorCache::NotifyExpired() > > if the stage 2 of handling OOM is done. > > This doesn't need to be in the commit message. I'll remove it when committing. > > >+ bool GetList(const nsAString& aSelector, nsCSSSelectorList** aList); > > Please leave this inline. This code is fairly performance-sensitive. > Will do. > >+ // "memory-pressure" synchronously, the revise to some use case > >+ // of nsExpirationTracker such as nsIDocument::SelectorCache:: > >+ // NotifyExpired() is needed to prevent the re-entry to reclaim > >+ // in-used data unexpectedly. > > How about something like: > > // "memory-pressure" synchronously, please audit all > // nsExpirationTrackers and ensure that the actions they take > // on memory-pressure notifications (via NotifyExpired) are safe. > // Note that nsIDocument::SelectorCache::NotifyExpired is _known_ > // to not be safe: it will delete the selector it's caching, > // which might be in use at the time under > // querySelector or querySelectorAll. Thanks, this looks much better.

Pushed by btseng@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/ab96d8a9e247 Delete SelectorCacheKey synchronously. r=bz