672651 - Track cache init times via telemetry

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[(dormant account)]

Attachment: Measure cache init times

Comment 1

[Jason Duell]

Comment on attachment 546909 [details] [diff] [review]
Measure cache init times

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

I'm working on a patch that will change how DeleteDir works, and I'll incorporate these telemetry calls into it.  Thanks for the patch.

::: netwerk/cache/nsDeleteDir.cpp
@@ +46,4 @@
>  
>  static void DeleteDirThreadFunc(void *arg)
>  {
> +  Telemetry::AutoTimer<Telemetry::NETWORK_DISK_CACHE_DELETEDIR> timer;

Actually the real killer is not this function (which occurs off-main-thread), but the rename of Cache -> Cache.trash, which is massively slow on NTFS.  See bug 670911 comment 5.  I'll time both the directory move and the deletion separately.

Comment 3

[(dormant account)]

(In reply to comment #1)
> Comment on attachment 546909 [details] [diff] [review] [review]
> Measure cache init times
> 
> Review of attachment 546909 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> I'm working on a patch that will change how DeleteDir works, and I'll
> incorporate these telemetry calls into it.  Thanks for the patch.
> 


This also does cache init telemetry. Can we land that bit?

According to the trace I saw, deleting large numbers of files also starves our io elsewhere. I don't see a strong reason to not land this while you are working on your improved version. Worst case we'll have some before/after numbers.

Comment 4

[Jason Duell]

> deleting large numbers of files also starves our io elsewhere. 

Which is why we're going to slowly delete the file tree, with sleeps in between.  

> I don't see a strong reason to not land this while you are working on your 
> improved version. Worst case we'll have some before/after numbers.

The code to get the timing will change (sleep calls == more than 1 function call, so no autotimer), as well as the meaning (the timing for the fixed version will actually be much higher due to the sleep calls).  Also, I'm going to request aurora approval for the fix, so it makes life slightly easier not to have to write a separate patch for it that includes or works around this patch not being there.  But I can work around that if you think it's really worth having these numbers for nightly users now.

> This also does cache init telemetry. Can we land that bit?

Yes, of course.

Comment 5

[(dormant account)]

(In reply to comment #4)

> > This also does cache init telemetry. Can we land that bit?
> 
> Yes, of course.

What i meant was, can I scrap the delete part and land the init part asap? if so I'll reopen bug

Comment 6

[Jason Duell]

Yes.  You've got my +r for that part--go ahead and land it.

Comment 7

[(dormant account)]

Attachment: measure cache init

cut down patch

Comment 8

[(dormant account)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/47c7c675129f

Comment 9

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/47c7c675129f

please don't narj bygs as fixed when pushing to inbound, fixed should only be used for central.

Comment 10

[Marco Bonardo [:mak]]

damn, I meant "don't mark bugs as", sorry just changed keyboard layout :)

Comment 11

[(dormant account)]

(In reply to comment #9)
> http://hg.mozilla.org/mozilla-central/rev/47c7c675129f
> 
> please don't narj bygs as fixed when pushing to inbound, fixed should only
> be used for central.

Sorry, brain misfired.