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

perf(log)!: LazyHash for producing txs and blocks debug hashes #4340

Open
wants to merge 14 commits into from

Conversation

wesl-ee
Copy link
Contributor

@wesl-ee wesl-ee commented Oct 25, 2024

Closes #4167

Generalizes LazyBlockHash as LazyHash so now Txs and Blocks are hashed only on evaluation of the debug printf

#go-api-breaking

@wesl-ee wesl-ee requested review from a team as code owners October 25, 2024 20:55
Copy link
Contributor

@melekes melekes left a comment

Choose a reason for hiding this comment

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

Thanks @wesl-ee ❤️

I think we can now remove fmt.Sprintf because it's HexBytes

mempool/clist_mempool.go Outdated Show resolved Hide resolved
mempool/clist_mempool.go Outdated Show resolved Hide resolved
mempool/clist_mempool.go Outdated Show resolved Hide resolved
mempool/clist_mempool.go Outdated Show resolved Hide resolved
mempool/clist_mempool.go Outdated Show resolved Hide resolved
mempool/clist_mempool.go Outdated Show resolved Hide resolved
mempool/clist_mempool.go Outdated Show resolved Hide resolved
mempool/clist_mempool.go Outdated Show resolved Hide resolved
mempool/clist_mempool.go Outdated Show resolved Hide resolved
mempool/clist_mempool.go Outdated Show resolved Hide resolved
@melekes
Copy link
Contributor

melekes commented Oct 26, 2024


libs/log/lazy.go:36:34: unexported-return: exported func NewLazyHash returns unexported type *log.lazyHash, which can be annoying to use (revive)
func NewLazyHash(inner hashable) *lazyHash {
                                 ^
test/e2e/tests/app_test.go:96:47: unnecessary conversion (unconvert)
		require.Equal(t, res.Hash, cmtbytes.HexBytes(hash))

@melekes melekes changed the title perf: LazyHash Txs and blocks for debug perf!: LazyHash Txs and blocks for debug Oct 26, 2024
@melekes melekes added breaking A breaking change logs Anything relating to logging labels Oct 26, 2024
Copy link
Contributor

@andynog andynog left a comment

Choose a reason for hiding this comment

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

Thanks @wesl-ee. Overall looks good to me. Could you please check @melekes suggestions ? Also, since this seems to be breaking change, could you also please add a changelog entry.

mempool/clist_mempool.go Outdated Show resolved Hide resolved
libs/log/lazy.go Show resolved Hide resolved
@cason cason changed the title perf!: LazyHash Txs and blocks for debug perf!(log): LazyHash for producing txs and blocks debug hashes Oct 29, 2024
@cason cason changed the title perf!(log): LazyHash for producing txs and blocks debug hashes perf(log)!: LazyHash for producing txs and blocks debug hashes Oct 29, 2024
Copy link
Contributor

@cason cason left a comment

Choose a reason for hiding this comment

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

Thank you for this, the solution looks very elegant.

Left some comments.

.changelog/unreleased/breaking-changes/4340-lazy-hash.md Outdated Show resolved Hide resolved
mempool/clist_mempool.go Outdated Show resolved Hide resolved
types/tx.go Show resolved Hide resolved
libs/log/lazy.go Outdated Show resolved Hide resolved
libs/log/lazy.go Outdated Show resolved Hide resolved
libs/log/lazy_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@andynog andynog left a comment

Choose a reason for hiding this comment

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

thanks for adding the changelog entry

@wesl-ee wesl-ee requested a review from cason October 30, 2024 02:36
Copy link
Contributor

@melekes melekes left a comment

Choose a reason for hiding this comment

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

👍

@melekes melekes added this pull request to the merge queue Oct 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking A breaking change logs Anything relating to logging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

mempool: log.NewLazySprintf("%X", entry.Tx().Hash()) does not improve performance
4 participants