Added Gradle 4.9 Configuration Avoidance support #425
Conversation
|
Thanks @carbotaniuman! I'm eager to merge new features into Spotless, but the bar for merging something that adds complexity to the core infrastructure is pretty high. The way that spotless builds formatters is already lazy. Because of this, I have always been skeptical that adding support for Gradle's lazy configuration would be a speedup. @jbduncan did some profiling which showed that it was a speedup. However, since then, we have found and fixed a big performance sink in #348. Now that that has been fixed, I am again skeptical that task configuration avoidance will bring any speedup for us. In order to merge this PR, or any like it, I would need to see profiling data which shows that it is a clear win. |
|
If the overhead concern is just because of the duplicated logic, why not just only support Gradle 4.9+ (or even just 5.0 for major version simplicity)? Supporting fairly old versions seems like it's the real problem here, not really the complexity or potential performance improvements this brings. Regardless of the above - the request for profiling is also unfortunately hard, as this project uses an old version of gradle, is quite small (overhead would be best measurable in a large project), and the cited performance sink fix actually has no profiling stats either. You might be right about existing implementations being very lazy and thus leading to trivial overhead, but I don't think that's necessarily a reason to not move to better APIs designed to assist and enforce those patterns. At worst, they're better guard rails that lend confidence to that lazy claim. |
Not to people with old projects! Compatibility is Good, it lets our users pick their priorities, rather than us forcing them to adapt to ours. See #419 or #252 or many others.
Why don't you just update your gradle to make profiling easy? Hard to update? Good thing that your plugin maintainers don't force you to update gradle ;-) If the project is too small to measure a performance increase, why are either of us spending time on performance? Making code simpler is Good. Maintaining compatibility is Good. Being faster is also good, but it is so common that code that you thought was faster is actually not. The only way to know if something is faster is to profile it. And if it's already so fast that we can't tell if it's faster, then how did this make it to the top of our todo list? I'm freaking busy, and you guys probably are too!! Helping people get their work merged into Spotless is always at the top of my todo list, but I have to protect the interest of other stakeholders if the change causes churn or adds complexity. If nobody in this PR or the last PR actually has a performance problem, I prefer that we work on problems that we already have. YAGNI. YAGNI YAGNI YAGNI. This PR and it's related issue and PR have taken a lot of time and discussion. I'm happy to talk more, and I'm also happy to merge this truly high-quality PR, but only if there is profiling data showing a clear benefit. If you don't already have a project like that, why make the work? But I promise not to ask where you got the problem if you decide to do the work to make the problem. I don't insist on proof that the problem that this PR solves is natural, only that it exists ;-) Reopen iff you have profiling data. |
|
Would be helpful if you provided some guidance on what kind of profiling
you'd expect, as it seems you have a clear idea of what you're looking for.
i.e linkable oss projects, build scans, flame graphs, raw command line
times, which versions of gradle, etc.
Not really with you on the compatibility to the extremes you're describing.
Current major versions seems pretty reasonable to me, and a fairly standard
model for plugin/extension libraries built for another API. If anything, I
think supporting extremely old versions actively inhibits the community's
ability to move forward as it's now adding friction to adopting modern APIs.
…On Tue, Aug 6, 2019, 2:01 AM Ned Twigg ***@***.***> wrote:
Supporting fairly old versions seems like it's the real problem
Not to people with old projects! Compatibility is Good, it lets our users
pick their priorities, rather than us forcing them to adapt to ours. See
#419 <#419> or #252
<#252> or many others.
the request for profiling is also unfortunately hard, as this project uses
an old version of gradle, is quite small
Why don't you just update your gradle to make profiling easy? Hard to
update? Good thing that your plugin maintainers don't force you to update
gradle ;-)
If the project is too small to measure a performance increase, why are
either of us spending time on performance?
Making code simpler is Good. Maintaining compatibility is Good. Being
faster is also good, but it is *so common* that code that you thought was
faster is actually not. The only way to know if something is faster is to
profile it. And if it's already so fast that we can't tell if it's faster,
then how did this make it to the top of our todo list? I'm freaking busy,
and you guys probably are too!!
Helping people get their work merged into Spotless is always at the top of
my todo list, but I have to protect the interest of other stakeholders if
the change causes churn or adds complexity. If nobody in this PR or the
last PR actually *has a performance problem*, I prefer that we work on
problems that we already have. YAGNI. YAGNI YAGNI YAGNI.
This PR and it's related issue and PR have taken a lot of time and
discussion. I'm happy to talk more, and I'm also happy to merge this truly
high-quality PR, but only if there is profiling data showing a clear
benefit. If you don't already have a project like that, why make the work?
But I promise not to ask where you got the problem if you decide to do the
work to make the problem. I don't insist on proof that the problem that
this PR solves is natural, only that it exists ;-)
Reopen iff you have profiling data.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#425?email_source=notifications&email_token=AAKMJPS4QIC6HVUOCQRJMATQDEHS7A5CNFSM4IIMKPX2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3T64EY#issuecomment-518516243>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAKMJPVXNYLRC4RCBHYCJPTQDEHS7ANCNFSM4IIMKPXQ>
.
|
You have a project, using the latest Spotless. Some task runs in X seconds. You switch to this PR. It runs in (X - delta) seconds. delta / X > 25% If you have a performance problem, this will be trivial. If you don't have a performance problem, I wouldn't recommend working on performance yet. |
|
So the project I wrote this for turned out to not benefit because it was another misconfigured plugin that broke build times. So I guess I'll keep this closed until something comes along that benefits. |
Work in progress for Gradle 4.9 Configuration Avoidance.
Once complete, this should close #269 as this as a continuation of #277
All tests pass and a manual test on Gradle 5.4.1 shows that it does in fact lazily configure task.
Gradle has been bumped up to 4.9, as this is the first version with this new API.