Huge Memory Problems with PropertysetItem and and MethodProperty

Vaadin 6 / 7 / 8 Framework Data Binding
1

Hello,
we have a quite huge application, using Vaadin for a rich UI. We rely heavily on Vaadins BeanItems and MethodPropertys.
Yesterday, I did a memory profiling, with, well, interesting results.
In total, the server uses 7,8GB of Heap Space. 1555MB of that are the real data from our HIbernate data model, then there are chaches, etc.
The interesting thing here is that for enriching that data model with vaadins items and properties, we use another 2,5GB of Heap space. The reasons for that are:
Each property has support for setArgs and getArgs - we never use this feature, but instead of null-initilising the fields, the constructor creates empty object arrays. For each fo our 12 million properties. This means we’ve got
450 MB of unused, empty Object[] in our Heap.

Isn’t that great?
Then, each property holds references to these Object arrays, to the Method’s used for getting and setting, to the object instance and an integer for that setArgument-Feature. For 12 million properties on a 64bit-VM, this equals to 740MB of memory just for the MethodProperty objects.
The BeanItems aren’t really better: They hold a LinkedList to keep the order of which you added properties to an Item. We never care in which order properties were added to an item, but we’ve at least 100MB of LinkedLists in our Heap for that feature for which i can’t see any usage.
And each BeanItems holds an (eagerly initialized) map of all Properties belonging to that Item, so we’ve got another 700MB here.
In total, that’s 1950MB for a GUI Layer on top of a 1555MB data model. That’s definitly too much, and we’d like to increase our usage of properties even more in the future.
So we propose the following solution:

  • Make the setArgument-feature optional - if you want it, use another subclass of MethodProperty, or, if you think of it as core feature, provide a class which implements everything in MethodProperty BUT setArguments, saving us from the cost of two references, one integer and two empty Object arrays in each property.
  • Even for the class with the setArgument-feature, don’t initialize the setArgs and getArgs with an empty array but null to indicate that no extra arguments are provided. The extra logic in the code is well justified by the easy memory savings availably
  • To allow extending and enriching Vaadin MethodPropertys, provide protected getters and setters for internal variables like instance. Also think about removing the private final reference to bean in BeanItem, so subclasses can update the bean of the item whenever they want to - we’d like to do this in case the underlying hibernate-managed bean changed.
  • To get rid of the duplicates of setMethod and getMethod and the property-maps in the items, the proposed solution is a bit more complicated:
    PropertysetItem.getItemProperty works the following way:
    Each Item class computes once the list of properties it has, saving these as PropertyDefs to a TLS. A PropertyDef contains the setMethod, the getMethod and maybe the name of the Property.
    When getItemProperty is invoked, the item looks in it’s WeakHashMap of properties, trying to find the matching property - if there’s a property in it, return it. If there is no property in it, get the HashMap of PropertyDefs from the TLS, lookup the corresponding PropertyDef for the requested Property and construct a lean MethodProperty, just consisting of a reference to the instance of the Item, and a reference to the PropertyDef. This MethodProperty is then saved into the item-specific WeakHashMap and then returned to the caller.
    This allows for very lean items: We only need a WeakHashMap for the properties which are currently in use and one reference to a TLS storage, which contains the PropertyDefs.
    We also have very lean properties: We only need two references for a property + shared PropertyDefs. When using the property, it get’s its getMethod and setMethod from the
    PropertyDef.
    The reason for using TLS here and not a global ConcurrentHashMap is just lock contention - we construct so many items and properties, we can’t allow a lock to come in that critical code path which would problably reduce parallelism by a large amount.

This solution is just theoretical, we haven’t gone through implementing it (yet).
There are two options for us to go forward:
Implement our proposal locally, replacing every instance of BeanItem and MethodProperty with our own solution, probably having problems with the upgrade to Vaadin 7, maintainig even more code, replacing a piece of core code of a framework we use, being not a nice citizen in the OS world, etc.
As you can imagine, we’d not be happy about pursuing that path.
The other option is, obviously, trying to get our solution, or an entire different solution, which nullifies our memory problems pushed into Vaadin Core. Then we are happy, other users will profit from the reduced memory footprint as well, the upgrade path is ensured by the core team, etc.
Because if this, we’d like to hear your feedback - is the outlined solution possible? Where are the faults in our design? What are blockers for implementing this in Vaadin Core? Which version could it go in? While we plan to upgrade to Vaadin 7.0, fixing memory leaks is of higher priority than upgrading to Vaadin 7.0, and waiting for Vaadin 7.1 is definitly not possible, as that would mean we’d run memory efficient code (at least) until March or April - we have a growing user base, we don’t want to throw the memory out of the window.
If you did not understand parts or everything of what I said, don’t hesitate to ask, i’m happy to answer your questions and explain our problems to you.
Thanks for your support, thoughts, etc.
Cornelius

2

While Bean(Item)Container, BeanItem and MethodProperty definitely use more memory than they should, I would recommend using a lazy container if you have this big a data model.

For the memory usage issues, please
create a ticket
 - or perhaps even separate tickets for logically separate issues whose solutions may have side effects. I agree that at least some of these points can and should be addressed, and it helps that you have already done some profiling and identified some of the issues. For instance, the getArgs()/setArgs() functionality is rarely used in BeanItems (maybe more in stand-alone MethodProperties), so this small optimization should definitely be done and might even take less time to implement than writing this forum post. While you might not need the order of properties in a BeanItem, others are likely to need it (e.g. when creating a form). Therefore, I’m afraid it cannot be simply removed, but could perhaps be made slightly more memory efficient some way.

For the possibility to switch the bean in a BeanItem, it does require several other changes as well, but it is doable - one morning a few weeks ago, I hacked together a variant of BeanItem that supports that (about 900 lines of code), but unfortunately never got around to properly testing it or publishing it. If you are willing to test it, I could perhaps attach it here. The implementation does not inherit BeanItem, though, so it is not directly usable in an AbstractBeanContainer but you would need to replace the container as well.

What do you mean by TLS?

Anyway, a fundamentally more efficient approach would be similar to what you suggest: not to have full instances of BeanItem and MethodProperty (including all the metadata etc.) around for all the items, but use the approach taken by IndexedContainer where property and item instances are created on the fly, only contain references to the container and the propertyId and the itemId, and fetch the data from the simple list of beans when requested to do so. Property metadata could probably be shared on the level of the container. IndexedContainer never stores the property instances, so where this gets a little tricky is with listeners, but it is certainly doable by keeping listeners in maps in the container. Also, custom properties for individual items etc. can get tricky with this approach, but VaadinPropertyDescriptor would allow creating them on the fly and keeping them in a map as you suggest, with cleanup based on unregistration.

WeakHashMap, while in many cases otherwise a good solution, cannot be used in the framework as it is incompatible with many clustering solutions.

As for the time table: some of the more local improvements such as the empty Object could be done anytime - it is a private implementation detail. Some of the bigger changes I would not expect for 7.0, though - the schedule is already tight enough and these could potentially cause complications.

I’ll write a separate post on some ways you could perhaps implement some improvements without replacing too much Vaadin code.

3

Part 2: how to bypass some of the problematic parts.

One option would naturally be implementing from scratch your own container, item and property classes based on AbstractInMemoryContainer etc. While it would take some effort, it would allow full customization.

If you prefer not to go quite as far, BeanItem has a (package visibility) constructor that takes a pre-constructed model consisting of a map of VaadinPropertyDescriptors. It is used by AbstractBeanContainer.createBeanItem(beanType) and uses the property descriptors added automatically when the container was created as well as what property descriptors have been added and removed by calls to the protected method ABC.addContainerProperty(String, VaadinPropertyDescriptor) and the public removeContainerProperty(Object).

I would thus suggest subclassing the container so that after it is constructed, you remove all the properties from the model and then add custom VaadinPropertyDescriptors which create customized versions of MethodProperty - a much simplified copy of the class. The property descriptor can also pre-compute e.g. the getMethod and setMethod so that the property instances can share them.

I would expect the custom MethodProperty to be perhaps 200 lines of code, the custom VaadinPropertyDescriptor less than 200 lines (including pre-computing the accessor instances) and the customizations in a subclass of AbstractBeanContainer some tens of lines, with the possibility to borrow most of the code - someone who knows the related mechanisms could probably implement all this in a day. If done this way, it is also very easy to switch back to standard Vaadin code simply by removing some or all of the customizations.

You should always set up the model before adding data to the container to avoid redundant creation and removal of property instances.

4

@Henry,

thanks for the instructive explanations.

what does the addItem() method in IndexedContainer store then? Is there a documentation where to find what IndexedContainer stores exactly? I looked for it but did not find.

In another thread you said “it only stores the data”, and “fetches it during runtime”.
does that mean IndexedContainer is less demanding on memory?

5

Hello,
and thanks for your thorough explanation.

I’ll answer to your general post now and will review the 2nd post wrt our application on monday.

I created yesterday two tickets for the easy stuff, removing the empty Objects and the setArgs/getArgs in an otherwise compatible MethodProperty class.

The order of properties in a BeanItem is not that important to us, if it’s a feature others rely on we can live with the 100MB - if there’d be even more optimized or there’d be an option to disable building of that list entirely, even better, but at the end of the day, it’s just 100MB.

About switching the bean in the BeanItem: I think i’ll open a separate thread for that issue later, as it’s not connected at all with our memory problems, which have obviously a higher priority than that.

TLS is Thread-Local Storage, implemented in Java with the ThreadLocal-class. I’m sorry, i thought that’d be common knowledge.

I proposed sharing the metadata of properties and of items not on a container level (afaik, we don’t always have items in containers, but I’m not a GUI guy, i’m doing the profiling and performance stuff…) but in a hashmap in a threadlocal - this allows for memory- and cpu-efficient sharing of all the metadata. In our configuration, we have up to 400 threads handling user requests, and each of them would get a map if needed and would build the metadata required for the items and properties used by that thread. In the worst case, we’d duplicate the metadata 400 times. As the metadata isn’t that big, that’s no problem - we currently have some hundred of thousands of copies of that metadata, and with a ThreadLocal, we evade all locking overhead - I expect a ConcurrentHashMap to be really slow when hundreds of threads try to read from it concurrently, and items and properties are heavily used, so it’s not just one read from the map per request, but probably thousands.

The only prolem left is the one of property identities - when I use Item.getItemProperty(BLA), i always want the same Property object back. That’s why I used the WeakHashMap, which is lazily initialized with the properties requested, and maintains the property identities as long as needed by the user, and when the property is not used it’s destroyed. As a property itself is very leight-weight (only a reference to the item or to the bean and to it’s PropertyDescriptor) it’s fast to re-create on demand when the user requests the property again. Could you explain why you can’t use a WeakhashMap in Vaadin Core? The only clustering solutions I know about use sticky sessions and don’t involve serializing the whole state of the UI and sending it over the network to another server.
But if it’s really not possible to use a WeakHashMap here, a normal, lazily initialized HashMap would obviously better than the current, eagerly initialized one.

It would be great to get as many of these fixes in as soon as possible, but we also could think about getting in the more involved changes at the beginning of the vaadin 7.1 dev cycle, so that we can backport them to vaadin 7.0 for us, so that we only have to maintain a slightly patched vaadin core till the release of 7.1 - when the roadmap stays the way it is, 3-4 months should be doable on our side, when the changes we apply to vaadin core are not much bigger than replacing some of the vaadin 7.0 classes with classes form the vaadin 7.1 development branch (or even with a snapshot directly after the changes we need went in).

Thanks for yoour efforts :slight_smile:
Cornelius

6

IndexedContainer internally uses a List of property IDs, a Map from property ID to data type and a Map from item ID to a Map from property ID to the property value. The superclass AbstractInMemoryContainer keeps track of a list of item IDs.

Each call to getContainerProperty() returns a new instance of IndexedContainerProperty, which only has the item and property IDs and a reference to the IndexedContainer whose maps it uses to actually fetch and update the data. Likewise, getUnfilteredItem() (used by the superclass) returns a new instance of IndexedContainerItem. Any listeners registered for an IndexedContainerProperty are actually registered in the container in another Map of Map to List.

As there is no other metadata nor redundant copies of such, and the property instances do not need to be stored anywhere, IndexedContainer is often more memory-efficient although the multiple maps may waste a little memory.

I hope this clarifies things enough to enable diving into the source code if you are interested in more details.

7

I guessed that might be the case, but TLS also has a number of other meanings and I did not see the relevance until you mentioned you might have 400 threads accessing the data…

Still, a ConcurrentHashMap might not be a bottleneck as it is only needed to get the instance - might be worth testing before spending more time on a more complex solution.

Among others, Google App Engine (cloud platform) and Terracotta (clustering with failover support etc.) do depend on serialization of the state and do not support WeakHashMap. This probably also applies to other such solutions. Actually in the case of GAE, each request might be served by a different server and full serialization and deserialization is done every time in any case, which makes the performance quite bad especially in cases where the state is big.

I would expect the changes to be quite local so this sounds good.
If necessary, push for these changes to be done as there are many items in our backlog.