Skip to content

constellation: Optimize tracing of maybe_close_random_pipeline#39035

Merged
jschwe merged 1 commit intoservo:mainfrom
jschwe:jschwender/trace_maybe_close_random_pipeline
Oct 24, 2025
Merged

constellation: Optimize tracing of maybe_close_random_pipeline#39035
jschwe merged 1 commit intoservo:mainfrom
jschwe:jschwender/trace_maybe_close_random_pipeline

Conversation

@jschwe
Copy link
Copy Markdown
Member

@jschwe jschwe commented Aug 30, 2025

Most of the time this function does nothing and returns immediately, so there isn't really a point in tracing it.
If one wanted to know when a pipeline is closed, one could add a trace point after the early return.
However, during review it was suggested that we currently don't need this information and thus just remove the trace.

Testing: Not required.

Copy link
Copy Markdown
Member

@mrobinson mrobinson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I would prefer not to trace this function at all. It's just used for testing, so we shouldn't be too worried about the performance.

Most of the time this function does nothing
and returns immediately,
so there isn't really a point in tracing it.
If one wanted to know when a pipeline is closed,
one could add a trace point **after** the early return.
However during review it was suggested that we currently don't need this
information and thus just remove the trace.

Signed-off-by: Jonathan Schwender <[email protected]>
@jschwe jschwe force-pushed the jschwender/trace_maybe_close_random_pipeline branch from 32989f6 to 77ecf7d Compare October 24, 2025 13:47
@servo-highfive servo-highfive added the S-awaiting-review There is new code that needs to be reviewed. label Oct 24, 2025
@jschwe
Copy link
Copy Markdown
Member Author

jschwe commented Oct 24, 2025

@mrobinson I came back to this PR, updated the description and just removed the trace point as you suggested.

@servo-highfive servo-highfive removed the S-awaiting-review There is new code that needs to be reviewed. label Oct 24, 2025
@mrobinson
Copy link
Copy Markdown
Member

@jschwe Thanks!

@jschwe jschwe enabled auto-merge October 24, 2025 14:13
@jschwe jschwe added this pull request to the merge queue Oct 24, 2025
@servo-highfive servo-highfive added the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Oct 24, 2025
Merged via the queue into servo:main with commit 63f9a32 Oct 24, 2025
34 checks passed
@jschwe jschwe deleted the jschwender/trace_maybe_close_random_pipeline branch October 24, 2025 15:46
@servo-highfive servo-highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Oct 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants