-
Notifications
You must be signed in to change notification settings - Fork 5k
Add fine grained incremental inputs for tasks and transforms #8746
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
Conversation
a40e1cd
to
2e1dc7e
Compare
ec70c6a
to
14d18fa
Compare
It is already logged by the execution engine.
import spock.lang.Unroll | ||
|
||
class IncrementalTasksIntegrationTest extends AbstractIntegrationSpec { | ||
abstract class IncrementalTasksIntegrationTest extends AbstractIntegrationSpec { |
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.
Let's rename this to AbstractIncrementalTasksIntegrationTest
, or IncrementalTasksIntegrationSpec
.
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.
Done.
subprojects/execution/src/main/java/org/gradle/internal/execution/steps/SkipUpToDateStep.java
Show resolved
Hide resolved
public <T> T visitInputFileChanges(IncrementalInputsVisitor<T> visitor) { | ||
return isRebuildRequired() ? | ||
visitor.visitRebuild(thisExecution.getInputFileProperties()) : visitor.visitIncrementalChange(collectInputFileChanges()); | ||
} |
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.
Would be so nice if we could detect beforehand if rebuild is necessary, instead of having to carry around all this state to make the decision late...
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.
I am now eagerly detecting the rebuild and other changes, so we don't need to carry the state around.
* {@literal @}TaskAction | ||
* void execute(IncrementalInputs inputs) { | ||
* if (!inputs.incremental) | ||
* project.delete(outputDir.listFiles()) |
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.
If the task declares itself to be incremental, we should do this automatically whenever rebuild is required. IncrementalInputs
gives us an opportunity to introduce this behavior. WDYT?
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.
Also, what's with the lack of braces?
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.
I fixed the braces.
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.
Without overlapping outputs that would be easy - visit the output properties and wipe out the things. Shall we disallow overlapping outputs for "new" incremental tasks? Or only wipe out the outputs if no overlapping outputs have been detected?
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.
We should think about breaking the build on overlapping outputs. What do we still need to be able to do that?
Until then I guess leaving the outputs in place (and disabling incrementality) when overlaps are present is the way to go. I think it's okay to disable incrementality for both old and new incremental tasks. WDYT?
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.
FTR, I'm fine implementing this behavior in a separate PR.
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.
I resurrected https://github.com/gradle/build-cache/issues/711 to implement this behaviour.
subprojects/core-api/src/main/java/org/gradle/api/execution/incremental/IncrementalInputs.java
Outdated
Show resolved
Hide resolved
public Iterable<InputFileDetails> getChanges(Object property) { | ||
CollectingChangeVisitor visitor = new CollectingChangeVisitor(); | ||
CurrentFileCollectionFingerprint currentFileCollectionFingerprint = currentInputs.get(determinePropertyName(property, propertyNameByValue)); | ||
currentFileCollectionFingerprint.visitChangesSince(FileCollectionFingerprint.EMPTY, "Input", true, visitor); |
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.
Is this the best way to get the files?
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.
I now directly get the changes from the fingerprints in the file collection.
* | ||
* @param property The instance of the property to query. | ||
*/ | ||
Iterable<InputFileDetails> getFileChanges(Object property); |
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.
Should we consider using Stream<>
here?
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.
We could. When using stream, we could add some logic later to only produce the changes which are consumed if we wanted to.
What other benefits do we get from returning Stream
here?
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.
We could do lazy like that with Iterable<>
, too. I'm not sure if Stream<>
has any additional benefits. Maybe it's harder to use from Kotlin? Or simpler?
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.
One of the benefits of Stream
is that you can only iterate it once. That makes it clearer for the user of the API that they should only do it once.
} | ||
|
||
private void visitAllFileChanges(CurrentFileCollectionFingerprint currentFileCollectionFingerprint, CollectingChangeVisitor visitor) { | ||
currentFileCollectionFingerprint.visitChangesSince(FileCollectionFingerprint.EMPTY, "Input", true, visitor); |
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.
Is this the best way to get the files?
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.
Using the fingerprint directly now.
|
||
@Override | ||
public ExecutionOutcome execute(IncrementalChangesContext context) { | ||
public ExecutionOutcome execute(Optional<ExecutionStateChanges> changes) { |
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.
We could pass IncrementalChangesInternal
here, no?
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.
We could also have two methods instead of passing an Optional
.
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.
I am now passing InputChangesInternal
.
Regarding the two methods, I don't think this will make it easier for the one implementing the unit of work. How did you imagine that to work?
...tion/src/main/java/org/gradle/internal/execution/history/changes/InputToPropertyMapping.java
Show resolved
Hide resolved
That allows us to determine whether or not a rebuild is required more easily.
@lptr PTAL! |
This replaces `InputFileDetails` from the old incremental task API.
|
||
@Override | ||
public void contextualise(TaskExecutionContext context) { | ||
public void contextualise(InputChangesInternal inputChanges) { |
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.
I think we need a better name for this method now.
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.
setInputChanges
maybe? The other method would be called clearInputChanges
I guess.
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.
I also renamed ContextAwareTaskAction
to InputChangesAwareTaskAction
.
Optional<ExecutionStateChanges> getExecutionStateChanges(); | ||
|
||
void setExecutionStateChanges(ExecutionStateChanges executionStateChanges); | ||
|
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.
So good to see more methods go from TaskExecutionContext
! I'm going to get rid of outputRemovedBeforeExecution
soon, too.
? incremental | ||
? ExecutionOutcome.EXECUTED_INCREMENTALLY | ||
: ExecutionOutcome.EXECUTED_NON_INCREMENTALLY | ||
: ExecutionOutcome.UP_TO_DATE; |
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.
Didn't we say that this logic should be moved to the execution engine?
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.
Will do. So UnitOfWork.execute
will then return WorkResult
?
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.
Done.
} | ||
|
||
@Override | ||
public void visitIncrementalFileInputs(InputFilePropertyVisitor visitor) { |
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.
Why "incremental" in the name here? We are just visiting the input properties here.
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.
I was going to only visit the incremental inputs in a follow up. What you say makes sense as well: provide a flag to say that the input is incremental or not, so we only need one visit method for the inputs.
@Override | ||
public void visitIncrementalFileInputs(InputFilePropertyVisitor visitor) { | ||
for (InputFilePropertySpec inputFileProperty : context.getTaskProperties().getInputFileProperties()) { | ||
Object value = inputFileProperty.getValue().call(); |
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.
Do we ever use the returned Callable
other than to immediately call call()
on it? Can we return an Optional<Object>
(or a @Nullable Object
) 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.
Any preference for @Nullable
or Optional
?
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.
I made it @Nullable
for now.
...ion/src/main/java/org/gradle/internal/execution/history/changes/IncrementalInputChanges.java
Outdated
Show resolved
Hide resolved
subprojects/execution/src/main/java/org/gradle/internal/execution/steps/ExecuteStep.java
Outdated
Show resolved
Hide resolved
.../java/org/gradle/internal/execution/history/changes/DefaultExecutionStateChangeDetector.java
Outdated
Show resolved
Hide resolved
...ution/src/main/java/org/gradle/internal/execution/history/changes/ExecutionStateChanges.java
Show resolved
Hide resolved
...ts/execution/src/test/groovy/org/gradle/internal/execution/steps/SkipUpToDateStepTest.groovy
Outdated
Show resolved
Hide resolved
@lptr PTAL! |
945848f
to
ae9ee8e
Compare
By using the information if the work did any work or not.
case DID_WORK: | ||
return incremental ? ExecutionOutcome.EXECUTED_INCREMENTALLY : ExecutionOutcome.EXECUTED_NON_INCREMENTALLY; | ||
default: | ||
throw new IllegalArgumentException("Unknown result: " + result); |
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.
Nit: I think throwing an empty AssertionError
here is good.
public interface ContextAwareTaskAction extends ImplementationAwareTaskAction, Describable { | ||
void contextualise(InputChangesInternal inputChanges); | ||
void releaseContext(); | ||
public interface InputChangesAwareTaskAction extends ImplementationAwareTaskAction, Describable { |
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.
Good idea.
LGTM. |
Instead of only being able to query all file changes, we introduce a new interface which allows querying individual input file property changes.
Replaces #8658.