Revisit forbidden-api Gradle plugin

[breskeby]

This plugin introduces a couple of problems with the build:

• it does not handle the Gradle task avoidance api correctly (creating unrequited tasks immediately)
• it uses a bunch of deprecated Gradle apis
• it introduces a huge performance overhead when applied the first time in the build (see https://gradle-enterprise.elastic.co/s/ljk7lj64pdvai/performance/configuration?focused=700&openScriptsAndPlugins=WzFd)

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]

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

[uschindler]

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]

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]

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 ***@***.***> 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]

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]

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]

@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]

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]

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]

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 (policeman-tools/forbidden-apis#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: policeman-tools/forbidden-apis@0613f4d