search

eclipse-sisu / sisu-project

Sisu Inject
https://www.eclipse.org/sisu
Eclipse Public License 2.0
17 stars 15 forks source link

Add new space index: Jandex #106

Open cstamas opened 5 months ago

cstamas commented 5 months ago

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:

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 commented 5 months ago

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 commented 5 months ago

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

gnodet commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 https://github.com/eclipse/sisu.inject/issues/103 and https://issues.apache.org/jira/browse/MNG-7954 ...

kwin commented 5 months ago

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 commented 5 months ago

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

kwin commented 5 months ago

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)