Closed Bug 672651 Opened 10 years ago Closed 10 years ago

Track cache init times via telemetry

Categories

(Core :: Networking: Cache, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla8

People

(Reporter: taras.mozilla, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch Measure cache init times (obsolete) — Splinter Review
No description provided.
Attachment #546909 - Flags: review?(jduell.mcbugs)
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.
Attachment #546909 - Flags: review?(jduell.mcbugs)
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 670911
(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.
> 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.
(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
Yes.  You've got my +r for that part--go ahead and land it.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
cut down patch
Attachment #546909 - Attachment is obsolete: true
Attachment #547294 - Flags: review+
http://hg.mozilla.org/integration/mozilla-inbound/rev/47c7c675129f
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Summary: Track cache init/deletion times via telemetry → Track cache init times via telemetry
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.
Target Milestone: --- → mozilla8
damn, I meant "don't mark bugs as", sorry just changed keyboard layout :)
(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.
You need to log in before you can comment on or make changes to this bug.