-
Notifications
You must be signed in to change notification settings - Fork 168
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
[MCOMPILER-515] This plugin is not "incremental" #160
base: master
Are you sure you want to change the base?
Conversation
Drop the "incremental" bit of this plugin, make it just dumb always redo everything. The incremental bits just introduced bugs that exist for over 10 years, and just make us look bad. --- https://issues.apache.org/jira/browse/MCOMPILER-515
As expected, the two ITs did fail on GH CI. |
As expected, without the two IT it all pass OK. |
-1 |
Also -1 from me. Maybe it is not incremental compile - but detecting if module should be recompiled. Detecting on change for project configuration should be fixed instead. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.
@olamy, good, am really happy for you. So, there is "nothing to be seen here", everything is cool, is it? I have to admit, I was unaware of MCOMPILER project state: "Fun" starts that this issue was reported 10 years ago, but was closed due inactivity: https://issues.apache.org/jira/browse/MCOMPILER-213 But even more "fun" can be seen when one search for "MCOMPILER compiler incremental" in JIRA: https://issues.apache.org/jira/issues/?jql=project%20%3D%20MCOMPILER%20AND%20text%20~%20%22compiler%20incremental%22%20order%20by%20lastViewed%20DESC There are a TON of issues, that are mostly neglected. And all is about "incremental...". There are (relatively) young issues, only 4 years without feedback https://issues.apache.org/jira/browse/MCOMPILER-345 but there are old issues as well, that got closed, while users commented on them "this is still an issue" https://issues.apache.org/jira/browse/MCOMPILER-187 There are REAL issues where users are baffled WHAT AND HOW this plugin is meant to work at all https://issues.apache.org/jira/browse/MCOMPILER-209 -- this one is really funny, and all this is just the tip of the iceberg. Seems nobody understands this "incremental" feature. And now about the sad part: this "feature" is present in ONLY ONE class, that today has 2000 LOC. This PR removes cca 400 LOC from it (roughly 20%), and it "still works", no UT affected at all, and only two ITs out of 60 were affected. I hope we do understand what this means? Maybe not aware, but incremental compilation (and incremental build for that matter) has been solved for Maven years ago. There are situations, where things go so bad, that really just a "let's redo it from scratch" can help, Everything else would just potentially worsen the already bad situation. And this plugin (should be) one of the "crux" of Maven (like m-install-p/m-deploy-p). It's maybe me, who is not satisfied with "current state of affairs" and want to change something about it? Maybe. By vetoing this PR, you choose really the only possible bad outcome: nothing to happen, and continue this "status quo" of broken state. A bit better, but still bad outcome will be that someone will dedicate his time to "fix this", but we all know that solution is about "inputs and outputs", and this plugin and this "incremental" has no clue about that, so wasted time. I just say "let's burn it". And one more thing: are there some measurements, some numbers, perf tests, whatever, that prooves the gain (seemingly at cost of correctness) Maven "wins" with this "incremental" feature? |
maybe incremental is not the right name for this but yes on a 300 modules project not recompiling everything all the time makes a big difference at the end of the day |
think there are multiple points mixed there:
So overall I think it makes sense to keep at least the default behavior |
As everyone talks how this is "must to have" but nobody provided any number to support this claim, I tried to convince myself this is "must to have". So... Subject build: PreparationsBuilt this checked out project "as usual" (by the book) using Then, as 2nd (w/o clean) invocation consistently failed, had to remove tests-integration module as well, to be able to test "incremental" use case. This leaves us with 170 modules. Finally,. changed jetty to use locally built 3.11.0-SNAPSHOT of maven-compile-plugin: Full diff of my local changes: Used InvocationsI used following 3 invocations to time: Used Maven "total time" output as measure. With m-c-p:master1st master: 02:10 min With m-c-p:PR1st PR: 02:11 min ConclusionPlease, somebody, anybody, repeat these (the numbers cannot be "cross" compared, only can be compared among runs on same HW/env), and as always, "this is not a proper benchmark" type of notice... but. So far, it seems Jetty project (170) modules is not "large" enough to produce noticeable difference? |
as you are using a project which need install this doesn't work.... |
Nope, I removed bits needing install, 3rd invocation is |
Here is a simple project showing the usage:
as you can see gain is quite huge |
@rmannibucau i hope you understand your example does not make sense? Can you show me a real life example, a full build? What total gain are we talking about here? |
read my comment especially on the reactor and rebuild of artifacts. what do you see in the output because for every build (except the module
for jetty-10.0.x branch you have 02:10 min?? really?? you have a very very beefy machine so recompiling or not doesn't make any difference for you.... |
Full logs of (fixed link, sorry) |
My test
Scenario 1
Scenario 2
Result 1
Result 2
|
@rmannibucau a bit of simple math: So, the diff you showed would produce on 300 module build total time difference of 4 minutes? Am interested in how much percentage these 4 minutes are of the full build time....30%? 10%? 1%? 0.05%? Let's find out! |
@slawekjaranowski please compare m-c-p master (or any released) vs this PRl 😄 As your test really compares m-c-p against m-c-p, not m-c-p master/released against PR. |
@cstamas as Olivier mentionned, your build does not use this feature so you compare twice the same things. Also my example shows the real gain. I tested on a work project - sorry, there I can't share the full outputs but it is similar to the demo I shared but with 31 modules: 17s for the first build, 10s for the second using the command |
@rmannibucau could you please build locally this PR and try your private project with that same steps? Just interested in times.... (the "31 modules: 17s for the first build, 10s for the second using the command mvn package -pl '-documentation' -DskipTests") |
@cstamas 16s first time then always ~14s so overall the diff logic is worth it even if it costs some time - guess we can make it faster there too tracking files more efficiently in the project but overall this PR has a negative impact on the overall build time. |
@rmannibucau ok, thanks for doing this. As I thought: PR is clearly better at "full recompile" case of m-c-p master |
@cstamas you mean slower and produces the same output? master does by default a full rebuild in 10s whereas your PR does a full rebuild in 14s |
Ok, tried with another project, Maven this time: maven master: 984f73dc7ca2515ee520fbded3382820f62116e9 Invocations1st: With master1st: 49.868s With PR1st: 48.978s And again, the feature w/ master did not work (!) as I understand, as there are inter-dependencies in reactor? What's the point of multi module build if this feature does NOT work with it? Am I right that this feature works on multi module build ONLY if there are no inter project dependencies? I just realized what @olamy was saying.... |
The I don't understand this: "17s for the first build, 10s for the second", isn't first the "full"? |
@cstamas the first is the run after a |
Recap what we have so far:
Is this right? |
If we ignore the figures right - highly depends your build, tomee container/openejb-core module for ex, it is 14s -> 4s (1 module so if you do the math you almost save 1h for your example ;)).
Kind of, "magic" happens there https://github.com/apache/maven-compiler-plugin/blob/master/src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java#L897 and agree the rule can be enhanced to better manage multi-reactor builds, can be a great enhancement/PR to do. |
Sure, we can come up with numbers as we like, but I will shut up in a moment this very feature saves 1h on a real life project out there.
Sure, but this also means that almost no multi module build benefits from this feature, as the sole idea of multi module build is usually to "depend on each other". I can imagine some "lab" project where you have 300 modules that do not depend on each other, but I guess this cannot be called "majority", unless am mistaken. |
Mastercommit 5be3163 Result 1
Result 2
PR 160commit 7b59074 Only scenario 2, many times the same command -
take similar time like so difference from 1 to 5 minutes is important for project which I work at usual day, and many time I need to rebuild |
Depends how you build the multi-modules projects. Often you have some "core" bottleneck in the build graph then a tons of children (think camel if you want, core then most of the modules are children), then if you run a command bypassing the core - built manually before with install - then you get the benefit from it. |
I agree with @rmannibucau here. It is very dependent of the examples and really changes your output. |
no it depends if you reach install phase or not... I tried to explain previously in this issue and this has already been discussed here https://lists.apache.org/thread/1l87rfqk7k26lgh07vt8w6367qgp3sct |
I agree to drop the "incremental" boolean parameter, but not the default behaviour. Not for performance reasons, but just because the output much more often broken when you're developing : when setting Example steps on
in the main pom.xml
The build succeeds, whereas if you run So I'm +1 to get rid of the |
On the performance side, I've been trying to ensure that plugins don't modify any output file without any actual change. So having the compiler plugin behave in this way is really a big win. If the build is setup correctly, the second maven run should not modify any artifact at all (including packaged jars). I would -1 any change that does not keep such a behaviour. That does not mean any kind of incremental compilation : the current behaviour is all or nothing, i.e. any change will recompile the whole module, but I'm fine with that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-1 on the idea to drop the current behaviour
Incremental building is an important feature. If you need it, you need it. Most projects don't but some very important projects do and they need something like this the most. I think the right solution is a compromise where we scale back how this works. The problem is that computing the right set of code to recompile is hard. So let's just make that part easy. Perhaps make the incremental based upon something higher level that where we are now. If that's not possible, then the solution is to actually maintain a map of files to classes to dependent classes/files from the last build and to use that to compute what is built. I think that is the buggy part as that that map is hard to maintain. Perhaps make that map manually editable by devs. We only need this feature in dev environments anyway. TLDR This is an important feature so let's find a way to reduce the complexity to make it work and be maintainable. |
I agree that "incremental" is very important feature. My stance is that this implementation is bad/wrong. |
I agree. I am asking if there is a way we can relax the feature/requirement to make it easier to implement and maintain.
On Monday, April 22, 2024 at 12:07:46 PM PDT, Tamas Cservenak ***@***.***> wrote:
I agree that "incremental" is very important feature. My stance is that this implementation is bad/wrong.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you commented.Message ID: ***@***.***>
|
Today, if one needs fully incremental (and build avoidance) with current stable Maven 3.9.x, one should use Takari Lifecycle plugin. But even then you should use Eclipse JDT not javac. But, in turn, with JDT, you get other nice things like enforcing dependency usage and so on... |
Great, so we have 2 buckets to break these issues into. 1) issues with the difference in behavior with javac (vs JDT) and 2) issues with the difference in behavior with this plugin and the Takari one. I'm curious, what are the issues in the first bucket? Perhaps the target should be to get the maven compiler to work on JDT as a requirement instead of javac and JDT. Maybe that's a goal we can achieve.
|
While incremental builds is important, my reading of the source code inherited from Maven 3 suggests that in its current state, incremental build has bugs causing the opposite effect than what the plugin wanted to achieve. We could try to fix the Maven 3 compiler plugin, but I do not know if it is worth the effort given that the plugin is getting a significant rewrite for Maven 4. The current
Equivalences with the current parameter:
For an initial version of the Maven 4 compiler plugin, detection of changes would be based only on file timestamps. This is the same strategy than the current plugin. This approach would be unable to detect that a class needs to be recompiled because it depends on another class which has been changed. However, the current plugin cannot detect such cases neither, so we don't lost anything. If we support that feature in the future, it would be one new value in the above-cited list of comma-separated values. |
Thank you for the summary of the state of the code, that is very helpful.
I can tell you with 100% certainty that only tracking file timestamps won't work and will be a bug farm. You must know which .class files are generated from which .java files and in addition know which .class files depend upon which other .class files. If this isn't possible, then it isn't possible to implement this feature in a maintainable way. So either we need to track this additional information, or relax the requirement to for example the module level or package level and track dependencies at that level. I am OK with either path but we shouldn't go ahead with only tracking file timestamps and acting like that is a working incremental build feature.
EDITED for clarity and to remove all the cc includes from my email browser.
|
It is incomplete, but this is what the current plugin and the After a refactored compiler plugin become stable enough, it would be possible to investigate how to do better. The proposal to use comma-separated values in |
I agree it is a lot of work. But you can't say this implementation will work either. It will be a bug farm. Inner classes, enums, and interface inheritance will all probably have weird and unexpected issues in more complex cases. And if there are classes compiled by other plugins like scalac? That's why bugs like this exist, because the mapping between java files and class files isn't clean. |
remove plexus-compiler layer but what about other compilers? |
For languages other than Java, we would need a separated plugin. With JPMS support, multi-releases support, incremental build (even incomplete), etc., this plugin is very Java-specific. |
@desruisseaux +1 to drop plexus but also +1 to keep the current incremental feature support by default - whatever name is given if incremental is not considered accurate. |
The point is that this interface may not be sufficient. Ideally, we'd have a list of output files, that would help with the heuristic. |
The use of Eclipse compiler would not be mandated. If no tool chain or The |
Drop the "incremental" bit of this plugin, make it just dumb always redo everything. The incremental bits just introduced bugs that exist for over 10 years, and just make us look bad.
So just let "incremental" part go away.
This change renders 2 ITs broken:
That IMHO should be dropped as well, if this PR goes in.
https://issues.apache.org/jira/browse/MCOMPILER-515