I have improved Hbncontainer implementation, so I want to discuss my changes with someone familiar with architecture of this class and if the changes are ok, I’d like to ask for commit rights for incubator.
The first change is added method updateEntity. It should be used, when user updates entity transparently or instead of explicit update. For example now, if user updates the entity via Hibernate nothing notifies container, that the data it holds are no longer valid.
The second change is about index buffering. In current version the list called “ascRowBuffer” contains whole objects (100 of them), but only their identifiers are needed (because the data itself are cached by weakreferences set “itemCache”). So if someone has big objects (long articles, binary data etc.) this could be really big memory consumer and database time consumer (because we select something we dont need).
The change is simple - projection on id column and some changes in code to make it work with ids not whole objects.
Pavel Micka
edit: one thing I should change now are names of variables and methods using rowbuffer (because now it’s not rowbuffer, but only id buffer :-)) 11223.zip (11.2 KB)
The first improvement is a must have feature. Although you’d most often be using Form and fields, some day you will be modifying the bean directly. The current implementation probably works well in most cases (in Table), but it is not the most elegant solution. Here are some suggestions to do before committing this:
before firing change events, check if the container has that item instantiated or not. If not there is no point to notify that it has changed
instead of firing ItemSetChange event, you should fire Property.ValueChangeEvents. This way Vaadin can in some cases only update a Field in table instead of rebuilding the whole Table. I guess it is quite hard to say which property changed, so it should probably fire it for all properties. Still much better to render N fields instead of the whole table. Another option is to give a variable length parameter of changed property identifiers in the updateEntity method.
The second improvements initially makes some sense too, but it probably (I have not tested) takes some steps backwards too. As said in the field doc of ascRowBuffer, it is there to minimize query count, which is often far greater scalability issue than memory usage. If only identifiers are queried, getItem() method will fire an extra queries soon when the actual item is needed (one query per row in Table is not acceptable). I think the list itself could contain identifiers only, but the query must load all fields. This way it would let GC collect unused items if memory needs to be released, but still keep query count minimal when for example scrolling long Table. However I’d say this change falls into the category of “premature optimization”.
PS. Great work. Big + for using Vaadin coding conventions. Next + I’ll give you for using “svn diff” instead of zipping the modified file
About the second change…I have backtracked the usage of this buffer and the only thing that is used is the ID…everything else is cached by the weak reference buffer. This rowbuffer is used only to cache nextId query (100 of next id’s are stored), so there is no need for using whole objects (the getItem method calls loadItem that searches the weakReferenceBuffer (itemCache)).
The issue connected with this that the user cannot specify the size of this buffer, which would be probably useful in some cases.
Yes, only items are actually needed in that list. But as I tried to explain the container should query all items at once instead of hitting DB several times in rows. One select query instead of N selects of individual rows (+ id list query) is a must have feature considering scalability, saving some bytes of memory in application servlet is a secondary target.
The example application is a bad test case for this due several reasons ( relations without proper query hints, Table is using Container.Indexed interface and not using the related ascrowbuffer at all). You could test it with following code snippet:
System.out.println("Starting listing, query count should stay low");
HbnContainer<Workout> hbnContainer = new HbnContainer<Workout>(
Workout.class, this);
if (hbnContainer.size() != 0) {
Object id = hbnContainer.firstItemId();
int n = 20;
for (int i = 0; i < n && id != null; i++) {
id = hbnContainer.nextItemId(id);
}
}
System.out.println("Listing done");
If we query only the identifiers, total sql query count jumps from 4 to 4 + n.
ROW_BUF_SIZE should be opened for fine tuning, but it would of course require some documentation too.
I have gone through the code again…and you are correct. I think that the problem was in my proposed code, because it really should not make more queries than your implementation (its illogical, because my implemetation uses same data and only is not saving, what isnt needed anywhere).
This was the incorrect part of my code (in correct form) with encomented bug
public Object nextItemId(Object itemId) {
// TODO should not call if know that next exists based on cache, would
// optimize one query in some situations
if (isLastId(itemId)) {
return null;
}
// ==========> this was probably the cause of the queries, because the item
// neednt to be be in cache
//EntityItem<T> item = new EntityItem<T>((Serializable) itemId);
// check if next itemId is in current buffer
List<Object> buffer = getRowBuffer();
try {
//we dont need it...we have it as a method param...really stupid bug :-]
//int curBufIndex = buffer.indexOf(getIdForPojo(item.getPojo()));
int curBufIndex = buffer.indexOf(itemId);
if (curBufIndex != -1) {
Edit: I have the first version (and I hope the final one of the UpdateMethod improvement)…the container should work as a manager for the entities it creates (from database)…so the listeners should register to the container and the container will register listeners for them, when they are created. I think that this is the best option, because the object outside container has no idea about what is referenced and what is not…
I hope that have not made some stupid mistakes…but this works in my app :-))
I dont know why are there some lines with the star sign in the diff…I have updated it before creating diff (without reformatting…)…and the second thing…the forum does not allow *.diff for some reason…so it is again zipped 11235.zip (15.4 KB)
Going to the right direction. Still some suggestions/questions:
Why do you need sharedValueChangeListeners (and HbnContainer implementing ValueChangeNotifier) ? Vaadin components, like Table, attaches property value change listeners to individual properties they display.
Instead of creating new entity item, and updating it to various buffers, you should get the reference to possibly existing entity via the itemCache. Then reset its pojo to the instance given for method (if not the same) and then fire value change listeners of each property.
yes, you are correct…this originated from newly created entity item…I did not realize what this would do, so it has hid from me the Vaadin table feature of registering to Value changes…
here I have little bit different opinion… The pojo exchange instead of creating new entitItem is clear, here I absolutely agree. But I think that the all the bufferes must be updated as well, because the items in them might be loaded by different queries (possibly sessions), so the containing objects are not the same.
I know that these bufferes are there only to cache item id’s, but if they contain more information, it should be consistent with db - or as I have proposed earlier, they should cache only ids…
You are right that the contained objects may not be the same instance, but they reflect to exactly the same data in DB. If you create a new item instead of updating the previous EntityItem instances, you might end up in situation where you have Table displaying old (outdated) EntityItem object and some other component (like Form) having a different (new) instance of EntityItem (referencing the same DB row). There should be only one EntityItem instance per DB row per Container. Eg. the Table wouldn’t event get those valuechange events, because they are fired on the new EntityItem instance whose existence the table has no idea at all => table keeps displaying the old detach pojo.
I have changed the implementation according to your caveat Nr. 1 and developed two versions of the container
Version 1 refreshes all the buffers
Version 2 refreshes only the itemCache
Both should work correctly, but the second one is leaves the buffers unmodified, so it is faster, but there is a potential risk of someone using the stored object in inconsistent state (for example from ascrowbuffer) in future versions of container (in my humble opinion).
(this will happen if someone calls update and then retrieves the object from not updated buffer, but this is possible only for programmer of the container, not for the user…so it is a question wheather to refresh it or not…)
PS: again sorry for the space after stars in svn diff, i dont know where the problem is (probably in my ide)
#############################
edit:
upppps: I have accidentaly overwritten my old post…but there was nothing important…I was there only discussing the two possibilities of what to refresh… 11236.zip (6.97 KB)
Eclipse has changed its formatting settings subtly (but noticeably) quite frequently. There was a change for whitespace after the stars of javadoc comments between Galileo and Galileo SR1. I believe this change is one that you cannot override on the formatting preferences page without also affecting some other formatting.
The SR1 settings (which I guess also SR2 uses - haven’t tested it) are what we are now using in Vaadin. I’m not sure if the incubator version of HbnContainer was formatted with those settings or with an older Eclipse version.
The Vaadin Framework project itself is now be formatted with the SR1 settings.
As those various buffers are practically only used for identifier caching, I don’t see any risk that old pojo reference is taken. EntityItem always gets its pojo reference via session. You’d really need to do something odd in you modified version of HbnContainer to get into inconsistent state.
of the UpdateMethod improvement)…the container should work as a manager for the entities it creates (from database)…so the listeners should register to the container and the container will register listeners for them, when they are created. I think that this is the best option, because the object outside container has no idea about what is referenced and what is not…