Skip to content

Conversation

smklein
Copy link
Collaborator

@smklein smklein commented Sep 24, 2025

  • Adds PoolBuilder constructor for Pool objects
  • Adds a collect_backtraces override option during pool construction
  • Sets collect_backtraces = false for some tests

Fixes #9064

};

let policy = policy.unwrap_or_else(|| Policy::default());
let collect_backtraces = collect_backtraces.unwrap_or(true);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is where we match the current behavior for backtrace collection by default: unless requested otherwise, collect_backtraces = true.

///
/// Creating this pool does not necessarily wait for connections to become
/// available, as backends may shift over time.
pub fn new(log: &Logger, resolver: &QorbResolver) -> Self {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since we already have three different constructors for a Pool...

  • I didn't want to add an argument to all of them
  • I didn't want to add a fourth constructor

So I switched to a builder pattern instead.

@smklein
Copy link
Collaborator Author

smklein commented Sep 24, 2025

It's N = 1, but, observing the junit XML output for the ubuntu build-and-test (which is much more stable than the helios variant):

  • This PR shows a total time of 2702.7 seconds
  • Last green on main shows a total time of 2997.2 seconds

This supports your theory from #9064 (comment) - that the collection of backtraces on every collection had a non-negligible overhead across the whole test suite.

This PR should still give us the benefits of collecting backtraces in production, but avoid the cost for our test suite.

@smklein smklein marked this pull request as ready for review September 24, 2025 21:51
@smklein smklein requested a review from davepacheco September 24, 2025 21:52
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.

Test performance regression in f4239876
1 participant