Fix missing check output by allowing disabled workunits to re-enable themselves (cherrypick of #14854, #14856, #14934)
#14942
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ased on the level (pantsbuild#14854) This change cleans up a few TODOs around workunit storage. Workunit names are almost entirely `&'static str`, with the notable exception of `NodeKey::Task` names (which are the Python function names of `@rules`). But pantsbuild#14683 began interning `Task` structs, which made `Task` names static as well. This change moves to storing workunit names as `&'static str`. Additionally, because the `Level` is a field of the `WorkunitMetadata`, the `WorkunitMetadata` was forced to be constructed, even for workunits whose level meant that they would not be rendered. Since the `WorkunitMetadata` contains heavy fields like the string description and user-metadata, this change moves to constructing the `WorkunitMetadata` via lazily evaluated `key=value` arguments to the `in_workunit!` macro (which also has the advantage of being more concise). Finally: an unused argument to `in_workunit!` is removed. Speeds up common runs by ~2%. [ci skip-build-wheels]
As described in pantsbuild#14680: due to memoization, workunits are most accurately represented as a DAG, rather than as a tree. This PR changes the _storage_ of workunits from a tree to a DAG by giving workunits multiple parents. But it does not yet actually add multiple parents to a workunit, which will be accomplished in a followup change. This portion is worth landing separately because it eases the fix for pantsbuild#14867, while the actual addition of multiple parents is not necessary for that fix. # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
…e themselves (pantsbuild#14934) pantsbuild#13483 broke the rendering of `EngineAwareReturnType` implementations which relied on starting a workunit at `Level::TRACE`, and then escalating its level to something visible (usually `INFO` or greater) when it completed. `check` outputs for the JVM were strongly affected (see pantsbuild#14867), since they relied on the fact that `FallibleClasspathEntry` escalates to `ERROR` to render compile errors. To resolve this, we roll back a portion of pantsbuild#13483. Rather than not recording the workunit at all, we instead record it in the `WorkunitStore` as "disabled", which is signaled by a workunit not having any `WorkunitMetadata`. This has some of the efficiency benefits of pantsbuild#13483 (because we continue to skip heap allocating the metadata's fields), but allows a workunit to escalate itself from disabled to enabled as it completes by specifying a non-disabled level in `RunningWorkunit::update_metadata`. The recording of "disabled" workunits is additionally necessary for pantsbuild#14680, because otherwise workunits which were not actually recorded would break the tracking of multiple parents: when adding a new parent to a workunit, you need an existing `SpanId` corresponding to the work that you are adding a parent to (or else you might accidentally depend on a parent arbitrarily far up the stack). Fixes pantsbuild#14867. # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
tdyas
approved these changes
Mar 29, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix missing
checkoutput by allowing disabled workunits to re-enable themselves (#14934)#13483 broke the rendering of
EngineAwareReturnTypeimplementations which relied on starting a workunit atLevel::TRACE, and then escalating its level to something visible (usuallyINFOor greater) when it completed.checkoutputs for the JVM were strongly affected (see #14867), since they relied on the fact thatFallibleClasspathEntryescalates toERRORto render compile errors.To resolve this, we roll back a portion of #13483. Rather than not recording the workunit at all, we instead record it in the
WorkunitStoreas "disabled", which is signaled by a workunit not having anyWorkunitMetadata. This has some of the efficiency benefits of #13483 (because we continue to skip heap allocating the metadata's fields), but allows a workunit to escalate itself from disabled to enabled as it completes by specifying a non-disabled level inRunningWorkunit::update_metadata.The recording of "disabled" workunits is additionally necessary for #14680, because otherwise workunits which were not actually recorded would break the tracking of multiple parents: when adding a new parent to a workunit, you need an existing
SpanIdcorresponding to the work that you are adding a parent to (or else you might accidentally depend on a parent arbitrarily far up the stack).Fixes #14867.
[ci skip-build-wheels]