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

Compute event hashes in separate thread [Verifiable events part 4] #868

Merged
merged 8 commits into from Jun 25, 2021

Conversation

m4ksio
Copy link
Contributor

@m4ksio m4ksio commented Jun 18, 2021

As noted in previous PR, computing hashes can be CPU intensive, hence similar approach as with block commitments - separate comiter whish shouldn't block execution of further collections

Copy link
Contributor

@yhassanzadeh13 yhassanzadeh13 left a comment

Choose a reason for hiding this comment

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

LGTM

engine/execution/computation/computer/computer.go Outdated Show resolved Hide resolved
go func() {
bc.Run()
wg.Done()
}()

go func() {
eh.Run()
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense to make the eventHasher a ReadyDoneAware implementation?

// ReadyDoneAware provides easy interface to wait for module startup and shutdown
type ReadyDoneAware interface {
Ready() <-chan struct{}
Done() <-chan struct{}
}

Copy link
Member

@ramtinms ramtinms left a comment

Choose a reason for hiding this comment

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

👏

Base automatically changed from m4ksio/5473-verify-service-events-v2 to master June 23, 2021 22:54
@codecov-commenter
Copy link

codecov-commenter commented Jun 25, 2021

Codecov Report

Merging #868 (02d52c7) into master (9b26087) will decrease coverage by 0.01%.
The diff coverage is 90.90%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #868      +/-   ##
==========================================
- Coverage   56.89%   56.87%   -0.02%     
==========================================
  Files         429      429              
  Lines       25635    25652      +17     
==========================================
+ Hits        14584    14589       +5     
- Misses       9079     9086       +7     
- Partials     1972     1977       +5     
Flag Coverage Δ
unittests 56.87% <90.90%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
engine/execution/messages.go 100.00% <ø> (ø)
model/flow/event.go 73.68% <ø> (ø)
engine/execution/computation/computer/computer.go 87.19% <90.00%> (+0.23%) ⬆️
engine/execution/ingestion/engine.go 55.02% <100.00%> (-1.31%) ⬇️
module/chunks/chunkVerifier.go 65.34% <100.00%> (ø)
engine/verification/match/chunks.go 83.33% <0.00%> (-6.67%) ⬇️
engine/verification/match/engine.go 65.10% <0.00%> (-1.08%) ⬇️
cmd/util/ledger/migrations/storage_v4.go 42.16% <0.00%> (+0.60%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9b26087...02d52c7. Read the comment docs.

@m4ksio m4ksio merged commit 683fdd1 into master Jun 25, 2021
@m4ksio m4ksio deleted the m4ksio/5473-compute-hash-in-commiter branch June 25, 2021 02:13
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.

None yet

4 participants