470810 - getDirInternal should cache ProfD

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[(dormant account)]

calls to getDirInternal are expensive on arm, costing 2-4ms.

Comment 1

[Dave Townsend [:mossop]]

I'm happy to do this but I'm wondering whether the directory service itself is caching the result of ProfD. I'd assume it is a fairly quick lookup which makes me wonder why the cost of us getting it is so high.

Is that 2-4ms per call or total calls?

Comment 2

[Benjamin Smedberg]

Apparently, most of the time is being spent in xpconnect overhead for the getservice call.

Comment 3

[Dave Townsend [:mossop]]

So just fixing bug 470865 or caching the directory service would likely save us the majority of the time?

Comment 4

[Benjamin Smedberg]

Maybe. Although the call to dirsvc.get() also involves some xpconnect wrapping overhead.

Comment 5

[(dormant account)]

(In reply to comment #1)
> I'm happy to do this but I'm wondering whether the directory service itself is
> caching the result of ProfD. I'd assume it is a fairly quick lookup which makes
> me wonder why the cost of us getting it is so high.
> 
> Is that 2-4ms per call or total calls?

It's per call.

.getService() isn't much more expensive than other xpconnect calls, but it is called frequently is an easy target for optimization. .get and .append have a similar cost.

Comment 6

[(dormant account)]

Attachment: optimization

This didn't yield as much of a gain as I hoped for, only about ~30ms

Comment 7

[Dave Townsend [:mossop]]

A couple of very quick initial thoughts, I'll have time for a better review next week:

I was expecting to just cache the result of the directory service call, not the full getFile call. I'm a little surprised we are calling getFile for the same thing multiple times, what are we doing that for?

ifdeffing out xpcom calls for specific platforms feels pretty nasty to me, unless there is a decent saving from it I'm not sure I'm going to be happy with it.

You're clearing the cache and locator half way through the startup path. It is an odd place to do it and I'd imagine it loses you some potential wins later. We certainly need to clear these on shutdown.

Comment 8

[(dormant account)]

(In reply to comment #7)
> A couple of very quick initial thoughts, I'll have time for a better review
> next week:
> 
> I was expecting to just cache the result of the directory service call, not the
> full getFile call. I'm a little surprised we are calling getFile for the same
> thing multiple times, what are we doing that for?
The magic triplet:
 extensions.ini, extensions.rdf, extensions.cache

First the code checks for their existence..then in a different part of the code it actually reads from them. 

I don't like the way that's done as in addition to creating file objects too often it also tends to follow them by .exists() calls for no good reason.

I figured it'd be easier to cache all files than add 3 new global variables for each file and have to decide on where to initialize/clean up all of those vars.

There is no point in caching the getDirInternal call as the file object it returns is immediately mutated by the callers. All of the redundant getDirInternal calls happen because multiple files in a subdirectory are opened.
It might make sense to get rid of getFile/getDirectory way of building pathnames and only pass in a complete path to getFile() instead of doing it in multiple arguments. 


> 
> ifdeffing out xpcom calls for specific platforms feels pretty nasty to me,
> unless there is a decent saving from it I'm not sure I'm going to be happy with
> it.

I was afraid you were going to say that :). It's about half of the speed gain.

> 
> You're clearing the cache and locator half way through the startup path. It is
> an odd place to do it and I'd imagine it loses you some potential wins later.
> We certainly need to clear these on shutdown.

I clear them at the end of the cached startup path in my tests.

Comment 9

[Dave Townsend [:mossop]]

(In reply to comment #8)
> > ifdeffing out xpcom calls for specific platforms feels pretty nasty to me,
> > unless there is a decent saving from it I'm not sure I'm going to be happy with
> > it.
> 
> I was afraid you were going to say that :). It's about half of the speed gain.

Ouch, that's pretty disappointing. Random thought, I wonder if we can fast-path nsIFile operations as was done for DOM operations, must be pretty commonly used in chrome code.

> > 
> > You're clearing the cache and locator half way through the startup path. It is
> > an odd place to do it and I'd imagine it loses you some potential wins later.
> > We certainly need to clear these on shutdown.
> 
> I clear them at the end of the cached startup path in my tests.

You're clearing it at the end of the EM startup, I'm not sure why you'd clear it anywhere during the startup phase, why not just at shutdown? Normally extensions.rdf hasn't even been loaded by the time you clear the cache so there is likely a getFile request for that afterwards.

Comment 10

[(dormant account)]

I think it'd be nicer to clear the cache sometime during startup. if it happens that extension manager needs to access those files later, they'd get recached and can still be cleared on shutdown.

So are you interested in landing this patch for the minuscule perf gain(it's bigger if cache needs to be rebuilt) or should we refactor getFile usage in the code? Or should we WONTFIX this?

Comment 11

[Dave Townsend [:mossop]]

Attachment: alternate approach

I'm not adverse to the caching approach, but now I see where the cache is helping I think we can just rearrange things to get the same or better effect without the cache. This keeps the global directory service and changes the 5 calls to getFile into 1 call to getFile and 1 call to getDirNoCreate and a couple of file clones (which look to be cheap). I think we might be able to get rid of the last getFile call too but let's see how this looks for now.

Can we see what kind of effect this has on the numbers?

As for ifdeffing out the followLinks call, I'm not a fan but if bsmedberg is happy with it then we can include that too

Comment 12

[(dormant account)]

Comment on attachment 354671 [details] [diff] [review]
alternate approach

>   _ensureDatasetIntegrity: function EM__ensureDatasetIntegrity() {
>-    var extensionsDS = getFile(KEY_PROFILEDIR, [FILE_EXTENSIONS]);
>-    var extensionsINI = getFile(KEY_PROFILEDIR, [FILE_EXTENSION_MANIFEST]);
>-    var extensionsCache = getFile(KEY_PROFILEDIR, [FILE_EXTENSIONS_STARTUP_CACHE]);
>+    var profD = getDirNoCreate(KEY_PROFILEDIR, []);
>+    var extensionsDS = profD.clone();
>+    extensionsDS.append(FILE_EXTENSIONS);
>+    var extensionsINI = profD.clone();
>+    extensionsINI.append(FILE_EXTENSION_MANIFEST);
>+    var extensionsCache = profD;
>+    extensionsCache.append(FILE_EXTENSIONS_STARTUP_CACHE);

I like this. I think using .clone obsoletes getFile in the code. Lets get rid of getFile in favour of this approach.

> 
>     var dsExists = extensionsDS.exists();
>     var iniExists = extensionsINI.exists();
>     var cacheExists = extensionsCache.exists();
> 
>     if (dsExists && iniExists && cacheExists)
>-      return false;
>+      return [false, !iniExists];

nice job on avoiding the extra exists check.

This performs same or better than mine, without the ugly ifdef hack,so lets scrap that idea.

Comment 13

[Dave Townsend [:mossop]]

Comment on attachment 354671 [details] [diff] [review]
alternate approach

Rob, what do you think to this?

Comment 14

[Robert Strong (they/them - no direct email)]

Comment on attachment 354671 [details] [diff] [review]
alternate approach

Looks good

Comment 15

[Dave Townsend [:mossop]]

Landed: http://hg.mozilla.org/mozilla-central/rev/f768b388a775

Comment 16

[Dave Townsend [:mossop]]

Comment on attachment 354671 [details] [diff] [review]
alternate approach

This is a low risk patch that improves the startup time on mobile, would like to take it on 1.9.1

Comment 17

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 354671 [details] [diff] [review]
alternate approach

a191=beltzner

Comment 18

[Dave Townsend [:mossop]]

Landed on branch http://hg.mozilla.org/releases/mozilla-1.9.1/rev/1feb7b6b2f6d