Closed Bug 1350655 Opened 4 years ago Closed 4 years ago

MapURIToAddonID is too slow

Categories

(Core :: Networking, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

See this profile <https://perfht.ml/2n0zHtQ> from bug 1312373.

205ms of total of 4.4 seconds is spent in just this one function!  This is called from a whole bunch of places, but specifically from performance critical code like: https://perfht.ml/2nj2pae which is on the path of installing XBL fields.

Almost all of the cost is going to nsChromeRegistry::ConvertChromeURL().  Can we special case the chrome:// URIs that we ship or something?
Flags: needinfo?(wmccloskey)
I looked a bit at the access patterns here.  The XBL consumers tend to call this function over and over again passing the same aURI, so doing a very simple 1-level caching improves things a lot.

I have a simple patch that does that, which improves things by about 60-70% in a profile of restoring ~30 tabs.

But I also think we should consider adding fast paths for URIs such as chrome://browser, chrome://global, etc. here.
Attachment #8851310 - Attachment is obsolete: true
Attachment #8851310 - Flags: review?(wmccloskey)
With this patch on top of the previous one, MapURIToAddonID() is completely eliminated from this profile! \o/
Attachment #8851380 - Flags: review?(wmccloskey)
Assignee: nobody → ehsan
Comment on attachment 8851380 [details] [diff] [review]
Part 2: Special case some of the well-known chrome:// URIs that can never be used for add-on packages in ResolveURI()

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

Thanks. Really sorry about this :-(.
Attachment #8851380 - Flags: review?(wmccloskey) → review+
Comment on attachment 8851313 [details] [diff] [review]
Add a 1-level cache to MapURIToAddonID() to speed up XBL consumers, among others

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

::: toolkit/mozapps/extensions/AddonPathService.cpp
@@ +225,5 @@
> +    mAddonId = nullptr;
> +  }
> +  void Cache(void* aURI, JSAddonId* aAddonId)
> +  {
> +    mURI = aURI;

This is a little scary. In theory, the URI could be freed and then a completely different one could be allocated at the same address. I suspect this isn't even that unlikely. Maybe we could key the cache on the URL spec instead? That should be pretty fast to access, although we'd need to do a string comparison.

Even with your other patch, the cache is probably still useful for resource URIs. What if we special-cased resource:/// and resource://gre/ as well as the chrome stuff? Then maybe we wouldn't need the cache.
(In reply to Bill McCloskey (:billm) from comment #6)
> ::: toolkit/mozapps/extensions/AddonPathService.cpp
> @@ +225,5 @@
> > +    mAddonId = nullptr;
> > +  }
> > +  void Cache(void* aURI, JSAddonId* aAddonId)
> > +  {
> > +    mURI = aURI;
> 
> This is a little scary. In theory, the URI could be freed and then a
> completely different one could be allocated at the same address. I suspect
> this isn't even that unlikely. Maybe we could key the cache on the URL spec
> instead? That should be pretty fast to access, although we'd need to do a
> string comparison.

Yes, now that you mentioned it, I surprised it seemed like a good idea to me.  I guess I wrote the patch pretty late at night and it made things faster so I didn't hesitate much.  ;-)

> Even with your other patch, the cache is probably still useful for resource
> URIs. What if we special-cased resource:/// and resource://gre/ as well as
> the chrome stuff? Then maybe we wouldn't need the cache.

No, nothing non-chrome ever shows up in my profile.  But just to double check, I reverted part 1, and retested just with part 2, and indeed we don't need the cache at all!  \o/  So I will only land part 2.

(In reply to Bill McCloskey (:billm) from comment #5)
> Thanks. Really sorry about this :-(.

You didn't do anything wrong, this is the classic XPCOM way of doing things is super slow.  :-(

In general, I think after this Quantum rush, we should pick up the de-Contamination work again quite actively.  We let things slide for many years, and every time I look at profiles stupid things are super slow...  And even I keep letting things slide because I don't know what to do about specific instances.  For example, URI parsing shows up in profiles every now and then, and usually the XPCOM overhead is visible... same with a lot of our security checks, some of which is a bit better now, and so on.  Everywhere you look we have a lot of these issues.  In many cases when I look into it, it's because we're using our abstractions, which while nice are a bit unnecessary, for example like here!  The chrome registry component could have just as easily had a central global list of built in packages as a first class C++ API (for example an array of const char*!) and then the default way you would write this code would be the correct way...  For now the patch the hole approach like this will allow us to fix the bleeding but we need to have an effort to simplify our code in ways that improve things performance wise.  Anyways, sorry for the rant.  Just wanted to say that I would have written the same code if I were in your shoes before.  :D
Flags: needinfo?(wmccloskey)
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e442b92ea2d
Special case some of the well-known chrome:// URIs that can never be used for add-on packages in ResolveURI(); r=billm
https://hg.mozilla.org/mozilla-central/rev/1e442b92ea2d
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Awesome, with this patch talos is indicating a performance improvement:

== Change summary for alert #5681 (as of March 26 2017 21:12 UTC) ==

Improvements:

  2%  tpaint windows8-64 opt      305.04 -> 298.48

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=5681
You need to log in before you can comment on or make changes to this bug.