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

feat: a new userData API Remove #1117

Merged
merged 4 commits into from Oct 8, 2021
Merged

feat: a new userData API Remove #1117

merged 4 commits into from Oct 8, 2021

Conversation

tylitianrui
Copy link
Contributor

issue
a new API for deleting a key instead of setting nil.

  • a key is removed logically . adding a new attribute status to userDataKV which report whether the userDataKV is deleted or not.
  • the memory of the deleted one will be reuse,

@tylitianrui tylitianrui changed the title feat: userData new api "delete" feat: a new userData API Remove Oct 8, 2021
@erikdubbelboer
Copy link
Collaborator

Why ʼstatusDeletedʼ why not just remove the entry from the slice?

@tylitianrui
Copy link
Contributor Author

Why ʼstatusDeletedʼ why not just remove the entry from the slice?

removing an entry from the slice will migrate other entries following the removed one forward . it causes memory migration. removing an entry logically can avoiding the migration.

@erikdubbelboer
Copy link
Collaborator

I'm not sure what you mean with memory migration but rearranging the slice isn't a bad thing you can just copy the last entry of the slice to the one that gets deleted and decrease the len by 1. Just make sure to set the value of that last entry to nil before you decrease the length so the GC doen't keep it alive.

userdata.go Outdated
for i := 0; i < n; i++ {
kv := &args[i]
if string(kv.key) == key {
kv.status = statusDeleted
Copy link
Collaborator

Choose a reason for hiding this comment

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

So here something like:

n--
args[i] = args[n]
args[n].value = nil // Make sure not to keep the value alive for the GC.
args = args[:n]
*d = args

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 for reviewing, i will change it.

@erikdubbelboer erikdubbelboer merged commit 7fdd526 into valyala:master Oct 8, 2021
@erikdubbelboer
Copy link
Collaborator

Thanks!

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

2 participants