Do we have any data that the accessibility cache uses a significant amount of memory? I now have a tool that identifies unreported memory (bug 676724) that will drive the creation of new memory reporters. If you tell me what functions are responsible for allocating this memory I can check my profiles to see if they appear.

(In reply to Nicholas Nethercote [:njn] from comment #1) > Do we have any data that the accessibility cache uses a significant amount well, I for one don't have any great gues. Note that you should need to have accessibility enabled for it to even exist. > of memory? I now have a tool that identifies unreported memory (bug 676724) > that will drive the creation of new memory reporters. If you tell me what > functions are responsible for allocating this memory I can check my profiles > to see if they appear. well, I'm not sure of the total list off hand, but certainly most of the methods on nsAccessibilityService. In any case I think doing this would probably be useful to us, so the anser is most likely just going to help you prioritize :)

Comment on attachment 552375 [details] [diff] [review] correct patch Review of attachment 552375 [details] [diff] [review]: ----------------------------------------------------------------- What numbers did you see in about:memory with this patch -- how big is the accessibility cache? ::: accessible/src/base/Statistics.cpp @@ +50,5 @@ > + inline PRUint64 AccObjectsTotalSize() { return gAccessiblesMemory; } > + > + NS_MEMORY_REPORTER_IMPLEMENT(AccObject, "a11y/accessible-objects", KIND_HEAP, > + UNITS_BYTES, AccObjectsTotalSize, > + "total size of all accessibles") The path should be "explicit/a11y/accessible-objects". Please make the description a complete sentence, e.g. "Memory used by all accessibles." BTW, what is an "accessible"? Oh, it's a type. So "Memory used by all nsAccessible objects." would be a better description. And this memory is definitely allocated on the heap (i.e. via malloc or operator new), yes? That's very important if it's marked with KIND_HEAP. ::: accessible/src/base/Statistics.h @@ +52,5 @@ > + /** > + * called when an accessible is created. > + */ > + inline void AccessibleAdded(PRUint32 aSize) > + { gAccessiblesMemory += aSize; } There are two ways to implement memory reporters. The first way is to maintain a global counter, as you've done, that gets incremented/decremented every time memory is allocated/freed. The second way is to trace over a data structure, counting the size of all its elements. The second way is preferable when possible, because it's less error-prone. We've had problems in the past with memory reporters of the first kind, where the increment/decrement points weren't maintained properly and we ended up with bogus numbers. Does the second way make sense in this case?

Oh, you've forgotten one crucial thing -- you haven't registered the memory reporter, so it's not being used. You need to call NS_RegisterMemoryReporter. Do it from somewhere related to the start-up of the accessibility sub-system. If that sub-system can be shut-down without the browser shutting down, you'll also need code to unregister the memory reporter -- use NS_UnregisterMemoryReporter. BTW, the incorrect path and the lack of registration shows that this patch doesn't work, and that you haven't tested it. It's poor form to ask for review on a patch like that without any explanation of it being incomplete.

Comment on attachment 552375 [details] [diff] [review] correct patch ok, thanks, I'll try to fix this patch up soon

(In reply to Nicholas Nethercote [:njn] from comment #1) > Do we have any data that the accessibility cache uses a significant amount > of memory? I now have a tool that identifies unreported memory (bug 676724) > that will drive the creation of new memory reporters. If you tell me what > functions are responsible for allocating this memory I can check my profiles > to see if they appear. per each document accessible (nsDocAccessible class) we keep two hashes of accessible objects (nsDocAccessible::mAccessibleCache and nsDocAccessible::mNodeToAccessibleMap), they can be quite big. Also accessible object are organized into one tree (in single process firefox the root of tree is nsApplicationAccessible that can be received from nsAccessNode::GetApplicationAccessible()). What's the proper way to organize memory reports for these?

Btw, sizeof(this) is always 4 :) So that you count the number of accessible rather than memory used by accessible objects. On the another hand sizeof(*this) doesn't give you correct numbers because accessible object keeps pointer to owned structures like AccGroupInfo or array of children. They should be counted too.

(In reply to Nicholas Nethercote [:njn] from comment #5) > There are two ways to implement memory reporters. The first way is to > maintain a global counter, as you've done, that gets incremented/decremented > every time memory is allocated/freed. The second way is to trace over a > data structure, counting the size of all its elements. > > The second way is preferable when possible, because it's less error-prone. > We've had problems in the past with memory reporters of the first kind, > where the increment/decrement points weren't maintained properly and we > ended up with bogus numbers. Does the second way make sense in this case? We could do second apporach by nsDocAccessible::mAccessibleCache traversal for each document accessible. It contains all accessibles we maintain. But AT can continue keeping them alive when we removed them from mAccessibleCache. Anyway we can follow this way due to ongoing MSAA reorgs (and ATK if necessary). What alternatives we have to count the size of the object?

(In reply to alexander surkov from comment #8) > > What's the proper way to organize memory reports for these? Sorry, what do you mean by "organize" -- how they should be presented in about:memory? > What alternatives we have to count the size of the object? If you can use the "traverse a data structure" approach, that's better, but if you can't, the "maintain a global counter" approach is acceptable.

(In reply to Nicholas Nethercote [:njn] from comment #11) > (In reply to alexander surkov from comment #8) > > > > What's the proper way to organize memory reports for these? > > Sorry, what do you mean by "organize" -- how they should be presented in > about:memory? yes, any rules and hints to find ineffective memory usage. I guess that could be a tree like a11y crossplatform part accessible tree (all alive accessible objects) accessible objects hashes platform parts atk accessible tree MSAA accessible tree XPCOM accessible tree > > What alternatives we have to count the size of the object? > > If you can use the "traverse a data structure" approach, that's better, but > if you can't, the "maintain a global counter" approach is acceptable. Any example of "traverse a data structure" approach to not invent a wheel? Should it be virtual method that return size of the object or are there other ways?

(In reply to alexander surkov from comment #12) > > yes, any rules and hints to find ineffective memory usage. I guess that > could be a tree like > a11y > crossplatform part > accessible tree (all alive accessible objects) > accessible objects hashes > platform parts > atk accessible tree > MSAA accessible tree > XPCOM accessible tree I don't know how the accessibility data is structured, so you'd be in a better position to know this. However, I'd suggest you just have a single "explicit/a11y" bucket to begin with. If that gets large enough that you start wanting to know more, then think about splitting it up. > Any example of "traverse a data structure" approach to not invent a wheel? > Should it be virtual method that return size of the object or are there > other ways? It doesn't need to be a virtual method necessarily. As for reinventing wheels, you'll probably need to write some accessibility-specific code for this (unless you're using very standard container types like PLArena). But the new code shouldn't need to be very complicated.

(In reply to Nicholas Nethercote [:njn] from comment #13) > It doesn't need to be a virtual method necessarily. As for reinventing > wheels, you'll probably need to write some accessibility-specific code for > this (unless you're using very standard container types like PLArena). But > the new code shouldn't need to be very complicated. The link on example in Mozilla code how the object size is calculated would be great. Especially how to bypass "having a virtual method" requirement, because it's not clear for me how to get the size of the object having pointer to a base class.

https://wiki.mozilla.org/Platform/Memory_Reporting has some documentation on writing memory reporters that might be helpful, including details on measuring classes that involve inheritance.

Alex / Trevor, I've been tinkering with this for a few days and figured it was time to report status and ask for you to look it over. Please understand this as a proof-of-concept for us, nowhere ready for prime time. The goal is simply to get something up and successfully reporting on the about:memory page. Once that's done, I'll expand the process with actual useful information. This builds on what Trevor started originally. Currently I'm just trying to get a running total of our active accessibles displayed. So I'm: 1) Centralizing the accessibilty memory collecting / reporting methods in our Statistics module 2) Initializing the provided memoryreporter system and performing our register / unregister calls to it from the root of our nsAccessibiltyService subsystem 3) Calling the accesibility memory increment / decrement methods from the nsAccessible contructors and destructors For this simple example, I'm using the memoryreporter KIND_OTHER option, and treating accessible item counts as UNITS_BYTES. While this builds, it does not work. The browsers about:memory page works fine, until I start the nsAccessibilityService subsystem by starting the WEB Developers DOM Inspector. After that, refreshing the about:memory page shows an empty oval area where the "Main Process, Explicit Allocations, and etc." information normally appears. Please provide whatever feedback, and I'll continue to tweak ahead with it -- mark

Comment on attachment 616053 [details] [diff] [review] Patch (v1) Review of attachment 616053 [details] [diff] [review]: ----------------------------------------------------------------- The "empty oval area" means that the JS code that generates about:memory (toolkit/components/aboutmemory/contents/aboutMemory.js) threw an exception. Open the error console to see what it is. Hopefully the message will be clear enough to make the problem obvious. If not, feel free to paste it here and I'll try to help decipher it. ::: accessible/src/base/Statistics.cpp @@ +47,5 @@ > +namespace statistics { > + > +} // namespace statistics > +} // namespace a11y > +} // namespace mozilla This file doesn't seem to be necessary. ::: accessible/src/base/Statistics.h @@ +78,5 @@ > + > + /** > + * Called when accessible memory reporting is started. > + */ > + inline void AccObjectsInit() { gAccessiblesMemory = 0; } Since gAccessibleMemory is static it will be initialized to zero by default, so this isn't necessary. If you want to make the initialization explicit, just assign 0 to it in the declaration above. @@ +83,5 @@ > + > + /** > + * Called when accessible memory is queried. > + */ > + extern inline PRInt64 AccObjectsTotalSize() { return gAccessiblesMemory; } "Size" kind of implied that the unit is bytes. "AccObjectsCount()" might be a better name. ::: accessible/src/base/nsAccessibilityService.cpp @@ +110,5 @@ > using namespace mozilla::a11y; > > +// Initialize A11y memory reporting service > +NS_MEMORY_REPORTER_IMPLEMENT(AccessibilityObjects, "accessibility-object-count", > + KIND_OTHER, UNITS_BYTES, statistics::AccObjectsTotalSize, UNITS_COUNTS would be better here. However, is there a reason why you're counting the objects instead of summing their sizes in bytes? Bytes are more useful in general. @@ +1316,5 @@ > > NS_ASSERTION(!gIsShutdown, "Accessibility was shutdown already"); > > + // Unregister A11y memory reporting service > + NS_UnregisterMemoryReporter(new NS_MEMORY_REPORTER_NAME(AccessibilityObjects)); This code creates a new nsIMemoryReporter and then tries to remove it from the list of registered reporters. You instead need to pass in the old reporter that you created in nsAccessbilityService::Init(). See extensions/spellcheck/hunspell/src/mozHunspell.{c,h} for an example.

Nicholas, thanks for the above! From examining the exception thrown in the error console I saw that the description mentioned in comment#5 used by NS_MEMORY_REPORTER_IMPLEMENT does have to be "a complete sentence", as in an english-language sentence. Mine wasn't terminated by a "period" (Doh!) and failed. That's fixed now. Now I need to talk with Alex and Trevor further. Merely incrementing / decrementing a count in our nsAccessibles constructor / destructor isn't providing useful information. About:memory always shows zero accessible items. Could be a timing issue or something else stupid on my part. In addition, I'm also looking towards your more complete solution of cache traversal for accurate memory reporting. We have three main caches that I've found, the MS-AA sHWNDCache, our mDocAccessibleCache, and our mAccessibleCache. Each tree could be scanned for item counts and totaled together, then further logic developed to refine the memory counts.

> In addition, I'm also looking towards your more complete solution of cache > traversal for accurate memory reporting. We have three main caches that I've > found, the MS-AA sHWNDCache, our mDocAccessibleCache, and our > mAccessibleCache. > > Each tree could be scanned for item counts and totaled together, then > further logic developed to refine the memory counts. if you were to take this approach you'd want to iterate over the accessible documents in nsAccDocManager::mDocAccessiblecache, and for each document iterate over the accessibles in its mAccessibleCache. You could probably reuse some of the traversal stuff already used for cycle collection for this. The disadvntage of this approach is that you miss accessibles that have been detached from the document, but are still held alive for one reason or another. That should be less of an issue for internal objects as work progresses, but we can't stop it from being an issue where platform objects being keep alive by API consumers aren't counted.

(In reply to Trevor Saunders (:tbsaunde) from comment #19) > > In addition, I'm also looking towards your more complete solution of cache > > traversal for accurate memory reporting. We have three main caches that I've > > found, the MS-AA sHWNDCache, our mDocAccessibleCache, and our > > mAccessibleCache. > > > > Each tree could be scanned for item counts and totaled together, then > > further logic developed to refine the memory counts. > > if you were to take this approach you'd want to iterate over the accessible > documents in nsAccDocManager::mDocAccessiblecache, and for each document > iterate over the accessibles in its mAccessibleCache. You could probably > reuse some of the traversal stuff already used for cycle collection for this. that's the way I would go. you need to provide virtual method that traverses the object and reports its size. > The disadvntage of this approach is that you miss accessibles that have been > detached from the document, but are still held alive for one reason or > another. That should be less of an issue for internal objects as work > progresses, but we can't stop it from being an issue where platform objects > being keep alive by API consumers aren't counted. yes, this is unfortunate but if we don't have internal leaks then this info is rather useless since we can do nothing when AT keeps the object alive that should go away.

I'd be okay taking a patch that provides us *some* information... which I think is better than nothing. For example someone (with a11y instantiated) told me their FF was ballooning memory recently and it would be good to know if it was in a11y.

FYI, this is as far as I got with the patch ... before my short attention span wandered off to other bugs ... it's still more of a proof of concept than anything useful ... but it's something to hang code on. Fire up the browser, open a few tabs, open about:memory, scroll down to "Other Measurements" and the first entry shows "canvas-2d-pixel-bytes" entry ... open the Tools/WebDev/DOMInspector and refresh the about:memory page and the first entry displayed is now labelled "accessibility-object-count", info displayed about the number of entries in the Document Accessible Cache (mDocAccessibleCache). I thought it would be basically the number of open tabs, but I see there's +1 for the root document, and +1 per tab, though when some tabs open (for example the below) +2 entries are created/released in the cache. https://bugzilla.mozilla.org/buglist.cgi?emailcc1=1;emailtype1=exact;query_format=advanced;emailassigned_to1=1;email1=markcapella%40twcny.rr.com;list_id=3217805 The next step could be to add further traversal logic to total all the accessibles in its cache (mAccessibleCache) per Document in cache. And of course, this is still counting items, versus counting total item sizes. If someone can get there faster than I, or has a better approach, please feel free to run with it.

Ack, there's a lot of blue lines in the diff that look like I didn't change anything ... I forgot I ran a test of this patch through my automatic whitespace eliminator.

Freeing up items I'm not active on @ the moment ...