Track cache init times via telemetry

RESOLVED FIXED in mozilla8

Status

()

Core
Networking: Cache
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: (dormant account), Unassigned)

Tracking

(Blocks: 1 bug)

unspecified
mozilla8
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

6 years ago
Created attachment 546909 [details] [diff] [review]
Measure cache init times
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
Last Resolved: 6 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 670911
(Reporter)

Comment 3

6 years ago
(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.
(Reporter)

Comment 5

6 years ago
(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 → ---
(Reporter)

Comment 7

6 years ago
Created attachment 547294 [details] [diff] [review]
measure cache init

cut down patch
Attachment #546909 - Attachment is obsolete: true
Attachment #547294 - Flags: review+
(Reporter)

Comment 8

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/47c7c675129f
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 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 :)
(Reporter)

Comment 11

6 years ago
(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.