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.
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 ;-)
Severity: normal → minor
Status: NEW → ASSIGNED
Priority: -- → P4
Target Milestone: --- → mozilla1.2beta
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
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.
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)
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
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.
*** Bug 74818 has been marked as a duplicate of this bug. ***
-> moz 1.3
Target Milestone: mozilla1.2beta → mozilla1.3alpha
-> future (this patch doesn't help that much, and there are much bigger fish to fry)
Target Milestone: mozilla1.3alpha → Future
Keywords: nsbeta1 → nsbeta1-
-> default owner
Assignee: darin → nobody
Status: ASSIGNED → NEW
QA Contact: tever → networking.cache
Target Milestone: Future → ---
12 years ago
Priority: P5 → --
still happens. does patch still need work?
6 years ago
Duplicate of this bug: 734407
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?
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.
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 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.
Sorry, I was very confused there!
is this still an issue?
(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
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.