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

Make incremental compile faster on constant changes #6482

Open
breskeby opened this issue Aug 23, 2018 · 23 comments
Open

Make incremental compile faster on constant changes #6482

breskeby opened this issue Aug 23, 2018 · 23 comments

Comments

@breskeby
Copy link
Member

@breskeby breskeby commented Aug 23, 2018

We use the rocker template engine for generating ui and the gradle rocker plugin by @etiennestuder.

In our app we see that changing a rocker template file always triggers a full recompilation of the app source even though that should not be required.

After consulting with @melix I've learned that @oehme lately worked on incremental compilation. @oehme would be great if you can have a look.

Expected Behavior

  • A rocker template change does not result in a full recompilation

Current Behavior

  • A rocker template change results in a full recompilation

Context

The GE team is affected by this issue.

Steps to Reproduce (for bugs)

An Example build is attached:
rocker-incremental-compile.zip

  1. Run ./gradlew compileJava on example project
  2. Change content in src/rocker/Greetz.rocker.html (e.g change the greeting message)
  3. Run ./gradlew compileJava -d. This will result in a full recompile. the debug output provides the following information:
Full recompilation is required because 'Greetz.java' was changed

I've tested the behaviour with 4.9, 4.10-rc-2 and latest gradle master (Gradle 5.0-20180823103201+0000)

This behaviour seemed to indicate a constant change in the class. When checking the changes in Greetz.java (generated by the rocker plugin in src/generated/rocker/Greetz.java seems that only private static fields have changed. So a full recompilation shouldn't be required.

@oehme

This comment has been minimized.

Copy link
Contributor

@oehme oehme commented Aug 23, 2018

Does that class have public constants? If so this is expected, any change to a file with constant will result in a full recompile of its sourceset. We don't parse sources, so we don't know what actually changed.

Workaround would be to compile those templates in their own compile task and put the result on the main classpath. That way the full recompile would be limited to the templates only.

@breskeby

This comment has been minimized.

Copy link
Member Author

@breskeby breskeby commented Aug 23, 2018

thanks for checking. Indeed it has: ```

static public final com.fizzed.rocker.ContentType CONTENT_TYPE = com.fizzed.rocker.ContentType.HTML;
static public final String TEMPLATE_NAME = "Greetz.rocker.html";
static public final String TEMPLATE_PACKAGE_NAME = "";
static public final String HEADER_HASH = "973482035";
static public final String[] ARGUMENT_NAMES = { "message" };

I'll check if there's a way to tweak the template generation. 
@oehme oehme added a:performance-issue and removed a:bug labels Aug 23, 2018
@oehme oehme changed the title Changing rocker template always causes full recompilation of app source Make incremental compile faster on constant changes Aug 23, 2018
@oehme

This comment has been minimized.

Copy link
Contributor

@oehme oehme commented Aug 23, 2018

I've renamed this to reflect a possible improvement in Gradle itself. For instance, we write a javac plugin to analyze the actual usage of constants. This would work for all OpenJDK based compilers and would prevent full recompiles.

@oehme oehme unassigned breskeby and oehme Aug 23, 2018
@oehme

This comment has been minimized.

Copy link
Contributor

@oehme oehme commented Aug 23, 2018

You may also want to suggest the usage of static methods to the rocker team. The above don't look like constant at all, they can change as the template evolves. Constants are really only meant for things like PI, which is constant forever.

@breskeby

This comment has been minimized.

Copy link
Member Author

@breskeby breskeby commented Aug 23, 2018

That was my plan. They have different modes for code generation. I'm currently looking into this and then will report back to the rocker team too. Thanks again.

@breskeby

This comment has been minimized.

Copy link
Member Author

@breskeby breskeby commented Aug 23, 2018

Some further findings:

  • incremental compilation works when going back to gradle 3.4.
  • having all rocker templates in one folder leads to a high chance of recompilation as all compiled templates contain a public constant string with the shared package name.
  • using templates with same parameter names also raises the chance for a bigger recompile as a constant HEADER_HASH is based on those parameter names.

I assume with comments from @oehme above we know about this change in behaviour in Gradle and that it was done on purpose?

@oehme

This comment has been minimized.

Copy link
Contributor

@oehme oehme commented Aug 23, 2018

Incremental compilation was broken in 3.4, it used to ignore the fact that constants can be inlined.

I don't understand the other two points. It doesn't matter what those constants contain. If the file contains a constant at all and it changes, then you'll get a full recompile of the containing sourceSet.

Between different sourceSets you get a full recompile when the value of a constant actually changes, because we can analyze the bytecode and tell if it has changed or not.

@breskeby

This comment has been minimized.

Copy link
Member Author

@breskeby breskeby commented Aug 23, 2018

As I understand incremental compilation was broken with Gradle < 3.4 and 3.4 fixed the handling of inlined constants.
Though, the fact that we do a full recompile in the latest gradle version (and do not with 3.4) as soon as we detect a constant at all is another change in the behavior of the incremental compiler. FYI the current behavior does not match up with what we describe in https://blog.gradle.org/incremental-compiler-avoidance

Point 2 & 3 are just about incremental compiler behavior in Gradle 3.4 where in fact the content of the constants do matter (see explanation in https://blog.gradle.org/incremental-compiler-avoidance)

@oehme

This comment has been minimized.

Copy link
Contributor

@oehme oehme commented Aug 23, 2018

Inlined constants can be derived from other constants. 3.4 was broken in that case and 3.5 fixed that.

The value is only used across modules to tell whether a constant has changed or not. If any have changed, everything is recompiled.

@melix Would you mind correcting the blog post?

@ldaley

This comment has been minimized.

Copy link
Member

@ldaley ldaley commented Aug 23, 2018

Constants are really only meant for things like PI, which is constant forever.

But that is not how they are used in practice, and we are unlikely to be able to change that.

@ldaley

This comment has been minimized.

Copy link
Member

@ldaley ldaley commented Aug 23, 2018

Workaround would be to compile those templates in their own compile task and put the result on the main classpath

The templates often use classes from the main source so this is more work than it sounds and probably not worth the effort of the extra indirection in the particular case that caused us to raise this.

@oehme

This comment has been minimized.

Copy link
Contributor

@oehme oehme commented Aug 24, 2018

But that is not how they are used in practice, and we are unlikely to be able to change that.

Case in point - The rocker team is considering such a change. The Android team is also removing the constants from their R classes. And in user code they are often trivial to avoid.

Of course avoiding the cost altogether is the desired goal, hence this issue. But education can unblock some people in the meantime.

@ldaley

This comment has been minimized.

Copy link
Member

@ldaley ldaley commented Aug 28, 2018

Sure, but we should fully accept the weakness of the “don't use constants” answer.

@wordlessly

This comment has been minimized.

Copy link

@wordlessly wordlessly commented Jul 25, 2019

Can't you force the compiler to stop inlining public constants (or constants in general) ?

@oehme

This comment has been minimized.

Copy link
Contributor

@oehme oehme commented Jul 25, 2019

No, we can't, because inlining constants is part of the Java language specification. A compiler must do it or it's not a valid Java compiler.

@wordlessly

This comment has been minimized.

Copy link

@wordlessly wordlessly commented Jul 25, 2019

Convert all public constants into private constants and duplicate them (again as private constants) wherever they were used in external classes.

@oehme

This comment has been minimized.

Copy link
Contributor

@oehme oehme commented Jul 25, 2019

We won't change people's code :)

@wordlessly

This comment has been minimized.

Copy link

@wordlessly wordlessly commented Jul 25, 2019

You already do, by using a compiler :)

Also you could offer annotations for people to tell you to change their code. So instead of removing public constants people could annotate them and you do the rest :)

@Jazzepi

This comment has been minimized.

Copy link

@Jazzepi Jazzepi commented Jul 28, 2019

But that is not how they are used in practice, and we are unlikely to be able to change that.

Case in point - The rocker team is considering such a change. The Android team is also removing the constants from their R classes. And in user code they are often trivial to avoid.

Of course avoiding the cost altogether is the desired goal, hence this issue. But education can unblock some people in the meantime.

I'm glad you guys have an issue open for this. I use public static final String constants in quite a few places. The idea of gutting them and replacing them with methods because Gradle has no idea what the compiler is doing is just gross. This is very tail wagging the dog situation. A build system's behavior around incremental compilation should never be a driver for how source code should look.

@autonomousapps

This comment has been minimized.

Copy link
Contributor

@autonomousapps autonomousapps commented Aug 19, 2019

The documentation on this appears to be misleading. It says:

Since constants can be inlined, any change to a constant will result in Gradle recompiling all source files. For that reason, you should try to minimize the use of constants in your source code and replace them with static methods where possible.

But shouldn't it read

Since constants can be inlined, any change to a constant or a class containing a constant ...

?

@hungvietnguyen

This comment has been minimized.

Copy link

@hungvietnguyen hungvietnguyen commented Aug 20, 2019

Or even better:

Since public constants can be inlined, any change to a public constant or a class containing a public constant... For that reason, you should try to minimize the use of public constants...

@hungvietnguyen

This comment has been minimized.

Copy link

@hungvietnguyen hungvietnguyen commented Aug 20, 2019

Also, the message when running with --info says:

Full recompilation is required because 'SomeClass.java' was changed.

It would be great if it said:

Full recompilation is required because 'SomeClass.java' was changed and it contained a public constant.

(This confusion came up multiple times at http://issuetracker.google.com/110061530 where developers are unsure why a simple change resulted in full recompile.)

@wordlessly

This comment has been minimized.

Copy link

@wordlessly wordlessly commented Aug 20, 2019

Or even better:

Since public constants can be inlined, any change to a public constant or a class containing a public constant... For that reason, you should try to minimize the use of public constants...

Can be inlined? This is optional?

No, we can't, because inlining constants is part of the Java language specification. A compiler must do it or it's not a valid Java compiler.

According to @oehme they have to be inlined.

On a different note: can't Gradle "remember" all public constants and check if they have changed since the last compilation run? If not we can skip a full recompile.

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

Successfully merging a pull request may close this issue.

None yet
7 participants
You can’t perform that action at this time.