Skip to content
This repository has been archived by the owner on Jan 19, 2021. It is now read-only.

Fix double serialize in CheckpointTrie._formatNode() #127

Merged

Conversation

holgerd77
Copy link
Member

This PR fixes the double-execution of Node serialization in CheckpointTrie._formatNode() as well as moves a unified _formatNode() to BaseTrie (the added somewhat dummy isCheckpoint() getter there avoids a TypeScript error) to remove redundancy.

This gives a significant performance increase when running the checkpointing benchmarks:

Run on PR:

$ ts-node benchmarks/checkpointing.ts && ts-node benchmarks/checkpointing.ts && ts-node benchmarks/checkpointing.ts && ts-node benchmarks/checkpointing.ts && ts-node benchmarks/checkpointing.ts
benchmarks/checkpointing.ts | average execution time: 0s 148.515ms
benchmarks/checkpointing.ts | average execution time: 0s 146.228ms
benchmarks/checkpointing.ts | average execution time: 0s 150.828ms
benchmarks/checkpointing.ts | average execution time: 0s 146.222ms
benchmarks/checkpointing.ts | average execution time: 0s 152.767ms

$ ts-node benchmarks/checkpointing.ts && ts-node benchmarks/checkpointing.ts && ts-node benchmarks/checkpointing.ts && ts-node benchmarks/checkpointing.ts && ts-node benchmarks/checkpointing.ts
benchmarks/checkpointing.ts | average execution time: 0s 146.98ms
benchmarks/checkpointing.ts | average execution time: 0s 144.115ms
benchmarks/checkpointing.ts | average execution time: 0s 156.19ms
benchmarks/checkpointing.ts | average execution time: 0s 148.622ms
benchmarks/checkpointing.ts | average execution time: 0s 149.105ms

Run on master:

$ ts-node benchmarks/checkpointing.ts && ts-node benchmarks/checkpointing.ts && ts-node benchmarks/checkpointing.ts && ts-node benchmarks/checkpointing.ts && ts-node benchmarks/checkpointing.ts
benchmarks/checkpointing.ts | average execution time: 0s 172.482ms
benchmarks/checkpointing.ts | average execution time: 0s 176.066ms
benchmarks/checkpointing.ts | average execution time: 0s 181.225ms
benchmarks/checkpointing.ts | average execution time: 0s 180.5ms
benchmarks/checkpointing.ts | average execution time: 0s 172.593ms

$ ts-node benchmarks/checkpointing.ts && ts-node benchmarks/checkpointing.ts && ts-node benchmarks/checkpointing.ts && ts-node benchmarks/checkpointing.ts && ts-node benchmarks/checkpointing.ts
benchmarks/checkpointing.ts | average execution time: 0s 183.217ms
benchmarks/checkpointing.ts | average execution time: 0s 173.598ms
benchmarks/checkpointing.ts | average execution time: 0s 174.573ms
benchmarks/checkpointing.ts | average execution time: 0s 172.899ms
benchmarks/checkpointing.ts | average execution time: 0s 172.908ms

@coveralls
Copy link

Coverage Status

Coverage increased (+0.6%) to 95.29% when pulling 40b02ee on fix-double-serialize-in-checkpoint-trie-format-node into 7add2b2 on master.

})
// Do not use TrieNode.hash() here otherwise serialize()
// is applied twice (performance)
const hashRoot = keccak(rlpNode)
Copy link
Member

Choose a reason for hiding this comment

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

I was a bit confused here but this is the "unpacked" node.hash() function where we just hash this raw rlpNode data.

Copy link
Member

@jochem-brouwer jochem-brouwer left a comment

Choose a reason for hiding this comment

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

Looks good, great find! 😄

@holgerd77 holgerd77 merged commit 2a048f4 into master Aug 31, 2020
@holgerd77 holgerd77 deleted the fix-double-serialize-in-checkpoint-trie-format-node branch August 31, 2020 12:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants