Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

coin_processor optimization: compile regex once #333

Conversation

keyliaran
Copy link
Contributor

Currently, the regular expression is compiled anew with each iteration of the loop. This compilation process is resource-intensive, and there's no built-in caching mechanism. Consider utilizing lazy_static to cache the compiled result for optimization.

@larry-aptos
Copy link
Collaborator

looks good! can you add how do you test this change? ty

@keyliaran
Copy link
Contributor Author

looks good! can you add how do you test this change? ty

Currently, there are no unit tests for the indexer. I've conducted tests by running both the old and new versions on the same batch of events and confirmed that the resulting databases are identical.

@banool
Copy link
Collaborator

banool commented Apr 9, 2024

Hey thanks for the PR! Could you use once_cell::Lazy instead? I believe it is recommended these days over lazy_static, better API / types rather than macro magic.

@keyliaran
Copy link
Contributor Author

Hey thanks for the PR! Could you use once_cell::Lazy instead? I believe it is recommended these days over lazy_static, better API / types rather than macro magic.

Hi! Made an update, pls take a look!

rust/Cargo.lock Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey! Mind removing the Cargo.lock changes from the PR? There aren't any changes to the Cargo.toml so it shouldn't be necessary to include

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@keyliaran keyliaran force-pushed the coin_processor/regex_compilation_optimization branch from 003b390 to 5a7d703 Compare April 17, 2024 20:49
@grao1991
Copy link
Contributor

Great! It seems CoinBalance::from_write_resource consumes 5x less CPU with your change. Can you fix the lint and have your PR merged?

@keyliaran
Copy link
Contributor Author

Great! It seems CoinBalance::from_write_resource consumes 5x less CPU with your change. Can you fix the lint and have your PR merged?

Sure, fixed

@grao1991 grao1991 force-pushed the coin_processor/regex_compilation_optimization branch from 5aac566 to 3ef816b Compare April 24, 2024 21:14
@grao1991 grao1991 merged commit c4c012c into aptos-labs:main Apr 24, 2024
3 of 5 checks passed
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.

5 participants