XUL Prototype cache holds all loaded xul/css/xbl There is no eviction policy defined yet. I am thinking of something like : "if a held object isnt used in the last 5 mins, release it".

I just wanted to file a new bug, but I checked first for something similar. I think we definetely need this to reduce memory footprint. There is no need to keep rarely used dialogs in cache. We can even add pragma 'no-cache' to not cache some documents at all (bug 46183). I'll try to get some data to make a picture how much memory prototype cache typically takes. This bug already has nsbeta1+, but I think it should be retriaged first So nominating for Buffy.

Just found another similar bug 53313.

from bug 53313: Need to hook up memory flusher for XUL prototype cache. Unused prototypes typically take up 10's to 100's of KB of memory, depending on how many dialogs and windows have been used.

Nav triage team: nsbeta1+/adt2

I'd like to add this first.

Sorry for the spam -- review in a second. /be

Comment on attachment 106228 [details] [diff] [review] patch to flush XUL cache in low memory conditions sr=brendan@mozilla.org, Ben can r=. Did you get a chance to test this? The problem with memory-pressure is that on modern OSes, you will probably swap to death before it can do any good. /be

Hrm, shouldn't bug 53313 get the patch? Or else dup it against this one (better at this point). /be

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

To answer the previous question, I haven't tested in real low memory conditions, but I forced the flusher thread to flush every minute. It worked as expected. New windows were noticably slower after a flush. Btw, this is the way we do it in the XBL and String Bundle Service.

This patch is work in progress, I still want to land first patch in the meantime So what does this patch ? I enhanced the memory flusher thread a bit to notify observers with a memory-checkpoint topic every minute. When the XUL cache gets this notification it tries to find and release unused prototypes and style sheets. I still wonder how to make it less aggresive. Thoughts ?

Cc'ing some xpcom buddies for the memory flusher changes. They look ok to me, although I wonder whether the period should be 1 minute? It might be better to run the memory-pressure action with the flush-unused reason every 10 minutes, if only to avoid hurting certain benchmarks (pageload tests?). Also, and this must depend on nsIObserver, a nit: why is aAction a char string while aReason is a PRUnichar string? I think we want some hysteresis: if a cached xul prototype is referenced only by the cache, but was used 1 minute ago, it shouldn't be evicted as aggressively as a single-referenced prototype that was used 10 minutes ago. That suggests keeping a lastUsed timestamp in the prototype cache, setting it (somehow) when Release drops the ref-count from 2 to 1, and filtering based on it. /be

ooh, I like this. One simple weighing system would be to multiply the number of refs times the unused-time.. or maybe some variation on that..

Ok, I changed it to use the memory-pressure action and the flush-unused reason, and the interval from 1 minute 10 minutes. But I'm not sure how to track ref-count when it drops from 2 to 1. One possible way would be to implement special AddRef and Release for prototype documents and style sheets.

Ok, this is almost complete patch. I need to patch nsICSSStyleSheet to support nsICachedObject.

ok, this one is ready for r/sr

I already added NS_COM to nsCachedObject.h: -class nsCachedObject : public nsICachedObject +class NS_COM nsCachedObject : public nsICachedObject and also added nsCachedObject to dlldeps.cpp

Yikes, dougt needs to be in the loop on xpcom changes. Do we really need a new XPCOM interface with only one method? /be

Hmm, looks like dougt is on vacation Scc, alecf, could you take a look ? Thanks

Well, I haven't had enough time to go over this in great depth, but I have to wonder if we really need to add this to XPCOM, or add a whole interface for this. Here's a thought: make nsCachedObject implement nsIObserver, and call this->Observe(...) with some well known topic when the refcount == 1 I'm also not a fan of tons of calls to PR_Now() - each of these is a system call, and if we're calling this many times on each cached object, I fear performance problems. In global history, I did some extra work to cache a recent value of PR_Now() so I wasn't calling it constantly.. I sometimes wonder if it was overkill there, but here it looks much more important.

I do agree with Alec and bet that this functionality can be flattened into the nsIXULPrototypeDocument. Also, it may be a moot point, but isn’t the interface implemented incorrectly. Only certain addref's modify the last usage time.

Ok, I'll try to add this functionality to nsXULPrototypeDocument and nsCSSStyleSheet directly.

peterv and I discussed the eviction policy of unused entries. He came with a clean and simple solution to just keep one more reference to cached objects. So when the timer fires (each 10 mins) it will check for objects with 2 or 1 reference. Example: TimerHandler() { NS_ADDREF(obj); NS_RELEASE2(obj, &refcnt); if (refcnt == 2) { // consider this object as unused NS_RELEASE(obj) } else if (refcnt == 1) { // the object has already been considered as unused // remove it from the hashtable hashtable.Remove(&key, obj); } } PutObject(obj) { hashtable.Put(&key, obj); // additional addref NS_ADDREF(obj); } GetObject(*obj) { ... if (NS_ADDREF(*obj) == 2) { // don't consider this object as unused anymore NS_ADDREF(*obj); } } So we don't need a new interface nor calling PR_Now(). I'll try to rework my patch when I get to it (currently busy with bookmarks)

There is only one technical problem with getting number of references in TimerHandler. It's a bit ugly that we have to call NS_ADDREF and then NS_RELEASE2 to get the ref count.

Comment on attachment 116960 [details] [diff] [review] new patch this patch has an issue

Comment on attachment 116979 [details] [diff] [review] another try On first pass, you need WAY more docs/comments about all this NS_ADDREF/NS_RELEASE2 madness - explain who's holding the references such that its safe to call NS_RELEASE twice, and so forth.

ok, so who is willing to r/sr this stuff ?

for your reviewing pleasure

Comment on attachment 117064 [details] [diff] [review] patch with improved grammar, thanks glazou alec, please take a look

Comment on attachment 117064 [details] [diff] [review] patch with improved grammar, thanks glazou I don't understand what this means: + The cache listens to "memory-pressure" notifications and is flushed + accordingly to more precise type of notification (message). Please create an inline function which returns the reference count of any nsISupports. Then replace this pattern: + NS_ADDREF(object); + NS_RELEASE2(object, refcnt); Have you considered not adding aflush unused event but instead have your implementation ignore events? For example save the number of times Observer() has been called and when that number mod 10 is zero, do flush the unused? I would hope that you can update your patch to do this. Post a new patch and I will review the rest in more detail.

>I don't understand what this means: > >+ The cache listens to "memory-pressure" notifications and is flushed >+ accordingly to more precise type of notification (message). The memory implementation sends "events" using the observer service. The XUL cache registers as an observer for "memory-pressure" topic. The observer service also enables passing some data. The aSomeData argument is actually a PRUnichar, so I called it a message in this context. See: http://lxr.mozilla.org/seamonkey/source/xpcom/base/nsMemoryImpl.cpp#190 http://lxr.mozilla.org/seamonkey/source/xpcom/base/nsMemoryImpl.cpp#454 "More precise type of message" is actually aSomeData argument. We need it to know if we should flush cache completely or only flush unused objects. Does it make more sense now ? I can change the comment to something like this: The cache registers as an observer for "memory-pressure" topic and is flushed accordingly to passed message. >Please create an inline function which returns the reference count of any >nsISupports. Then replace this pattern: >+ NS_ADDREF(object); >+ NS_RELEASE2(object, refcnt); good idea, will do >Have you considered not adding aflush unused event but instead have your >implementation ignore events? For example save the number of times Observer() >has been called and when that number mod 10 is zero, do flush the unused? I >would hope that you can update your patch to do this. Actually we don't get any events at the moment, only the memory-pressure/low-memory event when we are low on memory. So it's not possible to count these events and even more I was told it's not good idea to send the event every minute, because it could affect page load time etc. Also we can't just flush after 10 minutes, because it could lead to flushing objects which were unused only for a minute. Please take a look at comment #12.

- changed the comment - inlined GetRefCnt()

Comment on attachment 117279 [details] [diff] [review] patch incorporating some of dougt's suggestions to be honest, I'd like to hear brendan's thoughts on this before we procede - all this fiddling with refcnts gives me the willies. I am personally much more a fan of out-of-band queries/events to figure out if something is unused or not.. but I don't know the architecture of this stuff well enough to suggest anything.

Alec, we wanted something relatively simple, w/o adding overhead. I think it's a simple and clear solution. I was quite confused when peterv tried to explain me the design, but once I figured out how it works I started liking it.

Jan Varga wrote: "Also we can't just flush after 10 minutes, because it could lead to flushing objects which were unused only for a minute." In your patch you mention the cache gets this notification every 10 mintues, and you mention that you addref twice, but you don't mention this specific sentence. To those who are new to this code, or dopes like me who forget things after a few days, I suggest you add some variant of this sentence after your comment on the double-addref. I am able to understand what this implementation is doing even after being away from the XUL PD stuff for about a year, so it seems nicely documented and relatively simple. Jan tells me he's matched addrefs to releases with printfs and nothing is leaking. As long as the nsIXULPrototypeCache remains the only gateway to these objects there "shouldn't be a problem." Like Alec, I am interested to hear what Brendan thinks :-)

I'm interested in Jan updating this bug with some results from testing he did today, which he told me about over IRC ;-). /be

I've just found something interesting, not directly related to this patch, let me investigate.

I filed bug 198316 for the different issue I found. Brendan suggested to test 2 things: 1. Force memory pressure at odd moments, watch xul re-load from fastload file 2. Try to make the cache flush race with cache re-fill due to new xul window being opened Results: 1. I found that I forgot to release doubly addrefed objects under memory pressure, it's fixed in my new patch. Otherwise it works as expected. Opening of new windows is noticeable slower after full flush. There is only one assertion: ASSERTION: The prototype binding has somehow lost its XBLDocInfo! But that happens even w/o my patch and we don't release unused XBL doc info. 2. Here I found another assertion, it's also fixed in my new patch: ASSERTION: URI mapped to two different specs?: 'uriMapEntry->mDocMapEntry This happens only sometimes after a xul protype doc has been removed from the cache. I debugged it a bit and it seems that protoDoc->Read() sometimes fails in GetPrototype() method http://lxr.mozilla.org/seamonkey/source/content/xul/document/src/nsXULPrototypeCache.cpp#271 I think we should call EndMuxedDocument even if Read failed.

not going to make 1.4alpha

Nav triage team: nsbeta1-

Anyone wanting to revive this one? It this still something worth to do?