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

added files for fine-grained linked list as a set #79

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

DeltaCube23
Copy link

Implementation of a Fine-grained linked list (as a set) based on optimistic synchronisation referred from section 9.6 in art of multiprocessor programming. The add function requires only 1 lock, and the remove function requires 2 locks. The linked list works like a set, with elements arranged in ascending order.

match temp with
| Some _ when temp == cur && prev == verify -> insert prev
| None when cur = None && prev == verify -> insert prev
| _ ->
Copy link
Contributor

@polytypic polytypic Jul 13, 2023

Choose a reason for hiding this comment

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

You could also use a simple conditional

if prev == verify && cur == verify.next then
  insert prev
else
  ...

here. This will also be slightly faster, because there is no need to distinguish between Some _ and None.

let add t value =
(* x will point to the new node after insertion *)
let insert x =
if x.value = value && not (x == t.head) then (
Copy link
Contributor

@polytypic polytypic Jul 13, 2023

Choose a reason for hiding this comment

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

It is usually best to give the notion of equality over values as a parameter, i.e. there should be something like t.eq x.value value here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually you want t.compare x.value value = 0, because the values need to be ordered.

| Some node -> node
| _ ->
is_tail := true;
create_node value None
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm... Is the node created here used for something?

is_tail := true;
create_node value None
in
if !is_tail then (
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you avoid allocating a ref cell for the is_tail condition by inlining the continuation branches to the match:

match cur with
| None ->
    Mutex.unlock prev.lock;
    false
| Some to_find ->
    Mutex.lock to_find.lock;
    let verify = find_previous_remove t value in
    (* ... *)

let temp = verify.next in
match temp with
| Some _ when temp == cur && prev == verify ->
let res = to_find.value = value in
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe the logic around here could be simplified and the equality check can be moved outside of the critical section. Something like this:

      let verify = find_previous_remove t value in
      if prev == verify && cur == verify.next then begin
          Mutex.unlock prev.lock;
          Mutex.unlock to_find.lock;
          to_find.value = value (* better to use user defined equality *)
      end
      else begin
          Mutex.unlock prev.lock;
          Mutex.unlock to_find.lock;
          let again = find_previous_remove t value in
          validate again again.next
      end

The common prefix of both branches could also be factored out.

let find_previous_add t key =
let rec aux prev next =
match next with
| Some node when node.value > key -> prev
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm... I wonder whether the logic and add and remove could be modified slightly so that only a single find_previous helper would be needed without impacting performance.

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.

2 participants