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 more tips on writing native applications #26676

Merged
merged 3 commits into from
Aug 9, 2022

Conversation

galderz
Copy link
Member

@galderz galderz commented Jul 12, 2022

Added 3 further tips on the topics of:

  • Importance of modular code bases for native applications.
  • Building singletons that behave as expected.
  • Potential issues with Object method overrides.

I've been discussing some of these topics with Dan Heidinga as well, so I'll ask if we can review these too.

Copy link
Contributor

@zakkak zakkak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great additions @galderz

I have added some comments for your consideration.

unless they would specifically configure Quarkus to add the jaxb AWT code to the native executable.
However, because jaxb code that uses AWT is in the same jar as the rest of the XML parsing code,
achieving this separation was rather complex and required the use of Java bytecode substitutions to get around it.
These substitutions are hard to maintain and can easily break, hence they are really the last line of defence.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
These substitutions are hard to maintain and can easily break, hence they are really the last line of defence.
These substitutions are hard to maintain and can easily break, hence they should be ones last resort.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. I think there's a small apostrophe missing (ones -> one's)

Comment on lines 350 to 351
As already explained in sections above,
during native image build time GraalVM analyses the application's call tree and generates a universe of all the code it needs.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
As already explained in sections above,
during native image build time GraalVM analyses the application's call tree and generates a universe of all the code it needs.
During native executable build time GraalVM analyses the application's call tree and generates a code-set that includes all the code it needs.

if the jaxb code that dealt with images was in a separate module or jar (e.g. `jaxb-images`),
then Quarkus could choose not to include that module unless the user specifically requested the need to serialize/deserialize XML files containing images at build time.

An added benefit of modular applications can make the universe of code that gets into the native executable smaller.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
An added benefit of modular applications can make the universe of code that gets into the native executable smaller.
Another benefit of modular applications is that they can reduce the code-set that will need to get into the native executable.

then Quarkus could choose not to include that module unless the user specifically requested the need to serialize/deserialize XML files containing images at build time.

An added benefit of modular applications can make the universe of code that gets into the native executable smaller.
The smaller the universe, the faster it native image builds will be and the smaller the native executable produced.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
The smaller the universe, the faster it native image builds will be and the smaller the native executable produced.
The smaller the code-set, the faster the native executable builds will be and the smaller the native executable produced.

As already explained in sections above,
during native image build time GraalVM analyses the application's call tree and generates a universe of all the code it needs.
Having a modular codebase is key to avoiding problems with unused or optional parts of your application,
while at the same time reducing both native image build times and native executable size.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
while at the same time reducing both native image build times and native executable size.
while at the same time reducing both native executable build times and size.

1656509254601%
----

Both of the examples above can be fixed to behave like a singleton in the JVM mode by marking the singleton classes to be runtime initialized.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Both of the examples above can be fixed to behave like a singleton in the JVM mode by marking the singleton classes to be runtime initialized.
Both of the examples above can be fixed to behave like they would in JVM mode by marking the singleton classes to be runtime initialized.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A link to how it can be done could be useful?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm sure I can do. Just note how to do this is already explained earlier in the very same document.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I'm removing this line because just after the cdi singleton example curl examples this recommendation is duplicated and there I include a link (to the wrong place, which I'm fixing).

e.g. `java.lang.Object`,
for the analysis to have to pull all implementations of that particular method.

So, what can you if the implementation one of these methods is giving trouble during native executable build time?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
So, what can you if the implementation one of these methods is giving trouble during native executable build time?
So, what can you do if the implementation of one of these methods is causing issues during native executable build time?

you might be able to use GraalVM substitutions,
but these are quite brittle and can break easily.
So if you end up having to use these,
you should try use them only temporarily,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
you should try use them only temporarily,
you should try to use them only temporarily,

but these are quite brittle and can break easily.
So if you end up having to use these,
you should try use them only temporarily,
until you have the chance to rework/rewrite the code to avoid the original issue.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
until you have the chance to rework/rewrite the code to avoid the original issue.
as a work-around until you have the chance to rework/rewrite the code to avoid the original issue.

Comment on lines 569 to 570
The best way to find out what parts of the source code get included in the analysis
is by looking at the <<native-reference.adoc#native-reports,native build time reports>>.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not clear how this sentence relates to the above.

Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for that. I added some suggestions.

this is both a waste of native compilation time and native executable disk space.
Even more problems arise when third party libraries or specialized API subsystems are used which cause native compilation or runtime errors,
and their use is not modularised enough.
A recent problem can be found in the jaxb library,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
A recent problem can be found in the jaxb library,
A recent problem can be found in the JAXB library,

Please do a search and replace for this one.


When code is not modular enough, generated native executables can end up with more code than what the user needs.
If a feature is not used and the code gets compiled into the native executable,
this is both a waste of native compilation time and native executable disk space.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't it also a waste of memory at the Quarkus level, given we initialize most classes at build time?

This can cause values in Java programs that would vary from one run to another,
to always return a constant value.
E.g. a static field that is assigned the value of `System.currentTimeMillis()`
will always return the same value when executed as Quarkus native.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
will always return the same value when executed as Quarkus native.
will always return the same value when executed as a Quarkus native executable.

1656509254601%
----

Both of the examples above can be fixed to behave like a singleton in the JVM mode by marking the singleton classes to be runtime initialized.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A link to how it can be done could be useful?


@Singleton
class CdiSingleton {
final long startTime = System.currentTimeMillis();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's important to add a comment that the field is not static here. Because if it was, you would have the same issue. Maybe with a callout?

Comment on lines 540 to 562
One more simple way to enforce a singleton relying static fields, or enums,
is to <<delay-class-init-in-your-app,delay its class initialization until run time>>.
The nice advantage of CDI-based singletons is that your class initialization is not constrained,
so you can freely decide whether it should be build-time or run-time initialized,
depending on your use case.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't it the first thing you mentioned above before the singleton example? It looks weird to come back to it now with One more simple way.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah there was some duplication. I'm removing the above mention and I'm rewording this paragraph.

what happens in these method overrides matters,
even if the application does not explicitly call them.
This is because these methods are used throughout the JDK,
and all it takes it's for one of those calls to be done on an unconstrained type,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
and all it takes it's for one of those calls to be done on an unconstrained type,
and all it takes is for one of those calls to be done on an unconstrained type,

=== Beware of common Java API overrides

Certain commonly used Java methods are overriden by user classes,
e.g. `toString`, `equals`, `hashCode`...etc.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should make it extra clear that this is relevant only if these methods are doing funky things? Because in most cases, it's not a problem at all?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree this is a more subtle point - the key is that these methods are pulling in code that wouldn't otherwise be used. Simple straightforward toString / equals is no problem. But the more additional formatters utils, etc that get pulled in, the more the image gets bloated by them.

@galderz do you have some examples for this? It would help drive the right point home that it's "extra" code pulled in by these methods that are problematic.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gsmet Indeed that's correct. I'll reiterate those points.

@DanHeidinga It's not only extra code that can be a problem. If implementations do reflection or use proxies, you could get a similar effect where your native image fails because there's some extra configuration missing. I'll add details on this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DanHeidinga Oh, about examples. I don't have any right now. I think @Sanne might have since this came from a discussion I had with him a while back. IIRC some toString() impls in Hibernate (not sure if in the code itself, or in user provided entities) did something funky that caused the native image build to break.

Comment on lines 560 to 561
E.g. instead of defining `String toString() { ... }` method,
create a `String show() { ... }` method.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, that could work for toString() but will probably be a pain but is definitely not useful for hashCode and equals.
TBH, I'm not sure if this paragraph brings a lot of value given in most cases these methods will be very simple and won't bring any new classes.
If we're worried it could be a problem, I think we need to be extremely clear at the beginning of the paragraph and describe exactly when it's a real problem.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This suggestion works great in small applications but I don't think it scales. Agree with @gsmet that it would be better to cut it as it gets too far into the weeds for most users. Users would be better to focus on other refactorings before trying to convert toString usage -> show

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 to both. I'll remove this refactoring suggestion.

which is capable of deserializing XML files containing images using Java’s AWT APIs.
The vast majority of Quarkus XML users don’t need to deserialize images,
so there shouldn’t be a need for users applications to include Java AWT code,
unless they would specifically configure Quarkus to add the jaxb AWT code to the native executable.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unless they specifically configure...

@galderz
Copy link
Member Author

galderz commented Jul 29, 2022

Thanks all for your feedback. I've addressed all the points and pushed updates for each tip.

Comment on lines 396 to 414
As already explained in the <<delay-class-init, delay class initialization>> section,
Quarkus marks all code to be initialized at build time unless otherwise specified.
This means that, unless marked otherwise,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
As already explained in the <<delay-class-init, delay class initialization>> section,
Quarkus marks all code to be initialized at build time unless otherwise specified.
This means that, unless marked otherwise,
As already explained in the <<delay-class-init, delay class initialization>> section,
Quarkus marks all code to be initialized at build time by default.
This means that, unless marked otherwise,

Minor nitpick on phrasing to avoid repeated use of "otherwise"

E.g. a static field that is assigned the value of `System.currentTimeMillis()`
will always return the same value when executed as a Quarkus native executable.

Singletons that rely on either static variable initialization will suffer similar problems.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Singletons that rely on either static variable initialization will suffer similar problems.
Singletons that rely on static variable initialization will suffer similar problems.

Removed "either" as I wasn't sure what the other option was.

1656509254601%
----

One more way to fix it is to build singletons using CDI's `@Singleton` annotation:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
One more way to fix it is to build singletons using CDI's `@Singleton` annotation:
One way to fix it is to build singletons using CDI's `@Singleton` annotation:

Removed "more" as this is the first proposed fix.

Copy link
Contributor

@zakkak zakkak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from a typo and the comments of the rest of the reviewers it looks good to me. Thanks Galder

unless they specifically configure Quarkus to add the JAXB AWT code to the native executable.
However, because JAXB code that uses AWT is in the same jar as the rest of the XML parsing code,
achieving this separation was rather complex and required the use of Java bytecode substitutions to get around it.
These substitutions are hard to maintain and can easily break, hence they should be the one's last resort.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
These substitutions are hard to maintain and can easily break, hence they should be the one's last resort.
These substitutions are hard to maintain and can easily break, hence they should be one's last resort.

@gsmet
Copy link
Member

gsmet commented Aug 8, 2022

@galderz any chance you could have a look at the final comments so that we can get this in?

@galderz
Copy link
Member Author

galderz commented Aug 8, 2022

@gsmet Yeah sure, I've been distracted by other things (e.g. your unsafe method handle thing 😜)

@galderz galderz force-pushed the topic.0629.more-writing-native-tips branch from c443e73 to a7cdbcf Compare August 8, 2022 15:38
@galderz
Copy link
Member Author

galderz commented Aug 8, 2022

Addressed the last round of comments.

@gsmet gsmet merged commit af641aa into quarkusio:main Aug 9, 2022
@quarkus-bot quarkus-bot bot added this to the 2.12 - main milestone Aug 9, 2022
@gsmet
Copy link
Member

gsmet commented Aug 9, 2022

Thanks, nice addition.

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

Successfully merging this pull request may close these issues.

5 participants