Loading classes with many methods is very expensive

[Aleksey Shipilev]

Hi Peter,

On 10/27/2014 03:45 PM, Peter Levart wrote:
> I merged the functionality of the method-signature-key with the
> linked-list node, holding just a reference to Method object and a link
> to 'next' node. I think this is the most compact temporary datastructure
> representation for that purpose that leverages standard LinkedHashMap.

I see. I wonder when somebody come with a stress test using multiple
interfaces with the same signature method :) Seems very unlikely.


>>   * Formatting and logic in MethodList.consolidateWith gives me chills. I
>> think at very least introducing variables for m.getDeclaringClass() and
>> ml.m.getDeclaringClass() improve readability. Also, moving the comments
>> at the end of the line should improve readability and better highlight
>> the boolean expression you are spelling out. Below is the
>> straight-forward rewrite:
>> ...
> 
> Excellent. I'll take your formatting if you don't mind ;-)

Sure. Check if I had captured it right first. I think we need to be more
strict with parentheses to avoid precedence overlooks:
  (A || B && C) should better be ((A || B) && C) or (A || (B && C))


>>    * Should we use just methods.put() here?
>>        2851             MethodList ml0 = methods.putIfAbsent(ml, ml);
> 
> It really doesn't matter as the exception is thrown if (ml0 != null) and
> LinkedHashMap is forgotten...
> 
> if (ml0 == null), put and putIfAbsent are equivalent. I used putIfAbsent
> to make the 3 loops (declared methods, superclass methods,
> (super)interface methods) more uniform.

Okay.


>>
>>    * I wonder if the code would be simpler if we push the Map<MethodList,
>> MethodList> maintenance to some internal utility class? That would
>> probably simplify the loops in privateGetPublicMethods?
> 
> I thought MethodList.add()/consolidateWith() were close to those utility
> methods. I could extract the logic in each of 3 loops to 3 void methods
> taking 'methods' Map and a Method instance, but I don't want to pollute
> the method namespace in j.l.Class any more than necessary and making a
> special inner class for that is an overkill. Previously the holder for
> such methods was MethodArray, but it's gone now. I could subclass
> LinkedHashMap if I wanted to avoid an unnecessary indirection, but
> that's not a good practice. It's not that bad as it is - 38 lines of
> code for 3 loops with comments and spacing. I could improve comments
> though...

Yes. I had to read the entire thing a couple of times to understand how
this works. Containing most of the logic into MethodList seems like a
way to improve this. Not sure though.


Thanks,
-Aleksey.