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

@Annotations(@Annotation("shared")) is amazingly wasteful #2035

Closed
gavinking opened this issue Feb 2, 2015 · 33 comments
Closed

@Annotations(@Annotation("shared")) is amazingly wasteful #2035

gavinking opened this issue Feb 2, 2015 · 33 comments
Assignees
Milestone

Comments

@gavinking
Copy link
Member

Check this out:

@.com.redhat.ceylon.compiler.java.metadata.Ceylon(major = 8)
@.ceylon.language.DocAnnotation$annotation$(description = "Hello world")
@.ceylon.language.AuthorsAnnotation$annotation$(authors = "Gavin")
@.com.redhat.ceylon.compiler.java.metadata.Annotations({
        @.com.redhat.ceylon.compiler.java.metadata.Annotation(
                value = "doc",
                arguments = {"Hello world"}),
        @.com.redhat.ceylon.compiler.java.metadata.Annotation(
                value = "by",
                arguments = {"Gavin"})})
interface Stuff {}

Duplicate information is stored in a very wasteful format for no apparent reason. I propose that:

  1. we stop storing doc annotation and user-defined annotations in @Annotations, and
  2. we encode language annotations into @annotations using a modifiers integer.

This could be done without breaking backwards compatibility.

@gavinking gavinking added this to the 1.2 milestone Feb 2, 2015
@tombentley tombentley self-assigned this Feb 3, 2015
@tombentley
Copy link
Member

Early indications are this will make the dist .zip about 6% smaller if we're using a long to encode the modifiers.

@gavinking
Copy link
Member Author

Seems worthwhile to me.

Sent from my iPhone

On Feb 3, 2015, at 12:45 PM, Tom Bentley notifications@github.com wrote:

Early indications are this will make the dist .zip about 6% smaller if we're using a long to encode the modifiers.


Reply to this email directly or view it on GitHub.

@tombentley
Copy link
Member

Sorry, I was completely wrong because I was looking at size of the wrong files. Whether we use an int or a long we're looking at the language module .car being less than 1% smaller.

@quintesse
Copy link
Member

Doesn't surprise me, strings are only stored once if they're exactly the same. It just looks wasteful in the (fake) generated Java source.

@FroMage
Copy link
Member

FroMage commented Feb 3, 2015

Well, why again didn't we remove the @Annotations one? I think it predates support for annotations, no?

@tombentley
Copy link
Member

IIRC because the model loader can't use the @Shared$annotation and friends because we can't put those on native things in the language module. In turn because of the bootstrap problem.

@FroMage
Copy link
Member

FroMage commented Feb 3, 2015

Mmm, why not? We are able to use the metamodel defined in Ceylon in its Java implementation, though.

@tombentley
Copy link
Member

Because in order to do annotation processing javac completes annotations very early on, before the model loader is involved in anything.

@FroMage
Copy link
Member

FroMage commented Feb 3, 2015

Oh, so it's different for annotations? Well, we could still stop generating @Annotations if we're not bootstrapping ;)

@tombentley
Copy link
Member

Oh, so it's different for annotations?

yes

Well, we could still stop generating @Annotations if we're not bootstrapping ;)

No the problem is using stuff like @Shared$annotation doesn't exist for javac when we're compiling natives, so we get a no such symbol. javac has no way to know that the symbol will be defined, because we only enter it into the symbol table during a later phase. I think I described the pb wrongly before, but it's been a while since I looked at it. But I do remember it's a pretty hard one to solve.

@FroMage
Copy link
Member

FroMage commented Feb 3, 2015

I don't get it. If we're not compiling the language module, then @Shared$annotation must be resolvable.

@tombentley
Copy link
Member

Well it was probably about 12 months ago that I was looking at this, so I could be confused, but the problem is what ceylon/ceylon.language#408 is about. When we're compiling the natives in the language module javac tries to complete annotation types like @Shared$annotation before our compiler get's a look-in.

@FroMage
Copy link
Member

FroMage commented Feb 3, 2015

Again, I can believe you when bootstrapping, but that should not be true for any other module.

@tombentley
Copy link
Member

Yes, it's during bootstrapping.

@FroMage
Copy link
Member

FroMage commented Feb 3, 2015

So we should be able to not generate these annotations when not bootstrapping then :)

@tombentley
Copy link
Member

Well obviously I don't understand how the hell the bootstrapping stuff works, but if that's all it needs... let's have a try.

@quintesse
Copy link
Member

Meaning we keep the code that knows how to read both types of annotations but we generate the older type only when compiling the language module, right?

@FroMage
Copy link
Member

FroMage commented Feb 3, 2015

Yes, that would be a good start.

@tombentley
Copy link
Member

Well, sure we could annotate all the native stuff with @Annotations(modifiers=243) (by hand) if you like. But that would be a lot of effort and it still wouldn't fix ceylon/ceylon.language#408 because the annotations are read at runtime using the "real" @Shared$annotation annotation. Of course we could also change the way we read language module annotations at runtime, so everything was consistent.

Or we could try to fix the real problem.

@FroMage
Copy link
Member

FroMage commented Feb 3, 2015

Or we could try to fix the real problem.

Well, we could, but starting by removing @Annotations from every module but the language module. That's easy. The rest is harder as you've pointed out, and IMO can wait.

@quintesse
Copy link
Member

On the other hand, if the net effect on output size is so little why bother? We could wait until we can do it properly and at least get rid of some code?

@gavinking
Copy link
Member Author

Because it is ugly when I turn on logging to see the generated Java schema and have my eyes hurt like someone shoved a stick in them.

That's reason enough.

Sent from my iPhone

On Feb 3, 2015, at 5:36 PM, Tako Schotanus notifications@github.com wrote:

On the other hand, if the net effect on output size is so little why bother? We could wait until we can do it properly and at least get rid of some code?


Reply to this email directly or view it on GitHub.

@quintesse
Copy link
Member

Go see an optician :)

tombentley added a commit to ceylon/ceylon.language that referenced this issue Feb 4, 2015
tombentley added a commit that referenced this issue Feb 4, 2015
…nstead of nested @annotation for the modifier annotations. Omit documentation annotations completely.
tombentley added a commit that referenced this issue Feb 4, 2015
tombentley added a commit that referenced this issue Feb 4, 2015
…t now handles documentation annotations too.
tombentley added a commit that referenced this issue Feb 4, 2015
tombentley added a commit that referenced this issue Feb 4, 2015
… annotations with both styles of annotation: @annotations and @shared$annotation$
@tombentley
Copy link
Member

More fiddly than I would have liked, but hopefully Gavin's eyesight is safe for the time-being.

@gavinking
Copy link
Member Author

Thanks! :)

@FroMage
Copy link
Member

FroMage commented Feb 4, 2015

So we didn't remove the $Shared$annotation when we have the info in @Annotation(modifiers = x)? Why not?

@gavinking
Copy link
Member Author

We need the $Shared$annotation for the metamodel, don't we?

@FroMage
Copy link
Member

FroMage commented Feb 4, 2015

Also, I think we're better off with the modifier masks somewhere in the language module's metadata package so that they can be defined manually in Java code without pulling the compiler module.

@FroMage
Copy link
Member

FroMage commented Feb 4, 2015

We need the $Shared$annotation for the metamodel, don't we?

Well, only if the metamodel can't produce it out of thin-air when required.

@tombentley
Copy link
Member

So we didn't remove the $Shared$annotation when we have the info in @Annotation(modifiers = x)? Why not?

That's what ceylon/ceylon.language#408 is about, and it would involve a similar duplication of code paths in the metamodel. Plus duplicated testing. That's quite a lot more work than this was.

Also, I think we're better off with the modifier masks somewhere in the language module's metadata package so that they can be defined manually in Java code without pulling the compiler module.

I can't really parse that sentence, but I don't have an opposition to putting the masks somewhere else if they need to be shared. Which they would if the metamodel was to read them at runtime.

@FroMage
Copy link
Member

FroMage commented Feb 4, 2015

I meant that if we have to write native code:

@Annotations(modifiers = CeylonModifier.SHARED | CeylonModifier.ABSTRACT)

then it's better if the constants live in the language module's metadata package rather than in the compiler package.

@FroMage
Copy link
Member

FroMage commented Feb 4, 2015

rather than

Or in addition to if we can't make it work.

@tombentley
Copy link
Member

Yes, in addition to because the flags being defined in ceylon.language doesn't help because that's not on the classpath of the compiler.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants