User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7a) Gecko/20040107 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7a) Gecko/20040107 Researching options for minimo: I found that nsICacheVisitor.idl can reduced drastically resulting in about 22K codesize savings. This is not only for Minimo interresting but for the whole of Mozilla. The tricks to simplify the cache visiting interaction (which is ONLY used for about:cache) by: * Replacing nsICacheEntryInfo with nsICacheEntryDescriptor, so that all of *Info stuff can be removed (saving about 15K code (on Linux optimized build)) * In VisitDevice call, not passing an object with the data, but just all 5 params directly in the call, saves about 5K code. Furthermore it saves some interaction back and forth between aboutCache and CacheDevice. * Instead of a DeviceID string, pass back a 'DeviceType' enumeration: only 2 values are used: "disk" and "memory". * Remove GetDescription from CacheDevice, this is for aboutCache to solve (based on 'DeviceType' enumeration. Attaching an example nsICacheVisitor.idl to show this. P.s. I've got a completed, but not really tested yet, patch in my Linux tree. Reproducible: Always Steps to Reproduce:

Note, just to show the reduced nsICacheVisitor.idl Real (but incomplete) patch to be attached soon.

alfred: i'm not convinced that 20k of savings is significant enough here to warrant all these changes to the API. i'd prefer to avoid API changes if possible here. also, please understand that the cache APIs are designed to support pluggable devices (though we do not support them at the moment). this is why the DeviceID is a string. perhaps the solution for minimo is simply to disable some code. we could for example not implement the enumeration methods to save code on minimo, but actually i can imagine that someone might want to develop an extension that uses these APIs. i really believe that we have bigger fish to fry. the cache is already pretty good... i know it can be improved slightly, but 20k is clearly not huge... so, is this worth the API change and reduced flexibility?

Completely remove the classes uses to pass the static data, by directly passing the data itself in the callback. This reduces codesize and memory allocation. Not complete yet, need to complete the nsAboutCache part, and will then report on codesize impact.

alfred: what about comment #3?

Darin, I am still trying to see what this can achieve. Compiled codesize savings sofar: * in netwerk\cache itself is 44698 KiB. * in netwerk\protocol\about 2330 KiB. total: 47028 KiB Which is more than 10% of nsCache, and almost 1% of a complete Firefox dist. Note, see bug 331032, for the removal of the need for a nsICacheEntryInfo.

Simpler patch coming: Only change the VisitDevice and VisitEntry functions, to not use respectively the nsCacheDeviceInfo, nsCacheEntryInfo objects but just passing the data as params. In the current build (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2pre) Gecko/2007112205 Minefield/3.0b2pre), using on VC8, this saves more than 155K object size in nsCache alone. This is more than 10% of the whole cache lib.

Make nsICacheVisitor pass the needed information directly in the callback functions, instead of passing an nsISupport object. There is no functionality change, and this code is only used for the 'about:cache' function, but this saves about 160K objectsize (which is 10% of the cache library), and saves the allocation of these objects...

Also remove the 'FromCacheKey' helper functions, as they now only used in one location.

Less change: 1. don't inline the Client*FromCacheKey functions (yet) 2. keep the nsICacheEntryDescriptor/Entry structure as it is to prevent impact elsewhere.

Requesting 1.9.1 as it is ready to go, except for the last SR, and this is both a sizeable code saving as well as sizeable memory allocation saving.

I guess dcamp doesn't do reviews anymore or something?

I don't think this is going to *block* any release. We should just commit it when it gets reviews.

How can be made sure that some patch is reviewed within at least 1 year or something?

Hm, I'm not sure I like these changes. The signature of visitEntry becomes pretty ugly with this change, and it's also harder to add new data for this function since it would now require updating all implementations.

The implementations of these interface has not been changed for many years. Also with the old code the nsICacheDeviceInfo needs to be extended for new data, which also means that 'all' implementations need to be accommodated for this. The code size savings for this is huge, and will benefit particular Fennec.

Comment on attachment 307667 [details] [diff] [review] V4: Unbitrotted again, and even smaller version It appears to me that this is still a live bug, but blocked on a design-style impasse between Alfred and Christian. Bringing in Shawn for a second opinion, in hopes of breaking the deadlock.

(In reply to comment #18) > (From update of attachment 307667 [details] [diff] [review]) > It appears to me that this is still a live bug, but blocked on a design-style > impasse between Alfred and Christian. Bringing in Shawn for a second opinion, > in hopes of breaking the deadlock. Shawn is not a valid necko reviewer (http://www.mozilla.org/about/owners.html#netlib), so not sure why you're asking him. biesi is module owner, so it's really his call.

(In reply to comment #19) > > Shawn is not a valid necko reviewer > (http://www.mozilla.org/about/owners.html#netlib), so not sure why you're > asking him. biesi is module owner, so it's really his call. I somehow had the impression that this was a history bug. Sorry about that. Still, when we have this clear dispute between patch writer and reviewer, it makes sense to me to bring in a third person. Maybe Darin? I don't want to give Boris even *more* to do.

Darin works on chromium.org and Chrome now, last I heard he has no cycles for Mozilla reviews. /be

ccing some folks who are actually actively working on cache stuff, in case they happen to have opinions. An important question: is the cache going to stick around in current form long enough that the long-term concern about how easy it is to change visitEntry is relevant?

(In reply to comment #22) > An important question: is the cache going to stick > around in current form long enough that the long-term concern about how easy it > is to change visitEntry is relevant? Right now I'm not actively working on replacing necko cache with the google chrome cache (bug #514213). I don't see any activity on bug #512849 either. I'd guess that we'll stick with the current cache for a while.

In that case, it seems like biesi's concerns are pretty relevant...

But, it also seems there will be not other development in the cache domain either, so why keep a very extensive XPCOM interface around just for the eventuality that something needs to be added again? This interface is ONLY used for the 'about:cache' functionality, which is only a kind of debug functionality, and keeping this around, keeps 150K of objectsize around...

(In reply to comment #25) > This interface is ONLY used for the 'about:cache' functionality, which is only And maybe some addons use it too.

If we stick with the current cache in the short term (like 1.9.3), it seems worthwhile to me to take massive improvements like this. And it looks likely that the implementation of a "modern" cache will be sooner than us wanting to change visitEntry in the future, which might be harder because of this change.

Comment on attachment 307667 [details] [diff] [review] V4: Unbitrotted again, and even smaller version Honza, can you take a look?

I believe Michal knows this code better then I do. If he agrees, I can forward r? to him.

Michal is on vacation, I'll take a look then. Do we want this for Aurora?

No.

(In reply to comment #29) > I believe Michal knows this code better then I do. If he agrees, I can > forward r? to him. I could do the review probably on next Monday or Tuesday if you haven't already done it.

Comment on attachment 307667 [details] [diff] [review] V4: Unbitrotted again, and even smaller version Reassigning review to Michal, he (AFAIK) knows this code much better then I do.

Comment on attachment 307667 [details] [diff] [review] V4: Unbitrotted again, and even smaller version > - * Get the id for the device that stores this cache entry. > - */ > - readonly attribute string deviceID; ... > - * Find out whether or not the cache entry is stream based. > - */ > - boolean isStreamBased(); I see no reason to remove these methods from nsICacheEntryInfo. > + const char * key = diskEntry->Key(); > + const char * colon = PL_strchr(key, ':'); > + if (!colon) > + return NS_ERROR_UNEXPECTED; > + nsCAutoString clientID(key, colon - key); Using ClientIDFromCacheKey() and ClientKeyFromCacheKey() would make the code easier to read. > + usageReport.AppendLiteral("</tt></td>\n</tr>\n"); > + // usageReport.Append("<tr><td><b>Files:</b></td><td><tt> XXX</tt></td></tr>"); > > - return NS_OK; > + PRBool keepGoing; > + rv = visitor->VisitDevice(DISK_CACHE_DEVICE_ID, > + "Disk cache device", > + usageReport.get(), I don't like this, we should get rid of the HTML in the cache code. One possible solution is to pass only cache directory and omit inactive size completely. IMO it is totally useless information. > - // Content length This was probably removed by mistake? Anyway, I'm not sure if the code saving is worth the API change.

Given the age of this bug, the general lack of interest from anyone other than Alfred, and the fact that we would get a whole lot more out of replacing the cache altogether, I am inclined to call that a decision and WONTFIX this bug. However, I will wait for Alfred's reaction first.

Is there a bug about replacing the cache with something else?

And I still think this is useful and worthwhile, as it simplifies the cache code, especially the visitor interface, making maintenance of the cache code (or replacing it with some other cache solution) simpler. Also created bug 824925 to merge nsCacheEntryDescription and nsCacheEntryInfo (to also reduce code complexity).

tryrun of V5: https://tbpl.mozilla.org/?tree=Try&rev=af5dcf71473c All builds fails with : Bug 817247 - Intermittent "TEST-UNEXPECTED-FAIL | content/browser/browser/components/preferences/tests/browser_bug410900.js, dom/browser-element/mochitest/test_browserElement_oop_AppFramePermission.html | Exited with code -20 during test run" Problem is in: http://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/in-content/advanced.js New patch coming up...

This one runs green on tryrun: https://tbpl.mozilla.org/?tree=Try&rev=d4abf11fa4ff

Also addressed the review comments: 1. Don't change nsICacheEntryInfo itself. 2. Using ClientIDFromCacheKey() and ClientKeyFromCacheKey() to make the code easier to read. 3. Replaced usageReport with cacheDirectory (removing inactiveSize as it is indeed meaningless).

v8 also passed tryrun: https://tbpl.mozilla.org/?tree=Try&rev=a5b08d9a39a2&pusher=alfredkayser@gmail.com

HTTP cache is currently being rewritten, as well as other caches. Is this bug still valid?

Certainly, because the whole CacheVisitor.idl is really space and code consuming, and is also a clear candidate for code cleanup for a new cache engine.

(In reply to Alfred Kayser from comment #44) > Certainly, because the whole CacheVisitor.idl is really space and code > consuming, and is also a clear candidate for code cleanup for a new cache > engine. nsICacheVisitor.idl is going away with the new cache. It's not using it. There is a new interface: http://mxr.mozilla.org/mozilla-central/source/netwerk/cache2/nsICacheStorageVisitor.idl. As I've quickly looked into the patch here, you are only removing nsICacheDeviceInfo and nsICacheEntryInfo usage from nsICacheVisitor.idl. We definitely remove nsICacheDeviceInfo (no devices in the new cache as well all info passed to the callback as args). However, we still pass the whole entry to onCacheEntryInfo callback. It can be changed to pass the most important entry's properties (new nsICacheEntry) as arguments to onCacheEntryInfo(), but still would be useful to pass the entry itself as well. This is easily doable in the new cache code (the patch will I believe be much much smaller) when you leave the entry as the first arg. See also bug 913828 and it's blockers to remove the whole old cache code.

Thanks for the info, Honza. Just to be clear, this bug will be invalid with the removal of the old cache?

No idea. What is it you exactly want to achieve with this bug?

Hmm.. when looking at the comment 0, it seems like this is just dupe/invalid. Marking so.