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

chore: use string builder instead of string concatenation in catchup service #5572

Merged
merged 2 commits into from
Jul 21, 2023

Conversation

kehiy
Copy link
Contributor

@kehiy kehiy commented Jul 16, 2023

Summary

hello Algorand team
I replaced string concatenation with a builder, that's faster and more efficient, concatenation with + is also too expensive.

@kehiy kehiy changed the title Builder fix: Builder Jul 16, 2023
@codecov
Copy link

codecov bot commented Jul 16, 2023

Codecov Report

Merging #5572 (407cf2f) into master (d316914) will decrease coverage by 0.02%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #5572      +/-   ##
==========================================
- Coverage   55.79%   55.77%   -0.02%     
==========================================
  Files         446      446              
  Lines       63418    63420       +2     
==========================================
- Hits        35381    35371      -10     
- Misses      25668    25678      +10     
- Partials     2369     2371       +2     
Impacted Files Coverage Δ
catchup/service.go 71.88% <0.00%> (+1.19%) ⬆️

... and 13 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@kehiy
Copy link
Contributor Author

kehiy commented Jul 21, 2023

@bbroder-algo @winder can you review this?

Copy link
Contributor

@winder winder left a comment

Choose a reason for hiding this comment

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

LGTM

@winder winder requested a review from a team July 21, 2023 11:58
@winder
Copy link
Contributor

winder commented Jul 21, 2023

Thanks for the contribution

@winder winder changed the title fix: Builder chore: use string builder instead of string contatination in catchup service Jul 21, 2023
@bbroder-algo bbroder-algo changed the title chore: use string builder instead of string contatination in catchup service chore: use string builder instead of string concatenation in catchup service Jul 21, 2023
Copy link
Contributor

@bbroder-algo bbroder-algo left a comment

Choose a reason for hiding this comment

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

no forks

@bbroder-algo bbroder-algo merged commit 4a5095e into algorand:master Jul 21, 2023
Copy link
Contributor

@jannotti jannotti left a comment

Choose a reason for hiding this comment

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

I'd rather see the whole s defined in one piece using backticks to avoid all the double quotes, the escaping, and concatenation altogether.

@kehiy kehiy deleted the builder branch July 21, 2023 14:43
@@ -744,17 +745,19 @@ func (s *Service) fetchRound(cert agreement.Certificate, verifier *agreement.Asy
if cert.Round == fetchedCert.Round &&
cert.Proposal.BlockDigest != fetchedCert.Proposal.BlockDigest &&
fetchedCert.Authenticate(*block, s.ledger, verifier) == nil {
s := "!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!\n"
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is the test for this change?

Copy link
Contributor

Choose a reason for hiding this comment

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

fwiw, I ran it in go playground and it worked.

@jannotti
Copy link
Contributor

BenchmarkPlusEq-8 1515818 779.0 ns/op
BenchmarkBuilder-8 3102158 384.8 ns/op
BenchmarkConstant-8 1000000000 0.3593 ns/op

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants