-
Notifications
You must be signed in to change notification settings - Fork 124
Skip list priority implementation #4287
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
Conversation
|
I’m wondering why it was necessary to mark a node as dead instead of deleting it directly. I know a regular list works this way, and I have no objection to it, just want to understand the rationale behind this. |
dthain
left a comment
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.
Overall, this is looking good, and I like how you have simplified several taskvine issues while maintaining basically the same interface to the list. I have a couple of persnickety naming fixups, but no major concerns.
|
@dthain Sounds good! @JinZhou5042 It is so that we can have more than one cursor pointing to the same node. We do not want to delete a node to which a cursor is pointing to as that will cause a segfault. Additionally, this allows us to 'delete' the item a cursor is pointing to during an iteration, which simplifies the places where we have to call 'next'. |
|
@btovar Got it! I think this is a nice and very maintanable implementation, especially the design in |
|
any concerns if I merge this pr? I have been running VineReduce+cortado on top it successfully. |
|
I think if you can run Cortado successfully with a clean valgrind, then I am comfortable merging. |
This implementation has only one cursor. Let's see if it is enough as compared to work queue.
Merge Checklist
The following items must be completed before PRs can be merged.
Check these off to verify you have completed all steps.
make testRun local tests prior to pushing.make formatFormat source code to comply with lint policies. Note that some lint errors can only be resolved manually (e.g., Python)make lintRun lint on source code prior to pushing.