Unify BuildOperationExecutor and BuildOperationProcessor APIs #1848

jjohannes · 2017-04-19T07:18:29Z

This introduces the following BuildOperationExecutor
interface (as outlined in #1676):

  void run(RunnableBuildOperation buildOperation);

  <T> T call(CallableBuildOperation<T> buildOperation);

  <O extends RunnableBuildOperation> void runAll(
                    Action<BuildOperationQueue<O>> schedulingAction);

  <O extends BuildOperation> void runAll(
                    BuildOperationWorker<O> worker,
                    Action<BuildOperationQueue<O>> schedulingAction);

To accomplish this, the following changes were performed:

Various representation of build operations have been merged into

BuildOperation (with sub-interfaces)
-> declare and describe a build operation
BuildOperationDescriptor (BuildOperationDescriptor.Builder)
-> describe a build operation
BuildOperationState
-> represents a running build operation, with run state, start time,
parent relationship information, and description

The DefaultBuildOperationExecutor and DefaultBuildOperationProcessor
implementations have been merged in DefaultBuildOperationExecutor,
which is now build session scoped.
Automatic parent/child association was improved (TODOs were added
for further improvements).

jjohannes · 2017-04-19T07:18:45Z

https://builds.gradle.org/viewQueued.html?itemId=2748058

jjohannes · 2017-04-19T07:45:37Z

...jects/base-services/src/main/java/org/gradle/internal/progress/BuildOperationDescriptor.java

-public class BuildOperationDetails {
-    private final BuildOperationExecutor.Operation parent;
+public class BuildOperationDescriptor {
+    private final Long id;


I felt we should move away from Object id. We could also use org.gradle.internal.logging.events.OperationIdentifier everywhere. But that basically just wraps a Long. And since the ID is passed around more and more (build scan plugin, worker processes) Long might be a simple and pragmatic solution here.

I see that there is also discussion about this in the context of #1838. We could also address it there or later (and change it back to Object for now).

We should move OperationIdentifier over to the build operation package and use it there. I think having an ID class is a good idea instead of using a plain long, as it makes it less likely to accidentally compare apples to oranges.

We don't need to do this as part of this change though.

Maybe it would be better then to create a new class BuildOperationIdentifier and use that. OperationIdentifier is currently used for the ids of other kinds of operations as well. So we would still allow compare apples to oranges situation.

Afaik it's currently only used for progress events, which should be referring to build operations anyway. They shouldn't have their own id.

I was just reading this comment: #1838 (review)
...which suggests otherwise.

There is at least one place outside of BuildOperationExecutor that creates OperationIdentifiers.
(

gradle/subprojects/logging/src/main/java/org/gradle/internal/logging/progress/DefaultProgressLoggerFactory.java

Line 55 in dc6eb2d

return new ProgressLoggerImpl((ProgressLoggerImpl) parentOperation, nextId.getAndIncrement(), loggerCategory, progressListener, timeProvider);

)

Maybe a way forward would be to introduce BuildOperationIdentifier now and eventually removed OperationIdentifier at a later point if we migrated more things, including the ProgressLogger related classes, to build operations.

❌ For now let's switch back to the previous behavior of these IDs and clean that up in another step. I.e. use Object here and set it to the current OperationIdentifier.

Alright. Changed back to previous behavior.

jjohannes · 2017-04-19T07:48:32Z

...jects/base-services/src/main/java/org/gradle/internal/progress/BuildOperationDescriptor.java

        this.name = name;
        this.displayName = displayName;
        this.progressDisplayName = progressDisplayName;
        this.operationDescriptor = operationDescriptor;


This is not ideal (field operationDescriptor inside the class BuildOperationDescriptor). We should probably rename one of them. In any case, we have to keep it backward compatible for the build scan plugin.

⭕️ We can add another getter with a better name and deprecate the old one. Maybe something like getDetails

Renamed to details (and added getOperationDescriptor() to BuildOperationInternal for backwards compatibility).

oehme

This is a great simplification! There are a few issues we need to sort out, but most of my comments are just stylistic.

oehme · 2017-04-19T12:10:22Z

subprojects/base-services/src/main/java/org/gradle/internal/operations/BuildOperation.java

 */
 public interface BuildOperation {
    /**
     * Returns a human consumable name for this operation.


⭕️ Outdated Javadoc

Well spotted. Changed it.

oehme · 2017-04-19T12:15:33Z

...rojects/base-services/src/main/java/org/gradle/internal/operations/BuildOperationWorker.java

     */
    String getDisplayName();
+
+    void execute(O buildOperation, @Nullable BuildOperationState parent);


❌ The workers shouldn't need to know about parent operations. Instead, we can use the same abstraction we had before with the wrapping ParentBuildOperationAwareWorker.

Right. When I pulled that up I did not have the full overview of how the parent/child relationship works.

I thought other worker implementations could also make use of the parent if they somehow would reconnect to the BuildOperationExecutor to create more build operations on their own. But since we do not do something like that (yet). Or might do it differently in the future, we may as well leave it like it was.

I reverted the interface change and reintroduced the ParentBuildOperationAwareWorker.

oehme · 2017-04-19T12:18:47Z

...rojects/base-services/src/main/java/org/gradle/internal/operations/BuildOperationWorker.java

 * Implementations must be thread-safe.
 */
-public interface BuildOperationWorker<T extends BuildOperation> extends Action<T> {
+public interface BuildOperationWorker<O extends BuildOperation> {


⭕️ This abstraction is there so you can execute the same build operations using different implementations, e.g. compiling a native file with gcc vs. clang. The word worker here might be a little confusing, since we also have the concept of "workers" (threads/processes) in the same area of our code base. Maybe something closer to the word "Tool" would be better.

What about using Tool then? We already have a Tool interface in native, which is describing how to call an external tools (args etc.).

Maybe ExecutableTool?

I created a follow up issue: #1860

oehme · 2017-04-19T12:20:42Z

...s/base-services/src/main/java/org/gradle/internal/operations/DefaultBuildOperationQueue.java

        @Override
        public String toString() {
-            return "Worker ".concat(worker.getDisplayName()).concat(" for operation ").concat(operation.getDescription());
+            return "Worker ".concat(worker.getDisplayName()).concat(" for ").concat(operation.getClass().getSimpleName());


❌ Should use the display name of the operation details here

The problem is that the description has to be build first (operation.description().build()). I did not want to do that just for the sake of toString().

I changed it to build it once (if toString is called) and cache it in the Holder.

oehme · 2017-04-19T12:22:53Z

...jects/base-services/src/main/java/org/gradle/internal/progress/BuildOperationDescriptor.java

-public class BuildOperationDetails {
-    private final BuildOperationExecutor.Operation parent;
+public class BuildOperationDescriptor {
+    private final Long id;


❌ For now let's switch back to the previous behavior of these IDs and clean that up in another step. I.e. use Object here and set it to the current OperationIdentifier.

oehme · 2017-04-19T12:51:28Z

subprojects/core/src/main/java/org/gradle/internal/progress/DefaultBuildOperationExecutor.java

+    private BuildOperationDescriptor.Builder createDescriptorBuilder(BuildOperation buildOperation, BuildOperationState parent) {
+        long id = nextId.getAndIncrement();
+        BuildOperationDescriptor.Builder descriptorBuilder = buildOperation.description();
+        if (descriptorBuilder == null) {


❌ Should we really allow this? Does this case happen somewhere currently?

Currently we do not have that case (only in tests). Yeah... we probably do not want to allow that.

I remove it and adjust the tests.

oehme · 2017-04-19T12:51:48Z

subprojects/core/src/main/java/org/gradle/internal/progress/DefaultBuildOperationExecutor.java

    @Nullable
-    private OperationDetails findCurrentOperation() {
-        OperationDetails current = currentOperation.get();
+    private ProgressLogger startProgressLogging(BuildOperationState currentOperation) {


⭕️ maybeStart

oehme · 2017-04-19T12:53:13Z

subprojects/core/src/main/java/org/gradle/internal/progress/DefaultBuildOperationExecutor.java

        // TODO:pm Move this to WARN level once we fixed maven-publish, see gradle/gradle#1662
        LOGGER.debug("WARNING No operation is currently running in unmanaged thread: {}", Thread.currentThread().getName());
-        UnmanagedThreadOperation operation = UnmanagedThreadOperation.create(timeProvider.getCurrentTime());
+        long id = UNMANAGED_THREAD_OPERATION_COUNTER.getAndDecrement();


⭕️ Why did we remove the UnmanagedThreadOperation class? It seems like a useful encapsulation of this logic.

changed back (see earlier comment)

oehme · 2017-04-19T13:06:36Z

subprojects/base-services/src/main/java/org/gradle/internal/operations/BuildOperationState.java

+    private final BuildOperationDescriptor description;
+    private final long startTime;
+
+    public BuildOperationState(BuildOperationDescriptor.Builder descriptorBuilder, BuildOperationState parent, long startTime) {


❌ Should take the built description instead of calling .build() itself.

oehme · 2017-04-19T13:07:31Z

...s/base-services/src/main/java/org/gradle/internal/operations/DefaultBuildOperationQueue.java

    private final AtomicBoolean canceled = new AtomicBoolean();

-    DefaultBuildOperationQueue(WorkerLeaseService workerLeases, ExecutorService executor, BuildOperationWorker<T> worker) {
+    DefaultBuildOperationQueue(WorkerLeaseService workerLeases, ExecutorService executor, BuildOperationWorker<O> worker, @Nullable BuildOperationState parent) {


❌ Shouldn't need to know about the parent.

Reverted (see comment above)

adammurdoch

A few things I think we should fix up.

I think this PR attempts to do too much. It changes 3 independent things:

The API through which clients schedule async build operations.
The way in which client define build operations.
The way in which build operations are presented to event consumers.

It would have been better to make each of these changes separately.

adammurdoch · 2017-04-19T23:18:32Z

subprojects/base-services/src/main/java/org/gradle/internal/operations/BuildOperation.java

     * Returns a human consumable name for this operation.
     */
-    String getDescription();
+    BuildOperationDescriptor.Builder description();


This should line up with Describable. We should continue to evolve a common concept of things that have display names.

Can you elaborate some more what direction you propose? One alternative that Jendrik and I discussed was having individual methods like getShortDescription, getDescription, getProgressDescription etc. That would make it easy to do BuildOperation extends Describable. We'd need a convenience base class for the very common case that most of these fields are null though.

adammurdoch · 2017-04-19T23:20:55Z

...jects/base-services/src/main/java/org/gradle/internal/operations/BuildOperationExecutor.java

+    /**
+     * Returns the operation being run by the current thread.
+     *
+     * TODO remove this. It is needed to create parent/child relationships between operation created in different threads.


I'm not sure we want to remove this. We will need this kind of method, and the one below, to ship context between processes. What it should do, however, is expose only the id.

changed (see comment below)

adammurdoch · 2017-04-19T23:34:22Z

subprojects/core/src/main/java/org/gradle/execution/taskgraph/DefaultTaskGraphExecuter.java

-            TaskOperationDescriptor taskOperation = new TaskOperationDescriptor(task);
-            BuildOperationDetails buildOperationDetails = BuildOperationDetails.displayName("Task " + task.getIdentityPath()).name(task.getIdentityPath().toString()).parent(parentOperation).operationDescriptor(taskOperation).build();
-            buildOperationExecutor.run(buildOperationDetails, new Action<BuildOperationContext>() {
+            buildOperationExecutor.setRootOperationOfCurrentThread(parentOperation);


This should be one of the parameters of the run() method, like it was. A client should be able to describe the context in which work is happening directly on the work, rather than through magic static state.

Should also clear the magic static state at the completion of the work.

Yes. I was thinking in another direction. But you are right. I changed that back. We now only have getCurrentOperationId() and getCurrentParentOperationId() on the executor. I am not sure if we need the latter on the long run.

Exposing only the ID (and no further state) led to some more changes in the executor implementation. The main issue was to determine the runtime state of the parent for the is parent running check. I ended up with maintaining a set of running operation ids. I think this is the cheapest way to handle this.

adammurdoch · 2017-04-19T23:46:50Z

subprojects/workers/src/main/java/org/gradle/workers/internal/DefaultWorkerExecutor.java

                    WorkerFactory workerFactory = fork == ForkMode.ALWAYS ? workerDaemonFactory : workerInProcessFactory;
                    Worker<ActionExecutionSpec> worker = workerFactory.getWorker(WorkerDaemonServer.class, workingDir, daemonForkOptions);
-                    return worker.execute(spec, currentWorkerWorkerLease, currentBuildOperation);
+                    buildOperationExecutor.setRootOperationOfCurrentThread(currentBuildOperation);


need to clear the static state after execution.

setRootOperationOfCurrentThread () was removed again (see comment above)

oehme · 2017-04-21T08:40:44Z

...jects/base-services/src/main/java/org/gradle/internal/operations/BuildOperationExecutor.java

+     * @throws IllegalStateException When the current thread is not executing an operation.
+     */
+    @Nullable
+    Object getCurrentParentOperationId();


❌ Can be removed, no one ever calls this.

Used by DefaultTestExecuter. Now moved to BuildOperationState interface as discussed.

oehme · 2017-04-21T09:06:34Z

...rojects/base-services/src/main/java/org/gradle/internal/operations/BuildOperationWorker.java

     */
    String getDisplayName();
+
+    void execute(O buildOperation);


❌ Should take the operation context as an argument.

At the moment, custom workers need to handle everything themselves (building description, notifying listeners etc.) - not only execution.

The execute() method of the DefaultBuildOperationExecutor is not used here. In fact, custom workers might use custom BuildOperations (i.e. not use RunnableBO or CallableBO).

We can discuss if we really want that behavior. But I think that's a different discussion.

That's a newly introduced bug on this branch and we need to fix it. The worker needs to be wrapped so that it executes through the same code path as everything else. See the behavior on master.

This could mean we're missing test coverage.

Ok I got that wrong.

I think the confusion was created for me when Executor and Processor were initially combined. The Worker with which the queue factory is configured is now always supposed to be "a worker" that works through the executor (firing build operation events etc.). As such, there should never be a use case were a custom worker (i.e. implementation of BuildOperationWorker) is directly passed to the queue factory.

This actually became clearer now as I added operation context as argument to BuildOperationWorker.

I use a separate interface now for initialising the queue factory - BuildOperationQueue.QueueWorker.

The previous behavior should now be restored.

oehme · 2017-04-21T09:08:27Z

...jects/base-services/src/main/java/org/gradle/internal/progress/BuildOperationDescriptor.java

+        /**
+         * Define the parent of the operation by ID. Needs to be the ID of an operations that is running at the same time
+         * the described operation is running.
+         * If parent ID is not set, The last started operation of the current thread will be used as parent.


⭕️ Of the executing thread

oehme · 2017-04-21T09:10:41Z

subprojects/core/src/main/java/org/gradle/api/internal/plugins/DefaultPluginManager.java

    }

-    private BuildOperationDetails computeApplyPluginBuildOperationDetails(PluginImplementation<?> pluginImplementation) {
+    private BuildOperationDescriptor.Builder computeApplyPluginBuildOperationDetails(PluginImplementation<?> pluginImplementation) {


⭕️ Should be inside the build operation class

Missed that one. Thanks. Moved it.

oehme · 2017-04-21T09:13:12Z

subprojects/core/src/main/java/org/gradle/execution/taskgraph/TaskPlanExecutor.java


 /**
- * Will be merged with {@link org.gradle.internal.operations.BuildOperationProcessor}
+ * Will be merged with {@link org.gradle.internal.operations.BuildOperationExecutor}


Feel free to skip this.

oehme · 2017-04-21T09:16:09Z

subprojects/core/src/main/java/org/gradle/internal/progress/DefaultBuildOperationExecutor.java

-            parentId = parent.id;
+    public <O extends BuildOperation> void runAll(BuildOperationWorker<O> worker, Action<BuildOperationQueue<O>> schedulingAction) {
+        executeInParallel(worker, schedulingAction);
+        maybeStopUnmanagedThreadOperation();


❌ I think this needs to be done in a finally block

Sound good. Added to all 4 occurrences.

oehme · 2017-04-21T09:16:48Z

subprojects/core/src/main/java/org/gradle/internal/progress/DefaultBuildOperationExecutor.java

+    private <T> T execute(BuildOperation buildOperation) {
+        BuildOperationDescriptor descriptor = createDescriptor(buildOperation);
+        BuildOperationState currentOperation = new BuildOperationState(descriptor, timeProvider.getCurrentTime());
+        BuildOperationDescriptor description = currentOperation.getDescription();


❌ Duplicate variable

oehme · 2017-04-21T09:26:19Z

subprojects/core/src/main/java/org/gradle/internal/progress/DefaultBuildOperationExecutor.java

+    }
+
+    private BuildOperationState maybeStartUnmanagedThreadOperation(BuildOperationDescriptor.Builder buildOperationDescriptorBuilder) {
+        if (!GradleThread.isManaged() && currentOperation.get() == null && (buildOperationDescriptorBuilder == null || !buildOperationDescriptorBuilder.hasParentId())) {


❌ desriptorBuilder should never be null

Simplified/clarified by passing the parent state as argument directly.

oehme · 2017-04-21T09:26:54Z

subprojects/core/src/main/java/org/gradle/internal/progress/DefaultBuildOperationExecutor.java

+            currentOperation.set(operation);
+            listener.started((BuildOperationInternal) operation.getDescription(), new OperationStartEvent(operation.getStartTime()));
+        }
+        return currentOperation.get();


⭕️ return operation would be clearer

oehme · 2017-04-21T09:29:58Z

subprojects/core/src/main/java/org/gradle/internal/progress/DefaultBuildOperationExecutor.java

    private final AtomicLong nextId = new AtomicLong();
-    private final ThreadLocal<OperationDetails> currentOperation = new ThreadLocal<OperationDetails>();
+    private final ThreadLocal<BuildOperationState> currentOperation = new ThreadLocal<BuildOperationState>();
+    private final Set<Object> runningOperationIds = Collections.synchronizedSet(new HashSet<Object>());


❌ We shouldn't introduce locking into this hot code path. The previous solution with state.running only used a volatile read, which is more efficient. We should either go back to that or use a concurrent map.

Changed as discussed:

Reintroduced getCurrentOperation() that returns the state of the operation but exposes only id and parentId through the public interface.

This introduces the following `BuildOperationExecutor` interface (as outlined in #1676): void run(RunnableBuildOperation buildOperation); <T> T call(CallableBuildOperation<T> buildOperation); <O extends RunnableBuildOperation> void runAll( Action<BuildOperationQueue<O>> schedulingAction); <O extends BuildOperation> void runAll( BuildOperationWorker<O> worker, Action<BuildOperationQueue<O>> schedulingAction); To accomplish this, the following changes were performed: - Various representation of build operations have been merged into 1) BuildOperation (with sub-interfaces) -> declare and describe a build operation 2) BuildOperationDescriptor (BuildOperationDescriptor.Builder) -> describe a build operation 3) BuildOperationState -> represents a running build operation, with run state, start time, parent relationship information, and description - The DefaultBuildOperationExecutor and DefaultBuildOperationProcessor implementations have been merged in DefaultBuildOperationExecutor, which is now build session scoped.

Review items have been addressed

jjohannes added a:feature A new functionality from:member labels Apr 19, 2017

jjohannes self-assigned this Apr 19, 2017

jjohannes requested a review from oehme April 19, 2017 07:18

jjohannes commented Apr 19, 2017

View reviewed changes

oehme suggested changes Apr 19, 2017

View reviewed changes

oehme added this to the 4.0 RC1 milestone Apr 19, 2017

adammurdoch previously requested changes Apr 19, 2017

View reviewed changes

jjohannes force-pushed the jj-build-operation-executor branch from 1420904 to 472ae7c Compare April 20, 2017 08:04

jjohannes mentioned this pull request Apr 20, 2017

Find a better name for what is now called Worker in the context of build operation execution #1860

Closed

jjohannes force-pushed the jj-build-operation-executor branch from 6064b13 to dd9b4dd Compare April 20, 2017 17:24

oehme suggested changes Apr 21, 2017

View reviewed changes

jjohannes force-pushed the jj-build-operation-executor branch from 56e6f2b to 4839a7c Compare April 21, 2017 15:22

jjohannes mentioned this pull request Apr 21, 2017

Add operation descriptor to Configure Project build operation #1852

Merged

oehme force-pushed the jj-build-operation-executor branch 2 times, most recently from 873e22d to 8165cc4 Compare April 24, 2017 09:59

oehme force-pushed the jj-build-operation-executor branch from 8165cc4 to 0b1d937 Compare April 24, 2017 11:14

oehme approved these changes Apr 24, 2017

View reviewed changes

oehme merged commit 57e44c7 into master Apr 24, 2017

oehme deleted the jj-build-operation-executor branch April 24, 2017 18:28

Unify BuildOperationExecutor and BuildOperationProcessor APIs #1848

Unify BuildOperationExecutor and BuildOperationProcessor APIs #1848

Uh oh!

Conversation

jjohannes commented Apr 19, 2017

Uh oh!

jjohannes commented Apr 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oehme Apr 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oehme Apr 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oehme left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oehme Apr 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jjohannes Apr 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adammurdoch left a comment

Choose a reason for hiding this comment

Uh oh!

oehme Apr 19, 2017 •

edited

Loading

oehme Apr 19, 2017 •

edited

Loading

oehme Apr 19, 2017 •

edited

Loading

jjohannes Apr 20, 2017 •

edited

Loading