search

elastic / elasticsearch

Free and Open Source, Distributed, RESTful Search Engine
https://www.elastic.co/products/elasticsearch
Other
1.2k stars 24.84k forks source link

Revisit forbidden-api Gradle plugin #56330

Closed breskeby closed 4 years ago

breskeby commented 4 years ago

This plugin introduces a couple of problems with the build:

The best fix IMO would be to get a new clean plugin on top of the forbidden-api project as the project does not support new Gradle versions and is designed to be build with ant + ivy and parts of the shortcomings described above are caused by that design decision. Also mixing the tool itself and the plugin in one jar is not a good design pattern.

As I can see it, in our build we configure the forbiddenApis tasks directly and do not use the provided plugin extension at all. On option to fix the configuration time issue would be to wire these tasks per sourceSet ourselves in our own plugin. This will not fix the use of deprecated Gradle API in these task implementations.

elasticmachine commented 4 years ago

Pinging @elastic/es-core-infra (:Core/Infra/Build)

uschindler commented 4 years ago

Maybe you just update to version 3.0, Ryan has it prepared already. Task Avoidance works and all your complaints regarding the input output annotations are fixed.

No need to fork the project.

No deprecated warnings anymore. What's the issue?

uschindler commented 4 years ago

I closed the issue on the forbiddenapis side. The only thing is the dynamically compiled Groovy script on apply. But that's neglevtible, look at your absolute numbers. Parsing signature files is a much higher overhead. I would like to work on that and caching them in the plugin, not on some minimal startup cost of a millisecond.

breskeby commented 4 years ago

Thanks Uwe for the fast and thorough feedback. We’ll upgrade to the latest release and then will revisit this issue from there. I think though that the groovy compilation stuff in the plugin is a bit more expensive in certain scenarios than a few miliseconds. Again, I’ll do the update and i want to run some tests to proof or disproof this believe.

On 7. May 2020, at 22:06, Uwe Schindler notifications@github.com wrote:

 I closed the issue on the forbiddenapis side. The only thing is the dynamically compiled Groovy script on apply. But that's neglevtible, look at your absolute numbers. Parsing signature files is a much higher overhead. I would like to work on that and caching them in the plugin, not on some minimal startup cost of a millisecond.

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub, or unsubscribe.

uschindler commented 4 years ago

Ok. The compilation only happens one time on loading of the plugin java class. So it's must be a static cost and should not be relevant for a huge project like Elasticsearch.

If you see some cost per project apply(), it could be because of another bottleneck. Currently it does some reflection on each apply. I can move that up and do once static (it creates a convention mapping for all extension props). That's easy to fix, could come with next version.

uschindler commented 4 years ago

It would be nice to get access to the link posted above with the build analysis so I can look at the graphs. I know much better what's going on and could figure out what you are seeing. In addition, most benchmarks have problems with one time costs (cold jvm, all takes long, and stupidly forbiddenapis is the first one using groovy parser and compiler. On the other hand later code also using dynamic groovy isz faster, forbiddenapis is just the bad guy because it was the first). Working the whole day on such stuff when optimizing Lucene and developing bytecode optimizers like painless compiler (a lot of its code was written by me), one should focus on real problems not startup costs. Painless scripts are also slow on first run and therefore much slower on startup than previous groovy in Elasticsearch. But on.long run they are like 200 times faster.

breskeby commented 4 years ago

@uschindler I cannot grant you access to this Gradle Enterprise instance, but what the link shows is that the application of the plugin to the benchmark subproject took 1.596s configuration time. Again this is done with the older plugin version and does not take into account the improvements done in 3.0.

The link is not a prober benchmark though just a certain ci build I saw in our build pipeline. the way we setup the elastic ci infrastructure (with transient docker based ci workers) makes it likely to have a build started with a fresh daemon (as in this example here).

Again, let me get back to you with a proper benchmarking. I'll use the Gradle-profiler project (https://github.com/gradle/gradle-profiler) for that, which also allows testing different scenarios (simple build, vs full build, vs IDE sync with and without daemons).

uschindler commented 4 years ago

I added a PR to forbiddenapis with my previous optimization idea. Maybe you can test this after upgrade to 3.0 (just try snapshot version from sonatype or compile locally and use `gradle -Dmaven.local=true' when running ES build.

Still the 1.5 seconds must come from somewhere else. I think it's caused by the fact that the task was initialized on project.apply() without task avoidance support - and it was therefore initializing too much stuff like JVM properties, signatures,...

rjernst commented 4 years ago

It looks to me task configuration avoidance completely removes the slowdown.

From the linked build here, running the tar distribution's assemble task:

de.thetaphi.forbiddenapis | Plugin | 2.196s |   | Applied to 239 projects | 861 tasks created | 
  | :benchmarks | 1.596s |   |   | 4 tasks created

And from the same task with my upgrade to 3.0:

de.thetaphi.forbiddenapis | Plugin | 0.137s |   | Applied to 239 projects |  
  | :benchmarks | 0.080s |   |  
uschindler commented 4 years ago

OK thanks for feedback! I still committed the PR on forbiddenapis, so it will come with next version, but I did not expect much difference. Great Work @rjernst, it was good to work wih you! You PR (https://github.com/policeman-tools/forbidden-apis/pull/162) a month ago was really cool. And in combination with the dynamically compiled script this is fully flexible.

@breskeby the problem with gradle is really that it changes all the time and sometimes a major release with backwards compatibility only exists for like 12 months. Because of this you sometimes need to release a new version of plugins quite often. As I can't do this all the time, please excuse: Users must live with deprecation warnings from time to time. That's not the plugin developer's fault, it's just that release cycles are too fast. For a one-man show this is to short.

In addition as there's also Maven and Ant and sometimes minimum Java version constraints, in addition to fixed requirements on ASM versions, so you can't always rely on latest Gradle version or use stuff like compile gradleApi(). Because of this the dynamic plugin initialization is perfect. We only compile against a minimum Gradle API from a Gradle's Maven Repo, which was IMHO a good design decision. In the early stage, the script was indeed compiled per project, but if you read the code now, the way how it's setup does not bring a per project overhead, the compilation phase of the dynamic script is only once on class loading. And that's not more expensive than loading a class in Java; it's a one-time cost.

Nevertheless, thanks for the feedback.

I just committed a "import" statement cleanup as folowup to yesterday: https://github.com/policeman-tools/forbidden-apis/commit/0613f4de8afc0330550fe8d0330f5aee3c2f1bd1

uschindler commented 4 years ago

But there's one question I have for @breskeby , a bit unrelated: There are some class unloading problems in Gradle causing issues if forbiddenapis is used with the gradle daemon. So it defaults to disable internal classloader URI caching, because otherwise you have problems like stale class files when underlying jar file of your build is repackaged (may also lead to JVM crash in Java 8).

Now with recent Gradle versions, the daemon is always used, also when you explicitely disable it - the only difference is that its shutdown after build. But the detection code in forbiddenapis detects daemon usage and then disables the caching, although it's not a problem for the single-use case.

Do you know a good way how to detect if Gradle is running in the daemon, and if it's single-use. Current detection code is:

https://github.com/policeman-tools/forbidden-apis/blob/master/src/main/java/de/thetaphi/forbiddenapis/gradle/ForbiddenApisPlugin.java#L122-L133

The linked stackoverflow question has now a new answer that looks like it allows to detect the "single use" daemon.

val daemonScanInfo: DaemonScanInfo? = (project as DefaultProject).services.get(DaemonScanInfo::class.java)
val runningAsDaemon = !daemonScanInfo.isSingleUse

What do you think? I don't like to discuss about pros/cons of Gradle Daemon, I just want to detect it and if I have detected it, if its "single-use". Because if that's the case, I can enable more optmization, not possible with multiple-use gradle daemon.

This would speed up CI builds by a certain amount, as scanning would be faster, if the Daemon is disabled anyways for CI builds.

breskeby commented 4 years ago

@breskeby the problem with gradle is really that it changes all the time and sometimes a major release with backwards compatibility only exists for like 12 months. Because of this you sometimes need to release a new version of plugins quite often. As I can't do this all the time, please excuse: Users must live with deprecation warnings from time to time. That's not the plugin developer's fault, it's just that release cycles are too fast. For a one-man show this is to short.

@uschindler I can totally see your point and I really appreciate the work you do here as open source. At Gradle we really try and invest a lot (they try, I tried, as I'm not an active gradle core committer anymore) to stay backwards compatible, but as you said the release cycle is relative aggressive which I think is good to keep the innovation going fast and which allowed bringing a lot of good things to gradle over the last years. The typical cycle usually is one major version per year. Though I can see how that can be too fast for open source contributors who mostly do that in their spare time or as a hobby and with other priorities that gradle integration. Obviously no one can expect you to be on top of all the changes in Gradle with every 6 weeks. Thats why I also offered my help in the initial issue raised in your repo.

Regarding the daemon issue. daemonInfo.isSingleUse is indeed what you're looking for. Likely the reason for single use daemons to be started is, that there is a org.gradle.jvmargs property set in the gradle.properties that cannot be matched with the already started jvm. Most likely a memory setting like -Xmx or -Xms. Java does not allow to change that for an already running process thats why another processed is forked here. To avoid that behaviour you could pass those settings by using a GRADLE_OPTS environment variable. This is passed to the initial gradle process and should make the gradle build to not fork a daemon.

uschindler commented 4 years ago

The current forbiddenapis release cycle is about 6 months maximum, because whenever a new Java Version comes out some changes or updates are needed.

This is also the time when I merge most pull requests, unless it's something urgent. This should be enough time to update and remove deprecations in Gradle code.

breskeby commented 4 years ago

quick update. After the update to 3.0 with the changes from @rjernst I see that the initialising cost of the plugin went down to around ~350ms. Given this is only happening against a cold daemon I think we can close this issue for now. when opening it I didn't realise there was already a PR and a newer version with the made changes avaiable. The update to 3.0 is a nice improvement.