MergeTree: Reimplement list #11625
MergeTree: Reimplement list #11625
Conversation
github-actions
bot
added
area: dds
| @@ -3,142 +3,127 @@ | |||
| * Licensed under the MIT License. | |||
| */ | |||
|
|
|||
There was a problem hiding this comment.
this file is best view in split most, or view the file directly, as it is a complete re-write
⯅ @fluid-example/bundle-size-tests: +17.53 KB
Baseline commit: cbb9ed6 |
| public headNode: HeadNode<T>; | ||
| private readonly _list?: List<T>; | ||
| constructor(list: List<T> | undefined) { | ||
| this.headNode = this; |
There was a problem hiding this comment.
is this different from the sugar you use to initialize _next and _prev? assuming not, i'd prefer consistent style
There was a problem hiding this comment.
yes. as when called by the DataNode which inherit from head node they will pass this and set it as whatever head they are bound to.
There was a problem hiding this comment.
ha. but looks like i don't even use it that way 🤦♂️. i wrote this a while back, and just peeling off changes. i'll make it more consistent
| @@ -279,8 +279,9 @@ export class LocalReferenceCollection { | |||
| public removeLocalRef(lref: LocalReferencePosition): LocalReferencePosition | undefined { | |||
| if (this.has(lref)) { | |||
| assertLocalReferences(lref); | |||
| // eslint-disable-next-line @typescript-eslint/no-non-null-assertion | |||
| ListRemoveEntry(lref.getListNode()!); | |||
| lref.getListNode()?.list?.remove( | |||
There was a problem hiding this comment.
consider assigning lref.getListNode() to a local
| return false; | ||
| }, true); | ||
| const taken: SegmentGroup[] = []; | ||
| while (taken.length < count && node) { |
There was a problem hiding this comment.
we should implement this in O(N) since it does look like it's used in potential production codepaths, even if for potentially rare scenarios (group ops--not sure how much people use that). Should just be able to use .push and then .reverse at the end.
There was a problem hiding this comment.
group are mainly use for reconnecting, as during rebase things could split.
is the problem that unshift is enumerates all elements, which requires another loop? i think i can even skip the reverse by using an initialized array, and a for loop
There was a problem hiding this comment.
yeah, unshift is O(N) as it moves all of the array elements over. Using an initialized array + for loop is fine too.
Co-authored-by: Abram Sanderson <Abram.sanderson@gmail.com>
| @@ -113,18 +113,18 @@ export class Client { | |||
| * It is used to get the segment group(s) for the previous operations. | |||
| * @param count - The number segment groups to get peek from the tail of the queue. Default 1. | |||
There was a problem hiding this comment.
Not related to your change, but I wouldn't know without looking at the code what order the returned SegmentGroups were in. It would be helpful to update the comment, optionally.
Description
The pre-existing list class was inconsistent in terms of typing, naming and semantics which made it very hard to use and extend. It also had a performance issue where whether an item was in a list, which is frequently used by local references was O(N).
This change fully re-implements the list class. It remains a circularly linked list, but separates the list and node implementation for better typing, and adds a reference to the head node to each list node to support O(1) determining if a node is in a particular list. Additionally, all methods have been implemented to match array methods in terms of shape and semantics as closely as possible to ease usage for those familiar with javascript arrays. The new implementation does regress the unused clear method, which is now O(N) rather than O(1) as each not must be detached from the head node as well.