1396958 - Make eTLD service cache lookup thread safe

Status: RESOLVED FIXED

(Core :: Networking: DNS, defect)

Description

[Eric Rahm [:erahm]]

Currently the eTLD service's cache is main-thread only. It would to remove this restriction so that components that want to move off main-thread can still access it.

Comment 1

[Eric Rahm [:erahm]]

Attachment: Make eTLD cache thread-safe

Adds a mutex that is used to guard lookups of and updates to the MRU cache.
This allows us to remove the main-thread only assertion in the lookup code.

Comment 2

[Nathan Froyd [:froydnj]]

Comment on attachment 8904684 [details] [diff] [review]
Make eTLD cache thread-safe

Review of attachment 8904684 [details] [diff] [review]:
-----------------------------------------------------------------

Not a huge fan of this...Would it be worth having a main-thread only cache and a everybody-else cache, and only locking for the everybody-else case?  My guess is that the main thread case is overwhemingly common, but I don't really know the netwerk code all that well.  Do we know what kind of performance hit this entails?

Alternatively, could we just get rid of the cache entirely?

::: netwerk/dns/nsEffectiveTLDService.cpp
@@ +313,5 @@
>  
>    // Update the MRU table if in use.
>    if (entry) {
> +    // Lock while updating the entry. We know the pointer is still valid as
> +    // the table size is static and immutable.

...and do we also care that we might be overwriting a value that was now equal to the value we are now storing?  I guess we don't.

Perhaps the comment should include something to the effect of "Even though we dropped the lock after the unsuccessful lookup, we know that the pointer is still valid as the table size is static and immutable, so we don't have to worry about what happened in between the dropped lock and now."  So we're making it very clear that we understand what we just did, and providing context as to why this is OK.

Comment 3

[Eric Rahm [:erahm]]

(In reply to Nathan Froyd [:froydnj] from comment #2)
> Comment on attachment 8904684 [details] [diff] [review]
> Make eTLD cache thread-safe
> 
> Review of attachment 8904684 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Not a huge fan of this...Would it be worth having a main-thread only cache
> and a everybody-else cache, and only locking for the everybody-else case? 
> My guess is that the main thread case is overwhemingly common, but I don't
> really know the netwerk code all that well.  Do we know what kind of
> performance hit this entails?

We could just make the lookup dependent on `NS_IsMainThread()` and avoid mutexes all together. I haven't measure the perf hit but we've previously gone with the assumption that uncontested locks are fast.

> Alternatively, could we just get rid of the cache entirely?

We cannot. Adding the DAFSA for eTLD lookups was a perf hit, but putting a small cache on top of it made it a perf win (bug 1380154, comment 20).

> ::: netwerk/dns/nsEffectiveTLDService.cpp
> @@ +313,5 @@
> >  
> >    // Update the MRU table if in use.
> >    if (entry) {
> > +    // Lock while updating the entry. We know the pointer is still valid as
> > +    // the table size is static and immutable.
> 
> ...and do we also care that we might be overwriting a value that was now
> equal to the value we are now storing?  I guess we don't.

Yeah it really doesn't matter.

> Perhaps the comment should include something to the effect of "Even though
> we dropped the lock after the unsuccessful lookup, we know that the pointer
> is still valid as the table size is static and immutable, so we don't have
> to worry about what happened in between the dropped lock and now."  So we're
> making it very clear that we understand what we just did, and providing
> context as to why this is OK.

Sure that makes sense, but let me know if you prefer to just not use the cache off main thread (bug 870460, comment 59 indicates it's pretty rare and being slower off main thread is maybe less of an issue).

Comment 4

[Nathan Froyd [:froydnj]]

(In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment #3)
> (In reply to Nathan Froyd [:froydnj] from comment #2)
> > Perhaps the comment should include something to the effect of "Even though
> > we dropped the lock after the unsuccessful lookup, we know that the pointer
> > is still valid as the table size is static and immutable, so we don't have
> > to worry about what happened in between the dropped lock and now."  So we're
> > making it very clear that we understand what we just did, and providing
> > context as to why this is OK.
> 
> Sure that makes sense, but let me know if you prefer to just not use the
> cache off main thread (bug 870460, comment 59 indicates it's pretty rare and
> being slower off main thread is maybe less of an issue).

My gut feeling is that this is what we want to do...but this is wandering into Necko territory about how often various things are used, and I have no knowledge of that.

So you should get somebody with appropriate privileges to sign off on that decision (valentin?  nick hurley?); if a Necko peer thinks not caching OMT is OK, that's fine, and if they think locking all the time is appropriate, that's fine too. :)

Comment 5

[Eric Rahm [:erahm]]

(In reply to Nathan Froyd [:froydnj] from comment #4)
> My gut feeling is that this is what we want to do...but this is wandering
> into Necko territory about how often various things are used, and I have no
> knowledge of that.
> 
> So you should get somebody with appropriate privileges to sign off on that
> decision (valentin?  nick hurley?); if a Necko peer thinks not caching OMT
> is OK, that's fine, and if they think locking all the time is appropriate,
> that's fine too. :)

Okay, I'll update to just not cache OMT and redirect to valentin.

Comment 6

[Eric Rahm [:erahm]]

Attachment: Make eTLD cache thread-safe

Restrict the MRU cache for eTLD lookups to main thread only. This allows off
main thread lookups, but they will just take a slower path.

Comment 7

[Valentin Gosu [:valentin]]

Comment on attachment 8906738 [details] [diff] [review]
Make eTLD cache thread-safe

Review of attachment 8906738 [details] [diff] [review]:
-----------------------------------------------------------------

I guess if there is no big performance regression from this, it should all be OK.

Comment 8

[Eric Rahm [:erahm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/afff11392670ae77ad6da75c1df446e5a21c79b4
Bug 1396958 - Make eTLD cache thread-safe. r=valentin

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/afff11392670