Make property specs independent of tasks #8231

wolfs · 2019-01-14T14:18:59Z

No description provided.

This brings PropertyValue closer to a basic ValidatingValue.

ValidatingValue is enough to capture the actual value.

Especially the annotation handler should not need the file resolver.

lptr

Moving things around and making several changes to them at the same time makes it really hard to review this change as is. Would there be a way to split this up into two or more smaller chunks? Just getting rid of the SpecFactory would be a nice first step. Or does that pull in everything with it?

.idea/inspectionProfiles/Gradle.xml

gradle/init-scripts/overlapping-task-outputs-stats-init.gradle

subprojects/core-api/src/main/java/org/gradle/api/tasks/TaskOutputFilePropertyBuilder.java

...core/src/integTest/groovy/org/gradle/api/tasks/TaskInputFilePropertiesIntegrationTest.groovy

...ojects/core/src/main/java/org/gradle/api/internal/tasks/properties/UnitOfWorkProperties.java

subprojects/core/src/main/java/org/gradle/api/internal/tasks/properties/ValidationActions.java

...le/api/internal/tasks/properties/annotations/AbstractInputFilePropertyAnnotationHandler.java

lptr · 2019-01-15T08:54:14Z

...le/api/internal/tasks/properties/annotations/AbstractInputFilePropertyAnnotationHandler.java

-public abstract class AbstractInputPropertyAnnotationHandler implements PropertyAnnotationHandler {
+import static org.gradle.api.internal.tasks.properties.annotations.InputPropertyAnnotationHandlerUtils.isOptional;
+
+public abstract class AbstractInputFilePropertyAnnotationHandler implements PropertyAnnotationHandler {
    @Override
    public boolean shouldVisit(PropertyVisitor visitor) {
        return !visitor.visitOutputFilePropertiesOnly();


This bothers me. Why do we need this? Why can't simply PropertyAssociationTaskFactory.Listener do nothing when visitInputProperty() etc. is called? We are breaking, or at least complicating the visitor pattern by adding quite a bit of code in order to return early instead of executing some no-op stuff. I might be missing something, but this looks like the wrong thing to do.

IIRC, that is a performance optimization introduced by @adammurdoch so we don't have a performance regression for attaching the producers. The performance regression was caused by going into the nested properties. See

gradle/subprojects/core/src/main/java/org/gradle/api/internal/project/taskfactory/PropertyAssociationTaskFactory.java

Lines 52 to 88 in 69efd80

private static class Listener implements PropertyVisitor {

private final Task task;

public Listener(Task task) {

this.task = task;

}

@Override

public boolean visitOutputFilePropertiesOnly() {

return true;

}

@Override

public void visitInputFileProperty(String propertyName, boolean optional, boolean skipWhenEmpty, Class<? extends FileNormalizer> fileNormalizer, PropertyValue value, InputFilePropertyType filePropertyType) {

throw new UnsupportedOperationException();

}

@Override

public void visitInputProperty(String propertyName, PropertyValue value, boolean optional) {

throw new UnsupportedOperationException();

}

@Override

public void visitDestroyableProperty(Object value) {

throw new UnsupportedOperationException();

}

@Override

public void visitLocalStateProperty(Object value) {

throw new UnsupportedOperationException();

}

@Override

public void visitOutputFileProperty(String propertyName, boolean optional, PropertyValue value, OutputFilePropertyType filePropertyType) {

value.attachProducer(task);

}

}

It would be better to be able to attach the producer to outputs in nested beans as well. I'll check again how big the regression is.

I checked it out, and there seems to be quite a performance impact: https://builds.gradle.org/viewLog.html?buildId=19160450&buildTypeId=Gradle_Check_PerformanceTestCoordinator&tab=report_project944_Performance&branch_Gradle_Check_Stage_ReadyforMerge=wolfs%2Fextract-property-specs-review

I would leave the code as is for now. Shall we add a card to clean it up?

Yes please.

https://github.com/gradle/build-cache/issues/1438

...core/src/integTest/groovy/org/gradle/api/tasks/TaskInputFilePropertiesIntegrationTest.groovy

subprojects/core/src/main/java/org/gradle/api/internal/AbstractTask.java

...ojects/core/src/main/java/org/gradle/api/internal/tasks/CompositeTaskOutputPropertySpec.java

subprojects/core/src/main/java/org/gradle/api/internal/tasks/TaskFilePropertySpec.java

subprojects/core/src/main/java/org/gradle/api/internal/tasks/properties/PropertyWalker.java

.../groovy/org/gradle/api/internal/project/taskfactory/TaskPropertyNamingIntegrationTest.groovy

Declared -> Registered Move ValidatingValue to properties package

Looks like 0.4.1 is using `TaskValidator`.

The value doesn't validate any more.

Since validation happens separately now, we don't need to wrap the value.

The method is so simple it makes no sense to extract it.

subprojects/core/src/main/java/org/gradle/execution/plan/DefaultExecutionPlan.java

...ore/src/main/java/org/gradle/api/internal/tasks/DefaultRegisteredTaskOutputFileProperty.java

subprojects/core/src/main/java/org/gradle/api/internal/tasks/DefaultTaskOutputs.java

...ojects/core/src/main/java/org/gradle/api/internal/tasks/RegisteredTaskInputFileProperty.java

subprojects/core/src/main/java/org/gradle/api/internal/tasks/TaskPropertySpec.java

subprojects/core/src/main/java/org/gradle/api/internal/tasks/properties/InputPropertySpec.java

...le/api/internal/tasks/properties/annotations/AbstractInputFilePropertyAnnotationHandler.java

...ojects/core/src/main/java/org/gradle/api/internal/tasks/execution/DefaultTaskProperties.java

...s/core/src/main/java/org/gradle/api/internal/tasks/properties/DefaultValidatingProperty.java

lptr · 2019-01-16T11:09:54Z

PropertyMetadata.getFieldName() should be getPropertyName()
DefaultFilePropertySpec doesn't need to implement Comparable<PropertySpec> explicitly

wolfs · 2019-01-17T07:50:42Z

@lptr I addressed your comments. PTAL!

lptr · 2019-01-17T14:15:12Z

subprojects/core/src/main/java/org/gradle/execution/plan/DefaultExecutionPlan.java

-                    public void visitOutputFileProperty(String propertyName, boolean optional, PropertyValue value, OutputFilePropertyType filePropertyType) {
-                        addCanonicalizedPaths(
+                    public void visitOutputFileProperty(final String propertyName, boolean optional, final PropertyValue value, final OutputFilePropertyType filePropertyType) {
+                        reportDeadlockOnParameterResolution(


This method name sounds like we already know there will be a deadlock. It should be more like something withDeadlockHandling() or something. WDYT?

Renamed to withDeadlockHandling.

Let's do the whole rename at once instead of piecewise.

wolfs added 12 commits January 10, 2019 10:51

Remove propertyName and value from PropertyValue

71848ba

This brings PropertyValue closer to a basic ValidatingValue.

Remove PropertyValue

938612a

ValidatingValue is enough to capture the actual value.

Don't use propertySpecFactory for input files

5c3408c

The visitor should handle all the creation

ab4c996

Especially the annotation handler should not need the file resolver.

Adapt visitInputProperty

5df2456

Adapt visitOutputFileProperty

b5c6f12

Adapt visitDestroyableProperty

9a18f47

Adapt visitLocalStateProperty

2904fca

Remove PropertySpecFactory

29386be

Use simplified visitor for task mutation info

1647d00

Get rid of TaskPropertySpecs in property package

a74a0c9

Move validation out of ValidatingValue

675ee62

wolfs added the from:member label Jan 14, 2019

wolfs self-assigned this Jan 14, 2019

wolfs requested a review from lptr January 14, 2019 14:19

wolfs force-pushed the wolfs/extract-property-specs branch 2 times, most recently from ac1b4b5 to 576cff9 Compare January 15, 2019 08:11

lptr reviewed Jan 15, 2019

View reviewed changes

wolfs force-pushed the wolfs/extract-property-specs branch from a9b7d4e to a93e431 Compare January 15, 2019 13:36

wolfs added 10 commits January 15, 2019 14:51

Some renames and moves

b98a552

Declared -> Registered Move ValidatingValue to properties package

Some polishing

f9b7327

Move more things to properties package

274ac12

Use Bintray version 1.8.0

e1de3a5

Looks like 0.4.1 is using `TaskValidator`.

Rename {Validating -> Property}Value

69efd80

The value doesn't validate any more.

Remove FileTreeValue

d69706d

Since validation happens separately now, we don't need to wrap the value.

Add back TaskValidator

acafeba

Fix overlapping-task-outputs-stats-init.gradle

bd61a1e

Remove resolver from PropertyWalker.visitProperties

e61406c

Remove InputPropertyAnnotationHandlerUtils

f9cdfbe

The method is so simple it makes no sense to extract it.

wolfs force-pushed the wolfs/extract-property-specs branch from a93e431 to f9cdfbe Compare January 15, 2019 14:00

wolfs added 2 commits January 15, 2019 22:41

Use only PathToFileResolver

4114c27

Remove TaskValidator

d4369a0

lptr reviewed Jan 16, 2019

View reviewed changes

wolfs added 8 commits January 16, 2019 15:36

Undo rename of TaskProperties

4de13a3

Address more review feedback

673164d

Add common superclasses for PropertySpec

f697186

Introduce common superclass for TaskFilePropertySpecs

0941190

Rename {Registered* -> *Registration}

20becbf

Rename PropertyMetadata.get{Field -> Property}Name

8fe627e

Rename LifecycleAware{Property -> Value}

c687241

Separate the two ValidatingProperty classes

16c0eb8

wolfs force-pushed the wolfs/extract-property-specs branch from 529444b to ed1dde1 Compare January 16, 2019 18:05

wolfs added 2 commits January 17, 2019 09:32

Re-use logic to resolve Map output properties

23446d2

Fix copyright header

e94630d

wolfs force-pushed the wolfs/extract-property-specs branch from ed1dde1 to e94630d Compare January 17, 2019 08:32

lptr reviewed Jan 17, 2019

View reviewed changes

wolfs added 2 commits January 17, 2019 16:24

Rename Parameter -> Property

05003ab

Let's do the whole rename at once instead of piecewise.

Rename method to handle deadlocks

330bafd

wolfs merged commit aaa71c9 into master Jan 18, 2019

wolfs deleted the wolfs/extract-property-specs branch January 18, 2019 09:20

wolfs mentioned this pull request Apr 15, 2019

Visit all task parameters when attaching the producer for output #9065

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make property specs independent of tasks #8231

Make property specs independent of tasks #8231

wolfs commented Jan 14, 2019

lptr left a comment

lptr Jan 15, 2019

wolfs Jan 15, 2019

lptr Jan 16, 2019

wolfs Jan 17, 2019

lptr Jan 17, 2019

wolfs Jan 17, 2019

lptr commented Jan 16, 2019

wolfs commented Jan 17, 2019

lptr Jan 17, 2019

wolfs Jan 17, 2019

	private static class Listener implements PropertyVisitor {
	private final Task task;

	public Listener(Task task) {
	this.task = task;
	}

	@Override
	public boolean visitOutputFilePropertiesOnly() {
	return true;
	}

	@Override
	public void visitInputFileProperty(String propertyName, boolean optional, boolean skipWhenEmpty, Class<? extends FileNormalizer> fileNormalizer, PropertyValue value, InputFilePropertyType filePropertyType) {
	throw new UnsupportedOperationException();
	}

	@Override
	public void visitInputProperty(String propertyName, PropertyValue value, boolean optional) {
	throw new UnsupportedOperationException();
	}

	@Override
	public void visitDestroyableProperty(Object value) {
	throw new UnsupportedOperationException();
	}

	@Override
	public void visitLocalStateProperty(Object value) {
	throw new UnsupportedOperationException();
	}

	@Override
	public void visitOutputFileProperty(String propertyName, boolean optional, PropertyValue value, OutputFilePropertyType filePropertyType) {
	value.attachProducer(task);
	}
	}

Make property specs independent of tasks #8231

Make property specs independent of tasks #8231

Conversation

wolfs commented Jan 14, 2019

lptr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lptr commented Jan 16, 2019

wolfs commented Jan 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment