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

refactor: LinkedList is in fact a Deque #222

Draft
wants to merge 53 commits into
base: master
Choose a base branch
from

Conversation

jerome-benoit
Copy link
Contributor

@jerome-benoit jerome-benoit commented Jan 17, 2024

Rationales:

  • The API exposed by the LinkedList is not what is expected for an API dedicated at manipulating linked list node or link data structure: { data, prev, next }
  • The current LinkedList API is what is expected for an API dedicated to non bounded Deque, except the missing pop() op
  • LinkedList in javascript is not really performant. Not sure it's worth the effort in the end at implementing them via a clean API making node or link a first class citizen
  • Offering non bounded efficient Deque implementation backward compatible with LinkedList is likely more useful to mnemonist users

Changelog:

  • Rename LinkedList related files to Deque related files
  • Deprecate LinkedList importation
  • Align the Deque API to the FixedDeque API: deprecate first(), last(), peek() ops, add peekFirst(), peekLast()
  • Add the pop() op by using a doubly linked list

Todo:

  • Optimize Deque implementation by using an array with two indexes start and end, or two arrays: benchmarks them
  • Factor out code between Deque and FixedDeque
  • Add ChangeLog entry
  • Update documentation

Jérôme Benoit added 30 commits January 2, 2024 19:46
Signed-off-by: Jérôme Benoit <[email protected]>
Signed-off-by: Jérôme Benoit <[email protected]>
Signed-off-by: Jérôme Benoit <[email protected]>
Signed-off-by: Jérôme Benoit <[email protected]>
Signed-off-by: Jérôme Benoit <[email protected]>
Signed-off-by: Jérôme Benoit <[email protected]>
Signed-off-by: Jérôme Benoit <[email protected]>
Signed-off-by: Jérôme Benoit <[email protected]>
Signed-off-by: Jérôme Benoit <[email protected]>
Signed-off-by: Jérôme Benoit <[email protected]>
Signed-off-by: Jérôme Benoit <[email protected]>
Signed-off-by: Jérôme Benoit <[email protected]>
A linked list API shall use a data structure as
{ data, next, prev} as a first class citizen.

Signed-off-by: Jérôme Benoit <[email protected]>
Signed-off-by: Jérôme Benoit <[email protected]>
Signed-off-by: Jérôme Benoit <[email protected]>
Jérôme Benoit added 20 commits January 15, 2024 19:21
Signed-off-by: Jérôme Benoit <[email protected]>
Signed-off-by: Jérôme Benoit <[email protected]>
Signed-off-by: Jérôme Benoit <[email protected]>
Signed-off-by: Jérôme Benoit <[email protected]>
Signed-off-by: Jérôme Benoit <[email protected]>
Signed-off-by: Jérôme Benoit <[email protected]>
Signed-off-by: Jérôme Benoit <[email protected]>
Signed-off-by: Jérôme Benoit <[email protected]>
Signed-off-by: Jérôme Benoit <[email protected]>
Signed-off-by: Jérôme Benoit <[email protected]>
@jerome-benoit jerome-benoit marked this pull request as draft January 17, 2024 18:27
@Yomguithereal
Copy link
Owner

Yomguithereal commented Feb 2, 2024

LinkedList in javascript is not really performant. Not sure it's worth the effort in the end at implementing them via a clean API making node or link a first class citizen

I agree with you. I have never seen the point of a dynamically sized Linked List in JavaScript and never had a use-case for it as it is never performant enough.

Personally I am pondering to drop it altogether from the library. However, I would like to introduce fixed size singly & doubly linked lists like those used internally in some other structures of the library such as the lru cache because those are efficient and can be useful. I will personally need one soon to properly implement a Chiba-Nishizeki routine to count triangles in a graph.

@jerome-benoit
Copy link
Contributor Author

jerome-benoit commented Feb 2, 2024

LinkedList in javascript is not really performant. Not sure it's worth the effort in the end at implementing them via a clean API making node or link a first class citizen

I agree with you. I have never seen the point of a dynamically sized Linked List in JavaScript and never had a use-case for it as it is never performant enough.

Happy to see you back. I hope it was because of holidays, not work :D

Personally I am pondering to drop it altogether from the library. However, I would like to introduce fixed size singly & doubly linked lists like those used internally in some other structures of the library such as the lru cache because those are efficient and can be useful. I will personally need one soon to properly implement a Chiba-Nishizeki routine to count triangles in a graph.

So, a roadmap:

  • deprecating the current LL API in favor of an unbounded Deque API
  • replacing the Deque internal design with array or double array as storage for performance under large size
  • reintroduce fixed singly LL with an API making linked list node a first class citizen
  • add fixed doubly LL with the same API

sounds reasonable?

@Yomguithereal
Copy link
Owner

Yomguithereal commented Feb 2, 2024

Happy to see you back. I hope it was because of holidays, not work :D

Unfortunately it's work. This and I maintain a lot of Open Source libraries.

deprecating the current LL API in favor of an unbounded Deque API

Why not, but I am not sure an actual linked list is the best implementation for a true unbounded Deque in JS. If I remember correctly the Queue of this library, for instance, has some linear array, amortized constant time implementation or such and does not rely on a Linked List (I think python and Rust deques also use some trick of the kind). In my mind the only fitting use (in higher level languages) of a doubly linked list is to move an item around in constant time and the only fitting use of a singly linked list is to have efficient multimaps or collision handling when you don't know in advance the order of insertion.

(I am only noticing now that you spoke about this in your comment here: "Optimize Deque implementation by using an array with two indexes start and end, or two arrays: benchmarks them")

What I mean, in the end, is that I am not sure a lot of people are using mnemonist for the Linked List anyway and that it would probably not be very costly to trash it, even without a transition phase. I see the value in adding an unbounded Deque class in the library though.

reintroduce fixed singly LL with an API making linked list node a first class citizen

I don't think we need a node class at all, because we can rely on representing them by numerical ids if the list is bounded. This is lighter and does not require allocation of node objects etc. It has some drawbacks because those ids are basically pointers that you can use to hurt yourself if you are not careful.

@jerome-benoit
Copy link
Contributor Author

deprecating the current LL API in favor of an unbounded Deque API

Why not, but I am not sure an actual linked list is the best implementation for a true unbounded Deque in JS.

It's not, but it's an intermediate step for introducing a fast unbounded Deque implementation.

(I am only noticing now that you spoke about this in your comment here: "Optimize Deque implementation by using an array with two indexes start and end, or two arrays: benchmarks them")

The subsequent steps are indeed targeted at making the Deque implementation lean and fast using JS.

reintroduce fixed singly LL with an API making linked list node a first class citizen

I don't think we need a node class at all, because we can rely on representing them by numerical ids if the list is bounded. This is lighter and does not require allocation of node objects etc. It has some drawbacks because those ids are basically pointers that you can use to hurt yourself if you are not careful.

Do you have an example code of using such an optimisation? What will be the exact underlying storage data structure?

I do not understand the trick here.

Jérôme Benoit added 2 commits February 6, 2024 14:01
Signed-off-by: Jérôme Benoit <[email protected]>
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