Removes map of class to module

[PoTheMagicDragon]

Type of change

• Bug fix
• New feature

Description

Makes it so there's a single source of truth for the modules list. I've been running into an issue creating an addon with multiple modules that have the same class created by a generic module builder.

Related issues

N/A

How Has This Been Tested?

Running Meteor and making sure the modules load correctly.

Checklist:

• My code follows the style guidelines of this project.
• I have added comments to my code in more complex areas.
• I have tested the code in both development and production environments.

[MineGame159]

I mean you are still filtering the list based on the class, so it has same effect as a map, no?

[PoTheMagicDragon]

Kind of. For

    public <T extends Module> T get(Class<T> klass) 

absolutely, but all of the other .values() calls, it will have the correct behavior. Currently the .values() only gets one instance of the multiple modules I've added

[MineGame159]

Ah, well in that case I think it's still good to keep the map purely for the get operation. Searching the list is much slower and retrieving the module is called very frequently.

[PoTheMagicDragon]

On the surface level that's what it would seem, but it's a micro optimization that in all reality probably makes no difference. Take a look at this for example. https://medium.com/@dejangegic100/why-benchmarks-are-lying-to-you-and-hash-maps-are-not-always-faster-than-arrays-ac42cae649a7

[PoTheMagicDragon]

That being said, I can keep it for the get by class portion if you'd prefer

[RacoonDog]

On the surface level that's what it would seem, but it's a micro optimization that in all reality probably makes no difference.

your pull request makes the average runtime of Modules#get roughly 73x slower, up to 107x slower if we take into account the two most popular meteor client addons

now consider that Modules#get is used in 334 different places in the meteor client codebase

[RacoonDog]

i'm fairly curious in seeing what your generic module builder looks like though

[PoTheMagicDragon]

@RacoonDog How did you take those benchmarks?

[PoTheMagicDragon]

I just did some benchmarking and it looks like you're right. It ran a little faster than 73x (I think it was 50 something) but setting up the stream is more resource intensive than I thought. I swapped it out with a plain ol for loop and it was was significantly better but still 5x slower than using the map. I'll update my pr in a bit

[PoTheMagicDragon]

Note, I'm closing this branch due to mangled history from trying to fix accidentally leaking private information