We're updating the issue view to help you get more done. 

Properly determine size of Collections to be cached

Description

DefaultCache.calculateSize does not try very hard to deal with Collections and other
not serializable components. Please note that an object does not have to be java.io.Serializable
to be cached.

A patch attached improves the situation described in the dicussion in two ways:

  • java.util.ArrayList$SubList and collections are handled properly now

  • not serializable objects (within a collection or not) will issue a warning:

org.jivesoftware.util.cache.DefaultCache - Unable to determine size of class java.util.ArrayList instance
with a stacktrace.

A small jython script to demonstrate the issue is attached.

Environment

None

Acceptance Test - Entry

None

Activity

Show:
Daryl Herzmann
December 10, 2011, 4:29 PM

Assigning to Guus for review, please keep the patches coming!

Marcin Marcin
December 11, 2011, 12:25 AM

This patch introduces a new exception everywhere (previous one was breaking the build)

Guus der Kinderen
December 16, 2011, 8:49 PM

Hello Marcin, thank you for your patch! It looks solid, but is missing some details:

  • a serialVersionUID in the new Exception class that you added

  • updated metadata for the clustering plugin. The modifications that you made make the plugin no longer backwards compatible, so we must be sure to enforce that in the plugin.xml! Also, please update the changelog for that plugin.

Other than those two remarks, I've got nothing to add. I was pleasantly surprised by the quality of the patch. Thanks for taking the time to contribute!

Can you please add the last few bits?

Marcin Marcin
December 16, 2011, 11:34 PM

Attached please find another try. Yeah, my Java got a bit rusty it seems.

  • org.jivesoftware.util.cache.CannotCalculateSizeException has now a nice, positive serialVersionUID;

  • updated metadata of the plugin to require Openfire 3.7.2, bumped pluin version to 1.2.1 with the date as of today;

  • move DefaultCache.calculateSize() to CacheSizes.sizeOfAnything() and update consumers to use it as well. Sorry for the name - sizeOfObject() was already taken.

  • replace sizeOfMap(Map<String, String>) with a generic one.

The only downsize of the patch is that it discards all partial correct calculations in case of collections, because we can't throw an exception and keep the calculation result at the same time.

Guus der Kinderen
January 1, 2012, 1:09 PM

Thanks for the update! I've just committed your patch. Have you tried this in production? I wonder how it affects performance (lots of iterations versus better cache management).

Fixed

Assignee

Guus der Kinderen

Reporter

Marcin Marcin

Expected Effort

None

Ignite Forum URL

Components

Fix versions

Affects versions

Priority

Minor
Configure