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

Better document _formatNode #109

Merged
merged 1 commit into from
Apr 15, 2020
Merged

Better document _formatNode #109

merged 1 commit into from
Apr 15, 2020

Conversation

ryanio
Copy link
Contributor

@ryanio ryanio commented Apr 14, 2020

This PR:

  • Adds typedoc for _formatNode and _walkTrie
  • Fixes typos extention => extension

While refactoring I found that _formatNode has a parameter remove that is not being utilized inside the method in baseTrie.ts but is being used in checkpointTrie.ts. It is called with the value being true in two cases: here and here.

In its history, the remove parameter was used alongside if (this.isCheckpoint) on whether to remove the node by pushing a del op to the opStack.

Asking @s1na, he mused:

thinking about it for a few mins, it doesn't occur to me why nodes are being removed from db only when the tree is during a checkpoint
so technically you don't need to remove the nodes from the underlying db, they can become orphan, as in no nodes in the trie references them
it probably has something to do with committing a checkpoint. because during a commit, you have to copy all these staged changes (those in the scratchdb) to the main db
so maybe it's an optimization, to avoid more copying?

So, is the remove parameter checked with isCheckpoint() just an optimization for copying less data to the db in a commit()? Are there any other uses or cases? There doesn't seem to be any test cases covering these scenarios.

@github-actions
Copy link

Coverage Status

Coverage increased (+1.2%) to 96.197% when pulling 99d2d6b on fixFormatNodeRemove into 41ef2d8 on master.

@s1na
Copy link
Contributor

s1na commented Apr 14, 2020

I went through the scrolls we've inherited to track down the change 😄 Seems like it was added in 1546716#diff-168726dbe96b3ce427e7fedce31bb0bcR597 and modified again in 236b43a#diff-168726dbe96b3ce427e7fedce31bb0bcR535. Couldn't find any explanation so far.

Btw if this is not only a performance optim, I expect the change in behaviour to happen in VM tests when you drop the remove flag. So that'd be interesting to test as well.

And removing it every time (in baseTrie) could itself have a performance penalty. Right now by not deleting nodes from db we're trading memory for speed.

@ryanio
Copy link
Contributor Author

ryanio commented Apr 14, 2020

@s1na ah thanks, you are right, I didn't consider that ignoring deletion of the nodes could actually primarily be for the performance boost. I will modify this PR to better document some of this instead of modifying any behavior.

@ryanio ryanio changed the title Fix node removal behavior of _formatNode Better document _formatNode Apr 14, 2020
_formatNode(node: TrieNode, topLevel: boolean, opStack: BatchDBOp[], remove: boolean = false) {
console.log('ENTERING HERE')
Copy link
Member

Choose a reason for hiding this comment

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

Here is a console.log left. Is this otherwise ready for review?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks got it. yes ready for review, i'll publish, just ended up being some typedoc additions and typo fixes.

* Fix typos e.g. extention => extension
@ryanio ryanio marked this pull request as ready for review April 15, 2020 17:40
Copy link
Member

@holgerd77 holgerd77 left a comment

Choose a reason for hiding this comment

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

LGTM

@ryanio ryanio merged commit 7583aa9 into master Apr 15, 2020
@ryanio ryanio mentioned this pull request Apr 17, 2020
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.

3 participants