groovy icon indicating copy to clipboard operation
groovy copied to clipboard

Replacing custom hashmap MetaMethodIndex with standard maps

Open blackdrag opened this issue 1 year ago • 3 comments

This replaces the old custom hashmap variant in MetaMethodIndex with standard maps. Problem though is that this change is a breaking change. MetaMethodIndex itself looks very different now, but is not likely used. There is one protected method in MetaClassImpl which is also used for example by ExpandoMetaClass, which I consider slightly more as problem. Also I think the refactoring needs input on how to improve further

blackdrag avatar Dec 22 '23 00:12 blackdrag

LGTM 🙂

daniellansun avatar Dec 22 '23 07:12 daniellansun

The changes in MetaClassImpl look fine. MetaMethodIndex does not even load it is so different.

Why are the methods deprecated in MetaMethod and ParameterTypes? Are they not useful for other purposes?

Besides replacing with standard types, does this improve performance or memory usage or some other characteristic?

eric-milles avatar Jan 03 '24 17:01 eric-milles

Why are the methods deprecated in MetaMethod and ParameterTypes? Are they not useful for other purposes?

There are two cases, both of them are unused. First there is the ParameterTypes constructor, which loads types based on String. This is something we have to be extra careful with, because this works only if the correct class loader is used. Which means there is only a limited use for this method in specific cases and in general we cannot use it. IMHO it makes no sense to keep this around. Next is the MetaMethod:

   public void checkParameters(Class[] arguments) {
     // let's check that the argument types are valid
     if (!isValidMethod(arguments)) {
         throw new IllegalArgumentException(
                 "Parameters to method: "
                 + getName()
                 + " do not match types: "
                 + FormatHelper.toString(getParameterTypes())
                 + " for arguments: "
                 + FormatHelper.toString(arguments));
     }
 }

for our runtime this method has no use. We have isValidMethod instead, which is also used in this case. The method is just a wrapper around that for error reporting. This could be useful in theory, but I don't see why we should keep it. I mean what is the scenario for it?

Besides replacing with standard types, does this improve performance or memory usage or some other characteristic?

It should be a bit more threadsafe since I use the concurrent hashmap. But I think in department we will have to do quite a bit more work. Otherwise the goal was no improving performance or memory usage but maintainability. It is now much more clear what this structure is compared to before.

blackdrag avatar Jan 04 '24 11:01 blackdrag

Merged. Thanks.

daniellansun avatar Feb 21 '24 13:02 daniellansun