Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Class search adds significant startup delay #730

Open
kennytv opened this issue Oct 30, 2023 · 4 comments
Open

Class search adds significant startup delay #730

kennytv opened this issue Oct 30, 2023 · 4 comments

Comments

@kennytv
Copy link

kennytv commented Oct 30, 2023

Class search through https://github.com/libraryaddict/LibsDisguises/blob/master/plugin/src/main/java/me/libraryaddict/disguise/utilities/reflection/ClassMappings.java adds a significant amount of time to startup due to scanning a large number of packages instead of storing the actual package location in DisguiseType/passing it in the method calls. For versions that have Spigot's old everything package, you should instead check in that one package, and otherwise use the pre-defined one(s if there is multiple across versions).

Combined and just on startup, this method adds anywhere from 3 to 15 seconds of class search depending on the setup, then more still during uptime.

@libraryaddict
Copy link
Owner

I don't really understand what you're trying to say.
You're aware that class locations have shifted over the versions yes?

@kennytv
Copy link
Author

kennytv commented Oct 30, 2023

Yes, I do. You should pass the exact package locations, considering those will be finite from 1 to 3 at worst instead of spending 10 seconds (!) searching through dozens of packages with every class search in total just on startup

So I would propose doing the following:

  • Throw ClassMappings into the shadow realm
  • Modify your class search method to look something like this
Class<?> findClass(String name, String... packages) {
    for (String package : packages) {
        // try catch around here
        return Class.forName(package + "." + name);
    }
    // another try catch for the old Spigot package
    return Class.forName("net.minecraft.server." + name);
    
    // error here
}
  • Simply change the method calls using the correct package, e.g. by including the package name(s) in DisguiseType to pass on

@libraryaddict
Copy link
Owner

Eh, yeah. Just realized I was looking at the wrong thing.
At the very least, I'm not sure what on earth is up with the packages array as I certainly write that.

That said, you need to check your attitude. Even though you are correct that the code needs to be reworked, your approach was hostile and my first reaction was to want you to go away.
A hostile approach doesn't win you good will.

@libraryaddict libraryaddict reopened this Oct 30, 2023
@kennytv
Copy link
Author

kennytv commented Oct 30, 2023

That is very true, sorry for that; I wanted to make it sound more jesting (or emphasizing) than hostile (https://m.youtube.com/watch?v=YqnS3LKsafY), though I failed. Hopefully you can get that sorted anyways :)

@kennytv kennytv changed the title What the hell is even that Class search adds significant startup delay Oct 30, 2023
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

No branches or pull requests

2 participants