Skip to content

Conversation

rootulp
Copy link
Collaborator

@rootulp rootulp commented Aug 19, 2025

Closes #2329

Benchmarks

Huge shout out to @rach-id for running the benchmarks for me 🙏

CheckTx time Average Max
Before 60ms 2160ms
After 44ms 860ms

Tests

  • Unit test added to verify the mutex is no longer held for CheckTx or CheckTxAsync
  • Needs more testing to ensure this change doesn't introduce a data race

To do

  • Remove traces from this PR
  • Add a data race test either in this repo or in celestia-app

Copy link
Contributor

@cmwaters cmwaters left a comment

Choose a reason for hiding this comment

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

This looks right to me (assuming we add a mutex on the cosmos-sdk side)

Do we have benchmarks yet of the impact of this change?

@rootulp
Copy link
Collaborator Author

rootulp commented Aug 20, 2025

Oops not yet. Rachid and I tried yesterday with Talis but hit a few issues so will retry today before merging.

@rach-id rach-id marked this pull request as draft August 20, 2025 06:14
@rach-id
Copy link
Member

rach-id commented Aug 20, 2025

Converting to draft to avoid accidental merge

@rootulp
Copy link
Collaborator Author

rootulp commented Aug 20, 2025

FWIW: #2299 has traces around the CAT mempool's CheckTx invocation so if we want to refactor this PR to not add a method to the Client interface, we can just use that PR (or extract the CheckTx tracing to this PR).

@rootulp
Copy link
Collaborator Author

rootulp commented Aug 20, 2025

We're using this config.toml to override a bunch of settings: https://gist.github.com/rootulp/e7a101a4468a68367bd6aa3f7bd08a2a in talis

Update: that config.toml + my experiment didn't work because no traces were captured but Rachid re-ran the benchmarks using a different tracing mechanism which worked.

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.

Move or remove the mutex on the application when applicable
3 participants