-
Notifications
You must be signed in to change notification settings - Fork 264
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
fix: memory leak in Import
#773
Conversation
don't need to reference the children, which recursively retain the whole subtree in memory.
don't reference the children, which recursively retain the whole subtree in memory.
@@ -55,7 +55,7 @@ var ( | |||
metadataKeyFormat = keyformat.NewKeyFormat('m', 0) // m<keystring> | |||
) | |||
|
|||
var errInvalidFastStorageVersion = fmt.Errorf("Fast storage version must be in the format <storage version>%s<latest fast cache version>", fastStorageVersionDelimiter) | |||
var errInvalidFastStorageVersion = fmt.Errorf("fast storage version must be in the format <storage version>%s<latest fast cache version>", fastStorageVersionDelimiter) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious why this change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There a lint warns about the leading upper case in string
leftNode := i.stack[stackSize-2] | ||
rightNode := i.stack[stackSize-1] | ||
|
||
node.leftNode = leftNode | ||
node.rightNode = rightNode | ||
node.leftNodeKey = leftNode.nodeKey | ||
node.rightNodeKey = rightNode.nodeKey | ||
node.size = leftNode.size + rightNode.size |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to me that you moved leftNode and rightNode to vars and re-used, but isn't that the same has node.leftNode
and node.rightNode
or does GO have an issue with using the object?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's equivalent, it's just I think it makes code shorter this way, because the node.leftNode will be repeated many times
// remove the recursive references to avoid memory leak | ||
leftNode.leftNode = nil | ||
leftNode.rightNode = nil | ||
rightNode.leftNode = nil | ||
rightNode.rightNode = nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Guess this is the actual fix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
removing what comments? i didnt touch anything other than merge. If i deleted a comment it would say a comments were deleted, not sure what youre talking about |
@tac0turtle the comments are back, probably a hiccup within GH API, basically I left the comments above and you merged the PR - I saw the event merged and the comments were no were so I assumed you had deleted them to merge/don't stale. Apologies! 🙇 |
Closes: #772
don't need to reference the children, which recursively retain the whole subtree in memory.