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

Unreasonably large configuration performance overhead #348

Closed
oehme opened this issue Feb 8, 2019 · 5 comments
Closed

Unreasonably large configuration performance overhead #348

oehme opened this issue Feb 8, 2019 · 5 comments

Comments

@oehme
Copy link

oehme commented Feb 8, 2019

This line iterates the whole file collection, no matter if the task will execute or not. This is a large performance overhead, paid on every build invocation, no matter if spotless will be used or not. It should instead only check wether the collection itself is null.

@nedtwigg
Copy link
Member

nedtwigg commented Feb 8, 2019

I think that line is using this static import:

import static com.diffplug.gradle.spotless.PluginGradlePreconditions.requireElementsNonNull;

Which then calls this:

@SafeVarargs
static <T> T[] requireElementsNonNull(T... elements) {
Objects.requireNonNull(elements);
for (T element : elements) {
Objects.requireNonNull(element);
}
return elements;
}

not this:

static <T, I extends Iterable<T>> I requireElementsNonNull(I elements) {
Objects.requireNonNull(elements);
for (Object element : elements) {
Objects.requireNonNull(element);
}
return elements;
}

because target is an Object[], but I might be making a mistake. So I think it's only iterating over (N filecollections), not (file collection of size N).

@oehme
Copy link
Author

oehme commented Feb 8, 2019

Sorry, I had linked the wrong line. Here is the real culprit:

this.target = requireElementsNonNull(target);

@nedtwigg nedtwigg added the bug label Feb 8, 2019
@nedtwigg
Copy link
Member

nedtwigg commented Feb 8, 2019

Aha! Thanks very much for the bug find 👍

@jbduncan
Copy link
Member

Oh, oops! The use of requireElementsNonNull was my doing, so thank you @oehme for submitting the bug report, and thank you very much @nedtwigg for rectifying it for me!

@nedtwigg
Copy link
Member

Published in 3.18.0

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

No branches or pull requests

3 participants