• Bug
  • Status: Open
  • 2 Major
  • Resolution:
  • ehcache-core
  • ljacomet
  • Reporter: cedrik
  • May 29, 2012
  • 0
  • Watchers: 4
  • September 03, 2013

Attachments

Description

While profiling an application using the new size-based cache configuration, we discovered ObjectGraphWalker#walk() consumes too much time. More specifically, the operation taking time is IdentityHashMap resizing.

I will commit a patch soon addressing the problem.

Comments

Alexander Snaps 2012-05-29

Cedrik, Can you also please provide more information about the use case ? How big is the object graph you’re measuring ? In my own measurement, with pretty small graphs, simply adding to the Map was the hotspot… It was on my list to think about what we could do about that, but haven’t come around to that yet. Basically, whatever you’re thinking about, we’d need to make sure it helps in all cases. Looking forward to that information… and your patch ! :) Alex

Cedrik LIME 2012-05-29

Hi Alex,

The object graph is mainly the kind of stuff you find in a HttpSession (usually smallish) and in the ServletContext (a bit more involved).

I was thinking on replacing the {{toVisit}} {{Stack}} with a proper {{Queue}} (a {{LinkedList}}) and initially sizing the {{IdentityHashMap}} to 5000 (in place of default 16). In my own internal tests, the optimal number _in my use case_ is 40000+, but I fear that may be a waste of space for small caches.

What do you think?

Alexander Snaps 2012-05-29

I expected something along those lines… Maybe tracking the avg. size of the object graph might be an option. Basically have the SizeOfEngine auto-adjust to what it gets fed with. Just an idea. I haven’t had time to investigate this much further for now myself.

Cedrik LIME 2012-05-29

The auto-sizing might be an idea, but the assumption would be that all cached objects are of the same size, which may, or may not, be the case. I think capping the auto-size (min=10, max=?) would be essential in any case.

In my experience, objects tend to vary greatly in complexity. It might be a little better in a cache (where, after all, we tend to put similar objects together), but still… different caches, different object types. Maybe we should have one auto-size by cache, but that would add a lot of complexity. It is really worth it?

I will commit my improvements in trunk this week, and will keep this JIRA opened for people to chime in. Feel free to close it should you consider it necessary.

Fiona OShea 2012-05-29

Sending to James to monitor the checkin and review the Patch

Cedrik LIME 2012-06-01

Hi all, The patch has been applied to trunk, using a conservative value of 5000 for the initial HashMap capacity (in place of default 16). Note that this value might still be too low for optimal performance, and an adaptive algorithm based on the class of the object being sized would be a better solution. Can I use a {{net.sf.ehcache.Ehcache}} inside Ehcache code to cache those values? :-)

Tim Eck 2012-06-01

minor FYI, I had to correct some things that your latest change caused with checkstyle (see r5741)

If possible, please run checkstyle before committing.

James House 2012-06-04

I have created http://svn.terracotta.org/fisheye/cru/CR-522 to review this change.

Chris Dennis 2012-06-08

Since the majority of the performance penalty you’re seeing here is resize/rehash related I’m wondering if the following patch preserves the performance improvement you are seeing? It obviously has the advantage of not needing to pushing up the initial size of the map.

Tim Eck 2012-06-08

pretty sneaky patch :-)

Too bad it depends on an implementation detail of IdentityHashMap.putAll() and I can’t think of a good way to write a test for that particular aspect.

Chris Dennis 2012-06-12

The intention would be to write an independent map implementation what exposes this functionality if this solution proves performant enough.

Tim Eck 2012-06-12

sounds good

Alexander Snaps 2012-07-11

This introduced some perf regression in our testing. Also, we think it’s linked to net.sf.ehcache.pool.sizeof.SizeOfTest sporadic failures. Reverting the changeset for now. I’ll see how the mnks behave and probably put the LinkedList back in at some point. I’ll need to put more efforts in the initial sizing issue though…

Alexander Snaps 2013-09-03

There will always be a hotspot though…