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

Delete performance would benefit from improvement #17

Closed
lkarlslund opened this issue Oct 22, 2022 · 4 comments
Closed

Delete performance would benefit from improvement #17

lkarlslund opened this issue Oct 22, 2022 · 4 comments

Comments

@lkarlslund
Copy link
Contributor

Very nice library for concurrent maps! For scenarios where you need to delete keys one at a time (not batching), the current performance makes it unusable.

I have an analysis application (https://github.com/lkarlslund/adalanche) and I've tried replacing some of the maps with yours (adding unsafe.Pointer in my fork).

With a nasty workaround where I don't delete keys, but set the value to a deleted flag, it works fairly good. But this is not the way.

Also looking at your code, I'm curious how you distinguish from a hash which is ^0 and the "marked" value which is the same?

@lkarlslund lkarlslund changed the title Delete performance needs improvement Delete performance would benefit from improvement Oct 22, 2022
@alphadose
Copy link
Owner

@lkarlslund agreed regarding the performance of deletion, I will make deletion lazy in nature (items will be deleted during Set()) and Del() will just add a marked node in front of the node we want deleted. Then during next iteration on that part of the map, all delete marked nodes will be removed with a CAS

Also looking at your code, I'm curious how you distinguish from a hash which is ^0 and the "marked" value which is the same?

Currently they are indistinguishable. One of the things in my TODO list was to reserve the first bit for all keyhashes to mark whether a node is deleted or not. Let me try this out then get back to you.

@lkarlslund
Copy link
Contributor Author

I haven't looked at the internals, but it looks like you're pushing a deletion node before the node that should be deleted. Why not just mark this node for deletion directly?
Or is that what you want to do with using one bit, so you can preserve ordering at the same time?

@alphadose
Copy link
Owner

alphadose commented Oct 25, 2022

@lkarlslund

Or is that what you want to do with using one bit, so you can preserve ordering at the same time?

I will use index masking to use only the last 63 bits for comparison. Here is an example snippet for the same

const mask uintptr = 1 << 63 - 1

// comparison
if a.KeyHash & mask > b.KeyHash & mask { // do something after comparison }

// check for deletion by checking the first bit
if a.KeyHash & (1 << 63) { println("item = deleted") } 

I haven't looked at the internals, but it looks like you're pushing a deletion node before the node that should be deleted. Why not just mark this node for deletion directly?

This is also a great method but I would have to store an extra field isDeleted of 1 byte uint8 in each node thereby increasing the overall size of the map. Thats why I add a marked deleted node in front of a node which I want to delete and then actually remove it lazily during traversal.

In my opinion both methods are good, I will just check the performance of both approaches and then implement one.

@alphadose
Copy link
Owner

@lkarlslund deletion performance has improved for single elements with 5a39538

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

No branches or pull requests

2 participants