Closed Bug 1294183 Opened 3 years ago Closed 3 years ago

Split WRITE level on the HTTP cache IO thread by priority

Categories

(Core :: Networking: Cache, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox51 --- affected
firefox52 --- fixed

People

(Reporter: mayhemer, Assigned: mayhemer)

References

Details

(Whiteboard: [necko-active])

Attachments

(1 file, 2 obsolete files)

I've seen favicon being given a priority over an html file (that didn't make the io shutdown lag timeout just because the favicon entry despite being actually much more important)
Whiteboard: [necko-next]
Assignee: nobody → honzab.moz
Status: NEW → ASSIGNED
Whiteboard: [necko-next] → [necko-active]
- splits the write level
- used the same way as READ/OPEN (via handle->mPriority)
- also moves the MANAGEMENT level (that can have pretty long queues too thanks slow READ operations) right after READ_PRIORITY, it should reduce its queue length and don't have impact on critical performance
- remove the CLOSE = WRITE level (didn't find use)

In general this is a prerequisite for dropping from the low-prio write queue on certain timeout.  Bug for that needs to be filed and discussed first.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=237f8dd560e2
Attachment #8795803 - Flags: review?(michal.novotny)
Comment on attachment 8795803 [details] [diff] [review]
v1 (add and use WRITE_PRIORITY io thread level)

Err.. I must also update the queue length telemetry probe.
Attachment #8795803 - Flags: review?(michal.novotny)
Duplicate of this bug: 1296285
- changes the telemetry probes to new versions (and adds the new level)
- otherwise as the patch v1 comment

https://treeherder.mozilla.org/#/jobs?repo=try&revision=130e6a07e8a6c98e0d2c9442870c190ea5293272
Attachment #8795803 - Attachment is obsolete: true
Attachment #8797947 - Flags: review?(michal.novotny)
Comment on attachment 8797947 [details] [diff] [review]
v2 (add and use WRITE_PRIORITY io thread level)

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

::: netwerk/cache2/CacheIOThread.cpp
@@ +49,5 @@
> +    Telemetry::HTTP_CACHE_IO_QUEUE_2_MANAGEMENT,
> +    Telemetry::HTTP_CACHE_IO_QUEUE_2_OPEN,
> +    Telemetry::HTTP_CACHE_IO_QUEUE_2_READ,
> +    Telemetry::HTTP_CACHE_IO_QUEUE_2_WRITE,
> +    Telemetry::HTTP_CACHE_IO_QUEUE_2_WRITE_PRIORITY,

The two above should be swapped
Attachment #8797947 - Flags: review?(michal.novotny) → review+
Thanks for r!
Attachment #8797947 - Attachment is obsolete: true
Attachment #8800316 - Flags: review+
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c09788a9b52b
Make HTTP cache write leader-class resources with more priority, r=michal
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c09788a9b52b
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
This seems to have added one new histogram.
Please note that data collection review is needed for that (probably from :francois here), possibly for changing the expiry as well:
https://wiki.mozilla.org/Firefox/Data_Collection
Flags: needinfo?(honzab.moz)
Comment on attachment 8800316 [details] [diff] [review]
v2.1 (add and use WRITE_PRIORITY io thread level)

According the docs this should be enough?
Flags: needinfo?(honzab.moz)
Attachment #8800316 - Flags: feedback?(francois)
Comment on attachment 8800316 [details] [diff] [review]
v2.1 (add and use WRITE_PRIORITY io thread level)

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

As far as the changes go (adding email address & bug, bumping the release number), that looks fine to me. Thanks for cleaning this up!

Our guidelines around data collection require more complete descriptions. I notice that all of these probes have the same generic description. I would recommend two things:

- expand the description to specify how each probe differs from the others (i.e. what does "read", "write", "management" mean?)
- add the appropriate units ("number of entries"?)

e.g. Number of entries in the priority HTTP Cache read IO queue.
Attachment #8800316 - Flags: feedback?(francois)
You need to log in before you can comment on or make changes to this bug.