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

ArC generates a lot of AnnotationLiteral resources #525

Closed
mkouba opened this issue Jan 16, 2019 · 7 comments · Fixed by #586
Closed

ArC generates a lot of AnnotationLiteral resources #525

mkouba opened this issue Jan 16, 2019 · 7 comments · Fixed by #586
Assignees
Labels
kind/enhancement New feature or request
Milestone

Comments

@mkouba
Copy link
Contributor

mkouba commented Jan 16, 2019

ArC needs to generate annotation literals for:

  • bean qualifiers
  • interceptor bindings
  • injected InjectionPoint metadata
  • maybe something else

E.g. for the following injection point:

@Inject
@ConfigProperty(name = "greeting.suffix", defaultValue="!")
String suffix;

ArC generates the following class:

public class ConfigProperty5_AnnotationLiteral extends AnnotationLiteral<ConfigProperty> implements ConfigProperty {
  public String name() {
    return "greeting.suffix";
  }
  public String defaultValue() {
    return "!";
  }
}

ArC can generate shared literals for an annotation type and values but this optimization must be disabled in shamrock because we have two classloaders there - one for app classes and one for framework classes.

We should try to improve this.

@mkouba mkouba added the kind/enhancement New feature or request label Jan 16, 2019
@mkouba
Copy link
Contributor Author

mkouba commented Jan 16, 2019

This is an attempt to share literals per target package:
https://github.com/mkouba/protean-shamrock/tree/anno-literals-share-package

But we should probably come up with something more efficient.

@dmlloyd
Copy link
Member

dmlloyd commented Jan 16, 2019

The ClassValue class is an easy way to add a cache per source Class without worrying too much about class loaders.

@mkouba
Copy link
Contributor Author

mkouba commented Jan 16, 2019

The problem with class loader is that we cannot share one annotation literal class for both an application consumer class and a framework consumer class. If I understand it correctly we use two class loaders because of the hot deployment feature (only app class loader is discarded if needed).

@stuartwdouglas
Copy link
Member

We should be able to share them, it should just use the ClassLoader of whatever is defining the Annotation itself.

@mkouba mkouba self-assigned this Jan 21, 2019
@mkouba mkouba added this to the 0.7.0 milestone Jan 21, 2019
@dmlloyd
Copy link
Member

dmlloyd commented Jan 21, 2019

I was curious if is necessary to generate an entire class per annotation instance? Wouldn't it be better to generate a single class per annotation with fields and equals/hashCode?

@mkouba
Copy link
Contributor Author

mkouba commented Jan 21, 2019

@dmlloyd Yes, I'm just working on this.

Basically we will generate something like this:

public class ConfigProperty5_AnnotationLiteral extends AnnotationLiteral<ConfigProperty> implements ConfigProperty {
  private final String name;
  private final String defaultValue;
  public ConfigProperty5_AnnotationLiteral(String name, String defaultValue) {
    this.name = name;
    this.defaultValue =  defaultValue;
  }
  public String name() {
    return name;
  }
  public String defaultValue() {
    return defaultValue;
  }
}

And the "client side" will provide the values.

The original approach was just simpler to implement ;-)

@dmlloyd
Copy link
Member

dmlloyd commented Jan 21, 2019

Ah, great. A nice thing about the fields approach is that you have a 1:1 mapping of @interface Class to implementation Class, so you can then use ClassValue to handle the correspondence and never care about class loaders at all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants