Loading classes with many methods is very expensive
Stanimir Simeonoff
stanimir at riflexo.com
Mon Oct 27 11:41:46 UTC 2014
On Mon, Oct 27, 2014 at 1:23 PM, Aleksey Shipilev <
aleksey.shipilev at oracle.com> wrote:
> On 10/27/2014 01:53 AM, Peter Levart wrote:
> > As you might have noticed, the number of methods obtained from patched
> > code differed from original code. I have investigated this and found
> > that original code treats abstract class methods the same as abstract
> > interface methods as far as multiple inheritance is concerned (it keeps
> > them together in the returned array). So I fixed this and here's new
> > webrev which behaves the same as original code:
> >
> > http://cr.openjdk.java.net/~plevart/jdk9-dev/Class.getMethods/webrev.02/
>
> This seems to be a candidate fix for
> https://bugs.openjdk.java.net/browse/JDK-8061950, right? If so, please
> assign yourself there. It might be advisable to start the separate RFR
> thread for a fix?
>
> My comments for the first reading of the patch:
>
> * Why MethodList maintains the linked list? Doesn't O(n)
>
MethodList.add() lead to quadratic complexity again? Should we instead
> use the ArrayList<Method> for storing method synonyms?
>
But N would be the amount of exactly the same overridden methods from
superclasses and declared in interfaces. Usually there would be zero or
1-2. So the in-place linked list conserves memory, the extra indirection of
ArrayList for n=1 would also be slower. IMO, it's a well designed approach.
The only case where it'd actually matter is hundreds of superclasses each
overriding all of the their methods.
Stanimir
> * Please convert some of the indexed loops into for-each loops for
> better readability? E.g:
>
> for (int i = 0; i < intfcsMethods.length; i++) {
> for (Method m : intfcsMethods[i]) {
>
> ...to:
>
> for (Method[] ms : intfcsMethods) {
> for (Method m : ms) {
>
> * I would think the code would be more readable if MethodList.m was
> having a more readable name, e.g. MethodList.base or MethodList.image or
> MethodList.primary?
>
> * 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:
>
> > MethodList consolidateWith(MethodList ml) {
> > Method mineM = m;
> > Method otherM = ml.m;
> > Class<?> mine = mineM.getDeclaringClass();
> > Class<?> other = otherM.getDeclaringClass();
> >
> > if (other == mine //
> other is same method as mine
> > || !Modifier.isAbstract(mineM.getModifiers()) //
> OR, mine is non-abstract method...
> > && (other.isAssignableFrom(mine) //
> ...which overrides other
> > || other.isInterface() && !mine.isInterface())) { //
> ...class method which wins over interface
> > // just return
> > return this;
> > }
> > if (!Modifier.isAbstract(otherM.getModifiers()) //
> other is non-abstract method...
> > && (mine.isAssignableFrom(other) //
> ...which overrides mine
> > || mine.isInterface() && !other.isInterface())) { //
> ...class method which wins over interface
> > // knock m out and continue
> > return (next == null) ? ml : next.consolidateWith(ml);
> > }
> >
> > // else leave m in and continue
> > next = (next == null) ? ml : next.consolidateWith(ml);
> > return this;
> > }
>
>
> * Should we use just methods.put() here?
> 2851 MethodList ml0 = methods.putIfAbsent(ml, ml);
>
> * 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?
>
> Thanks,
> -Aleksey.
>
>
More information about the core-libs-dev
mailing list