Skip to content

Replacing custom hashmap MetaMethodIndex with standard maps#2020

Merged
daniellansun merged 1 commit intomasterfrom
feature/MetaMethodIndex_refactor
Feb 21, 2024
Merged

Replacing custom hashmap MetaMethodIndex with standard maps#2020
daniellansun merged 1 commit intomasterfrom
feature/MetaMethodIndex_refactor

Conversation

@blackdrag
Copy link
Copy Markdown
Contributor

@blackdrag blackdrag commented Dec 22, 2023

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 blackdrag marked this pull request as draft December 22, 2023 00:35
@daniellansun
Copy link
Copy Markdown
Contributor

LGTM 🙂

@eric-milles
Copy link
Copy Markdown
Member

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?

@blackdrag
Copy link
Copy Markdown
Contributor Author

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 blackdrag marked this pull request as ready for review February 5, 2024 13:59
@blackdrag blackdrag self-assigned this Feb 5, 2024
@daniellansun daniellansun merged commit af41551 into master Feb 21, 2024
@daniellansun
Copy link
Copy Markdown
Contributor

Merged. Thanks.

@daniellansun daniellansun deleted the feature/MetaMethodIndex_refactor branch March 29, 2025 11:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants