If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

'decom' of nsICacheVisitor.idl: saves 10% / 150K from nkcache_s.lib

RESOLVED DUPLICATE of bug 913828

Status

()

Core
Networking: Cache
RESOLVED DUPLICATE of bug 913828
14 years ago
3 years ago

People

(Reporter: Alfred Kayser, Assigned: Alfred Kayser)

Tracking

(Blocks: 2 bugs, {footprint})

Trunk
footprint
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.2 -
wanted1.9.2 -
blocking1.9 -
wanted1.9 +

Firefox Tracking Flags

(blocking2.0 -)

Details

Attachments

(1 attachment, 9 obsolete attachments)

(Assignee)

Description

14 years ago
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:
(Assignee)

Comment 1

14 years ago
Created attachment 138896 [details]
Patched nsICacheVisitor.idl to show how it could be reduced

Note, just to show the reduced nsICacheVisitor.idl

Real (but incomplete) patch to be attached soon.
(Assignee)

Comment 2

14 years ago
Created attachment 139187 [details] [diff] [review]
Temporary stuff...

Comment 3

14 years ago
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?
(Assignee)

Comment 4

12 years ago
Created attachment 215181 [details] [diff] [review]
Patch to 'decom' nsICacheVisitor

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.
Attachment #138896 - Attachment is obsolete: true
Attachment #139187 - Attachment is obsolete: true

Comment 5

12 years ago
alfred: what about comment #3?
(Assignee)

Updated

12 years ago
Depends on: 331032
(Assignee)

Comment 6

12 years ago
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.
Summary: nsICacheVisitor.idl can reduced drastically resulting in about 20K codesize savings... → 'decom' of nsICacheVisitor.idl results in at least 47K codesize savings...

Updated

11 years ago
Assignee: darin → nobody
(Assignee)

Comment 7

10 years ago
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.
Assignee: nobody → alfredkayser
(Assignee)

Updated

10 years ago
Status: NEW → ASSIGNED
Flags: blocking1.9?
Keywords: footprint
(Assignee)

Comment 8

10 years ago
Created attachment 290504 [details] [diff] [review]
V1: Real first version: Remove the 'Info' objects from the CacheVisitor

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...
Attachment #215181 - Attachment is obsolete: true
Attachment #290504 - Flags: review?(dcamp)
Flags: wanted1.9+
Flags: blocking1.9?
Flags: blocking1.9-
(Assignee)

Comment 9

10 years ago
Created attachment 292371 [details] [diff] [review]
V2: Unbitrotted after bug 262116

Also remove the 'FromCacheKey' helper functions, as they now only used in one location.
Attachment #290504 - Attachment is obsolete: true
Attachment #292371 - Flags: review?(dcamp)
Attachment #290504 - Flags: review?(dcamp)
(Assignee)

Comment 10

10 years ago
Created attachment 305312 [details] [diff] [review]
V3: unbitrotted again and less changes

Less change:
1. don't inline the Client*FromCacheKey functions (yet)
2. keep the nsICacheEntryDescriptor/Entry structure as it is to prevent impact elsewhere.
Attachment #292371 - Attachment is obsolete: true
Attachment #305312 - Flags: review?(dcamp)
Attachment #292371 - Flags: review?(dcamp)
(Assignee)

Updated

10 years ago
No longer depends on: 331032
Summary: 'decom' of nsICacheVisitor.idl results in at least 47K codesize savings... → 'decom' of nsICacheVisitor.idl: saves 10% / 150K from nkcache_s.lib
(Assignee)

Comment 11

10 years ago
Created attachment 307667 [details] [diff] [review]
V4: Unbitrotted again, and even smaller version
Attachment #305312 - Attachment is obsolete: true
Attachment #307667 - Flags: review?(dcamp)
Attachment #305312 - Flags: review?(dcamp)
(Assignee)

Comment 12

9 years ago
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.
Flags: wanted1.9.1?
(Assignee)

Updated

8 years ago
Blocks: 459117
I guess dcamp doesn't do reviews anymore or something?
Flags: blocking1.9.2?
(Assignee)

Updated

8 years ago
Attachment #307667 - Flags: review?(dcamp) → review?(cbiesinger)

Updated

8 years ago
Flags: wanted1.9.2-
Flags: wanted1.9.1?
Flags: blocking1.9.2?
Flags: blocking1.9.2-

Updated

8 years ago
blocking2.0: --- → ?

Comment 14

8 years ago
I don't think this is going to *block* any release. We should just commit it when it gets reviews.
blocking2.0: ? → -
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.
(Assignee)

Comment 17

8 years ago
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.

Updated

8 years ago
Blocks: 105431
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.
Attachment #307667 - Flags: review?(sdwilsh)
(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.

Updated

8 years ago
Attachment #307667 - Flags: review?(sdwilsh) → review?(darin.moz)
Darin works on chromium.org and Chrome now, last I heard he has no cycles for Mozilla reviews.

/be

Updated

8 years ago
Attachment #307667 - Flags: review?(darin.moz) → review?(bzbarsky)
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...
(Assignee)

Comment 25

8 years ago
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.

Comment 27

8 years ago
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.
(Assignee)

Updated

8 years ago
Blocks: 457262
Comment on attachment 307667 [details] [diff] [review]
V4: Unbitrotted again, and even smaller version

Honza, can you take a look?
Attachment #307667 - Flags: review?(bzbarsky) → review?(honzab.moz)
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.
Attachment #307667 - Flags: review?(honzab.moz) → review?(michal.novotny)
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.
Attachment #307667 - Flags: review?(michal.novotny) → review-
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?
Attachment #307667 - Flags: review?(cbiesinger)
(Assignee)

Comment 37

5 years ago
Created attachment 695994 [details] [diff] [review]
V5: Updated to current tree
Attachment #307667 - Attachment is obsolete: true
(Assignee)

Comment 38

5 years ago
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).
(Assignee)

Comment 39

5 years ago
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...
(Assignee)

Comment 40

5 years ago
Created attachment 696332 [details] [diff] [review]
V7: fix all the tests that use the cacheVisitor

This one runs green on tryrun:
https://tbpl.mozilla.org/?tree=Try&rev=d4abf11fa4ff
Attachment #695994 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Blocks: 824925
(Assignee)

Comment 41

5 years ago
Created attachment 696620 [details] [diff] [review]
V8: Address the review comments as well

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).
Attachment #696332 - Attachment is obsolete: true
Attachment #696620 - Flags: review?(michal.novotny)
(Assignee)

Comment 42

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

Comment 43

4 years ago
HTTP cache is currently being rewritten, as well as other caches. Is this bug still valid?
Flags: needinfo?(alfredkayser)
(Assignee)

Comment 44

4 years ago
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.
Flags: needinfo?(alfredkayser)
(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.

Comment 46

4 years ago
Thanks for the info, Honza. Just to be clear, this bug will be invalid with the removal of the old cache?
Depends on: 913828
Flags: needinfo?(honzab.moz)
No idea.  What is it you exactly want to achieve with this bug?
Flags: needinfo?(honzab.moz)
Hmm.. when looking at the comment 0, it seems like this is just dupe/invalid.

Marking so.
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
No longer depends on: 913828
Resolution: --- → DUPLICATE
Duplicate of bug: 913828
Attachment #696620 - Flags: review?(michal.novotny)
You need to log in before you can comment on or make changes to this bug.