about:cache should not block the UI thread

RESOLVED FIXED

Status

()

Core
Networking: Cache
--
minor
RESOLVED FIXED
16 years ago
2 years ago

People

(Reporter: Darin Fisher, Assigned: valentin)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [necko-backlog], URL)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

16 years ago
about:cache should not block the UI thread.  it currently iterates over all of
the cache entries on the UI thread.  this can easily block the UI thread for
several seconds, maybe longer on machines with slow disks.
(Reporter)

Comment 1

16 years ago
Created attachment 97348 [details] [diff] [review]
v0 patch -- preliminary patch

had this sitting in my tree for a while now... it is mostly done.  i noticed
that OnDataAvailable is not actually sent until the 16k transfer buffer fills
up.  that makes loading of about:cache slightly chunky, but the goal of getting
the work off the UI thread is nonetheless accomplished.  i'd expect the
chunking would be unnoticed in an optimized build, so good enough... especially
since this is only about:cache ;-)
(Reporter)

Updated

16 years ago
Severity: normal → minor
Status: NEW → ASSIGNED
Keywords: mozilla1.2
Priority: -- → P4
Target Milestone: --- → mozilla1.2beta
(Reporter)

Updated

16 years ago
Blocks: 101863

Updated

16 years ago
No longer blocks: 101863

Comment 2

16 years ago
Perhaps use of + operator and NS_LITERAL_[C]STRING instead of all the .Append
functions might reduce the load on massive string memory copying that's
happening here?
http://www.mozilla.org/projects/xpcom/string-guide.html#Concatenation
(Reporter)

Comment 3

16 years ago
yeah, good point... it might be worthwhile to either factor the code to use one
large dependent concatentation (i.e., operator+), or perhaps it'd be enough to
just precalculate the approximate length of the buffer and call SetCapacity up
front.

Comment 4

16 years ago
Would using + reduce used memory ammount? (which is desired for Mozilla).
Might even create a few NS_NAMED_LITERAL_[C]STRINGs (if you can be bothered) for
the ones that are used repeatedly through the code, like:

+        buffer.Append("<td><tt>");
+        buffer.Append("</tt></td>\n</tr>\n");


Pre-pumping the buffer would be very good, there's way too much memory
re-allocation happening for dynamic data even if you use dependant concatenation
for the static markup.
Minimal buffer size can be easily calculated if you know how many cache entires
will be displayed and minimal size of dynamic content (each URL at least has
size of "ftp://b/" and so on)
However minimal size would still be much less than actual size, for URLs tend to
be long, and they do take up the major chunk of each entry, plus HTML escaping
of entities)

Might be a good idea to get a really large (not pre-made, but actually
accumulated over time of browser use) cache, and divide the size taken up by
cache entries by their number, to get average entry size.
That would be user specific ofcoarse, but still a good way to approximate.

And don't forget that about:cache doesn't need buffer pre-pumping, only
about:cache?* does. :o)
(Reporter)

Comment 5

16 years ago
Created attachment 98013 [details] [diff] [review]
v1 patch

cleaned up the patch a bit.  added some calls to SetCapacity.
Attachment #97348 - Attachment is obsolete: true

Comment 6

16 years ago
I checked the size of data about:cache outputs.
The general about:cache page that does not list enties is about 1KB
That makes it about 500 bytes per device.

Entries take up on average 400 bytes each.
These estimates are slightly above average for me, so they give space to wriggle.
(Reporter)

Updated

16 years ago
Keywords: nsbeta1
Priority: P4 → P5
(Reporter)

Updated

16 years ago
Keywords: patch
(Reporter)

Comment 7

16 years ago
*** Bug 74818 has been marked as a duplicate of this bug. ***
(Reporter)

Comment 8

16 years ago
-> moz 1.3
Target Milestone: mozilla1.2beta → mozilla1.3alpha
(Reporter)

Comment 9

16 years ago
-> future (this patch doesn't help that much, and there are much bigger fish to fry)
Target Milestone: mozilla1.3alpha → Future

Comment 10

15 years ago
adt: nsbeta1-
Keywords: nsbeta1 → nsbeta1-
(Reporter)

Comment 11

12 years ago
-> default owner
Assignee: darin → nobody
Status: ASSIGNED → NEW
QA Contact: tever → networking.cache
Target Milestone: Future → ---

Comment 12

11 years ago
still happens. does patch still need work?
(Assignee)

Comment 14

6 years ago
Created attachment 667761 [details] [diff] [review]
v2 patch

This is a first draft of the patch, based on the one by Darin Fisher.
It seems to work well for Memory cache device; however, the browser still freezes when you go to the Disk cache device page. 
This may not be a threading issue, but a locking issue. Any feedback on this?
Attachment #98013 - Attachment is obsolete: true
Hi Valentin. Thanks for the updated patch. It's great to see that you are working on it. I for myself can't give proper feedback on it but I wonder who actually can do it from the networking team.

Josh, do you have someone who can have a look at it for feedback?

Updated

5 years ago
Assignee: nobody → valentin.gosu

Updated

5 years ago
Attachment #667761 - Flags: review?(michal.novotny)

Comment 16

5 years ago
Michal - can you help here? Do you know why Valentin's patch doesn't work well for disk devices?
Comment on attachment 667761 [details] [diff] [review]
v2 patch

Calling nsCacheService::VisitEntries() on a background thread can't solve UI hang since it holds the main cache lock for the whole time. The correct solution would be to also rewrite VisitEntries logic so that it could release the lock while doing IO and before calling the visitor's callback. There is already a similar bug about this (#403860). BTW I don't see a reason why we couldn't do all the nsAboutCache stuff on the cache IO thread instead of creating a new dedicated thread.
Attachment #667761 - Flags: review?(michal.novotny)
Depends on: 403860

Comment 18

5 years ago
Comment on attachment 667761 [details] [diff] [review]
v2 patch

I'd like to have Valentin review this, though we'll technically also need another peer reviewer.
Attachment #667761 - Flags: review?(valentin.gosu)
(Assignee)

Comment 19

5 years ago
Comment on attachment 667761 [details] [diff] [review]
v2 patch

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

This was the patch that I wrote. I'll try to rewrite it following Michal's feedback, and I'll have him review it afterwards.
Attachment #667761 - Flags: review?(valentin.gosu)

Comment 20

5 years ago
Sorry, I was very confused there!
is this still an issue?
Flags: needinfo?(honzab.moz)
Whiteboard: [necko-backlog]
(In reply to Patrick McManus [:mcmanus] from comment #21)
> is this still an issue?

Definitely not :))  Fixed with bug 916052.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Flags: needinfo?(honzab.moz)
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.