165821 - about:cache should not block the UI thread

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Darin Fisher]

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.

Comment 1

[Darin Fisher]

Attachment: 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 ;-)

Comment 2

[Alexey Chernyak]

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

Comment 3

[Darin Fisher]

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

[Alexey Chernyak]

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)

Comment 5

[Darin Fisher]

Attachment: v1 patch

cleaned up the patch a bit.  added some calls to SetCapacity.

Comment 6

[Alexey Chernyak]

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.

Comment 7

[Darin Fisher]

*** Bug 74818 has been marked as a duplicate of this bug. ***

Comment 8

[Darin Fisher]

-> moz 1.3

Comment 9

[Darin Fisher]

-> future (this patch doesn't help that much, and there are much bigger fish to fry)

Comment 10

[Samir Gehani]

adt: nsbeta1-

Comment 11

[Darin Fisher]

-> default owner

Comment 12

[Wayne Mery (:wsmwk)]

still happens. does patch still need work?

Comment 14

[Valentin Gosu [:valentin] (he/him)]

Attachment: 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?

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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?

Comment 16

[Josh Aas]

Michal - can you help here? Do you know why Valentin's patch doesn't work well for disk devices?

Comment 17

[Michal Novotny [:michal]]

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.

Comment 18

[Josh Aas]

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.

Comment 19

[Valentin Gosu [:valentin] (he/him)]

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.

Comment 20

[Josh Aas]

Sorry, I was very confused there!

Comment 21

[Patrick McManus [:mcmanus]]

is this still an issue?

Comment 22

[Honza Bambas (:mayhemer)]

(In reply to Patrick McManus [:mcmanus] from comment #21)
> is this still an issue?

Definitely not :))  Fixed with bug 916052.