[Do Not Merge] PoC of cyclic previous node technique

[lo48576]

This is a PoC implementation of #12.

I noticed:

• Updating neighbors are O(1) cost, if all the nodes have parents.
  • indextree does not provide implicit root nodes, and toplevel nodes can have siblings without parents.
    For such nodes, updating neighbors are O(num_of_siblings).
    

    indextree/src/relations.rs

    Lines 83 to 90 in 8e7fafa

    	// NOTE: These are not O(1) operations.
    	// This is because nodes are allowed to having no children.
    	// If the node has no children, it is impossible to get the first
    	// and the last siblings.
    	Some(id) => (
    	id.preceding_siblings(arena).last(),
    	id.following_siblings(arena).last(),
    	),

• Now all neighors getters (such as Node::parent()) should receive &Arena<T>.
  This is not intrinsic difficutly, because when people wanted to get NodeId from Node<T>, then they would have &Arena<T>.
  (However, I feel it is just annoying to pass &arena as an argument all the time).

If this change is worth merging, then I think indextree should support some mechanism for pseudo-parent node or implicit root node to make updates constant time.

[saschagrunert]

If this change is worth merging, then I think indextree should support some mechanism for pseudo-parent node or implicit root node to make updates constant time.

Do you see any drawback in adding an implicit root node? I would suggest moving forward with that approach... :)

[lo48576]

If implicit root node exists, top-level nodes (created by the user) may have siblings, though they were not told to have.
To treat siblings relations of top-level nodes correctly, the arena should have multiple implicit nodes (I'm not sure they can be called "root" nodes :-))

let mut arena = Arena::new();
let n1a = arena.new_node("1a");
let n1b = arena.new_node("1b");
let n2 = arena.new_node("2");
// arena
// `-- (implicit 1)
//     `-- 1a
// `-- (implicit 2)
//     `-- 1b
// `-- (implicit 3)
//     `-- 2
n1a.insert_after(n1b, &mut arena);
// arena
// `-- (implicit 1)
//     |-- 1a
//     `-- 1b
// `-- (implicit 3)
//     `-- 2

Mainly there are two problems:

1. I don't want users to manipulate implicit nodes manually, because they are implicit and non-usual node.
   To achive this, NodeId should not be created from index number directly (or users may accidentally access that node).
   • UPDATE: Fixed by Remove NodeId::new() and make NodeId::from_non_zero_usize() crate-local #48.
2. To refer implicit nodes by NodeId, they should be in Arena::nodes field, but it contains non-optional pub data: T field, and implicit nodes should have no values.
   • UPDATE: Fixed by Hide Node::data field and export Node::get{,_mut}() #52.
     However, I'm not sure how the internal representation of implicit nodes should look like.

I think they (implicit nodes) bring some complexity and exceptional rule, and it may make the code hard to read and maintain...

[lo48576]

Required breaking change for this feature is additional parameter for neighbors accessors (such as Node::previous_sibling(self, &Arena<T>) instead of Node::previous_sibling(self)).
(Current state of this branch is already working, except that updates are not always O(1).
I think no more interface changes are necessary.)

If you really want this, you can change the interface before implementing internals.
Then, later internal changes can be non-breaking and new release with this feature will be safely done with revision bump.

[phrohdoh]

In one of my uses of indextree I do want all top-level/parentless nodes to be siblings.
How is that use-case affected by these changes?

[lo48576]

TL;DR: I don't think this change is worth doing with non-obvious breaking change...

---

The problem of the current PR is that insertion and detach of top-level nodes are not O(N).

Possible solutions (currently I'm aware of) are:

1. Allow O(N) operations for top-level siblings.
   • In this case, your code will get slower when adding and detaching top-level sibling nodes.
2. Add virtual "root" node internally to arena.
   • NodeId value generation or internal structure of NodeId may be changed (so that it can represent virtual node).
     • This may also require data access API to be changed (because the virtual root node has no data).
   • NodeId might (or might not) lose some good properties, such as PartialOrd and/or predictability.
     • I think this won't affect usual use case, but I'm not sure.
   • The maximum number of nodes in an arena might be reduced.
3. Force users to have a "root" node for each arena.
   • This hugely breaks compatibility and affects many users, so I think this won't happen...
4. Give up using cyclic previous node, i.e. make no change.

Choice 1 will cause performance degration.
Choice 2 and 3 requires non-obvious breaking change, i.e. migration will be non-straightforward and difficult.
Choice 4 is default (current) situation when there are no progress.

This change reduces memory consumption a little, but also make the implementation more complex and may require semantics to change.
I'm not willing to push this PR forward.

[lo48576]

Closing as the branch is outdated and I won't maintain this experimental PoC anymore.