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

Add new space index: Jandex #106

Open
cstamas opened this issue Jan 8, 2024 · 8 comments
Open

Add new space index: Jandex #106

cstamas opened this issue Jan 8, 2024 · 8 comments

Comments

@cstamas
Copy link
Member

cstamas commented Jan 8, 2024

Jandex https://smallrye.io/jandex/jandex/3.1.6/core/browsing.html seems pretty much similar to Sisu Index but it does not use ASM and seems faster.

Could sisu be made to:

  • support jandex indexed JARs?
  • have a ClassFinder that supports Jandex index?
  • decouple ASM from SpaceScanner?

Basically "jandex" could be just another provider for index, with probably alt set of Space related classes and could this remove dep of Sisu on ASM?

@kwin
Copy link
Contributor

kwin commented Jan 8, 2024

I think using a Jandex index for just Sisu (which just needs all FQCNs of classes with a specific annotation) is way to much overhead. But maybe there are use cases where that index is already provided and used eslsewhere? @cstamas Can you elaborate a bit on the use case here?

@cstamas
Copy link
Member Author

cstamas commented Jan 9, 2024

No use case, was just wondering how could we get rid of ASM altogether... just brainstorming here...

@gnodet
Copy link
Contributor

gnodet commented Jan 23, 2024

I think using a Jandex index for just Sisu (which just needs all FQCNs of classes with a specific annotation) is way to much overhead. But maybe there are use cases where that index is already provided and used eslsewhere? @cstamas Can you elaborate a bit on the use case here?

Isn't that exactly what jandex has been created for ? You give the annotation and it just returns all its usage...

@kwin
Copy link
Contributor

kwin commented Jan 23, 2024

No, jandex indices contain a lot more information. Information about all method signatures and also all fields for example. Just look at how big an index is. This is IMHO way too much overhead to just retrieve annotations set on class level.

@gnodet
Copy link
Contributor

gnodet commented Jan 23, 2024

It's a tradeoff between runtime processing and storage obviously.
But it's not about finding annotated classes, it's about finding all the injection points, so basically all classes / fields / constructors injected with @Inject, @Named, @Singleton, @SessionScoped, etc...
I'm thinking in the context of #103 and https://issues.apache.org/jira/browse/MNG-7954 ...

@kwin
Copy link
Contributor

kwin commented Jan 23, 2024

IMHO this is just about finding @Named on class level. That is the only relevant annotation for building the Sisu index! The other annotations are always evaluated at runtime via reflection anyways (happens in Google Guice which does not have the ASM dependency)

@cstamas
Copy link
Member Author

cstamas commented Jan 23, 2024

Not only. ASM is used for more, gleaning the classes, no? Or is used only to scan, when no index present?

@kwin
Copy link
Contributor

kwin commented Jan 23, 2024

Is think ASM is primarily used for generating the index or scanning when no index is in place (in https://github.com/eclipse/sisu.inject/blob/550bd96afa244d22ea0fc84bef5d9b0a356bac25/org.eclipse.sisu.inject/src/main/java/org/eclipse/sisu/space/SpaceScanner.java).

There is one other edge case related to Dynamic Proxies in https://github.com/eclipse/sisu.inject/blob/550bd96afa244d22ea0fc84bef5d9b0a356bac25/org.eclipse.sisu.inject/src/main/java/org/eclipse/sisu/wire/DynamicGlue.java being referenced in https://github.com/eclipse/sisu.inject/blob/550bd96afa244d22ea0fc84bef5d9b0a356bac25/org.eclipse.sisu.inject/src/main/java/org/eclipse/sisu/wire/LocatorWiring.java#L213 but I have never seen the annotation @Dynamic being used. This usage of ASM is about modifying classes (in this case a Jandex index wouldn't help).

I am pretty sure that all run-time usages could be easily replaced by reflection (as the inspected classes are part of the classpath)

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

3 participants