Last Comment Bug 672651 - Track cache init times via telemetry
: Track cache init times via telemetry
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Networking: Cache (show other bugs)
: unspecified
: x86 Linux
: -- normal (vote)
: mozilla8
Assigned To: Nobody; OK to take it and work on it
:
:
Mentors:
Depends on:
Blocks: 572459
  Show dependency treegraph
 
Reported: 2011-07-19 14:27 PDT by (dormant account)
Modified: 2011-07-21 10:09 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Measure cache init times (2.17 KB, patch)
2011-07-19 14:27 PDT, (dormant account)
no flags Details | Diff | Splinter Review
measure cache init (1.62 KB, patch)
2011-07-20 16:49 PDT, (dormant account)
taras.mozilla: review+
Details | Diff | Splinter Review

Description (dormant account) 2011-07-19 14:27:52 PDT
Created attachment 546909 [details] [diff] [review]
Measure cache init times
Comment 1 Jason Duell [:jduell] (needinfo me) 2011-07-19 23:12:54 PDT
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 2 Jason Duell [:jduell] (needinfo me) 2011-07-19 23:13:35 PDT

*** This bug has been marked as a duplicate of bug 670911 ***
Comment 3 (dormant account) 2011-07-20 10:11:46 PDT
(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 [:jduell] (needinfo me) 2011-07-20 11:20:34 PDT
> 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) 2011-07-20 11:43:28 PDT
(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 [:jduell] (needinfo me) 2011-07-20 12:06:53 PDT
Yes.  You've got my +r for that part--go ahead and land it.
Comment 7 (dormant account) 2011-07-20 16:49:16 PDT
Created attachment 547294 [details] [diff] [review]
measure cache init

cut down patch
Comment 9 Marco Bonardo [::mak] 2011-07-21 06:21:16 PDT
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] 2011-07-21 06:21:54 PDT
damn, I meant "don't mark bugs as", sorry just changed keyboard layout :)
Comment 11 (dormant account) 2011-07-21 10:09:53 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.