review request (L): 7030453: JSR 292 ClassValue.get method is too slow

[John Rose]

On Dec 4, 2011, at 6:25 PM, David Holmes wrote:

>>> In the mean time, all the non-Groovy, non-JRuby, non-Nashorn, etc.
>>> uses of class Class and all the classes not visible in those
>>> environments when they are being used will be larger.
>>> 
>>> Adding the fields may be the right time/space trade-off, but I think
>>> the point merits some discussion given how many Class objects get
>>> created and the relative proportion of Java executions where
>>> ClassValue is currently used.
>>> 
>>> The more reasonable time/space trade-off can change over time of course.
>>> 
>> 
>> I agree but as I said, in that case, I think it's better to take a look
>> to the big picture
>> and see if not only class values fields but also annotations related
>> fields or reflection related fields can be moved.
> 
> This is currently being looked at with a general aim of reducing the size of Class instances. So adding back some size would need strong justification.

At a minimum we need a single word to root the lookup in the Class itself, in order to avoid (a) many extra branches and indirections per lookup and (b) scaling problems associated with centralized hash tables.  (This is the same reasoning as with Thread.threadLocals.)

Since the number of classes in apps is typically in the range 10^3 to 10^4, the extra word is not going to hurt anybody.  The proof of this is that nobody has needed to touch the highly expansive implementation of reflective caches on Class, which was noted by Remi.  These things have existed since March of 2001 without causing enough pain to require fixing, and in those years memory has shrunk in cost by orders of magnitude.

But, in order to respect the "general aim" you are mentioning, I have unhoisted one of the two words from the Class instance itself.  This will cause a minor slowdown in JSR 292 use cases.  This is tolerable, since the next level of speedup will probably come from compiler optimizations, not data structure reorganization.

I have updated the webrev in place with this additional change:
  http://cr.openjdk.java.net/~jrose/7030453/webrev.01

There is a separate mini-diff for the unhoisting, which is trivial:
  http://cr.openjdk.java.net/~jrose/7030453/unhoist-cache.patch

-- John
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/mlvm-dev/attachments/20111207/18720b50/attachment.html