Tree arena #752

KGrewal1 · 2024-11-17T19:16:33Z

I've had a try at implementing the unsafe tree_arena in a separate lib (but in the same workspace for now) - haven't thought of a good way to make non root access not O(depth), without slowing down insertion or using more memory, though have improved accessing nodes from the root to O(1) from O(depth) and accessing direct children to O(1) from O(children)

KGrewal1 · 2024-11-17T20:29:49Z

I am definitely doing something silly to break the needed preconditions, just need to work out what

Edit:
Think its just the reference to the Item being moved on insertion of new items, as its a reference to an element of the hashmap which can be invalidated

PoignardAzur · 2024-11-18T14:13:29Z

haven't thought of a good way to make non root access not O(depth), without slowing down insertion or using more memory,

~~The eventual plan will be to use a slotmap.~~

EDIT: Wait, no, I misunderstood the question.

PoignardAzur · 2024-11-18T14:34:11Z

(Copying from zulip)

Your PR looks good, but we'd probably want more intermediary steps before relying on custom unsafe code; especially since the TreeArena API isn't quite settled yet.

Those steps would be:

Moving TreeArena to a separate crate without changing the code.
Adding a test suite.
Adding a command-line flag so we can switch between the safe implementation and the efficient implementation.

The test suite would help us test the unsafe version with MIRI, which we'd probably add to CI.

PoignardAzur

This is a very high-quality contribution, and will be good to merge once the review points are addressed.

Something orthogonal to this review is the question of how the TreeArena API will evolve.

Having an unsafe implementation may slow us down if we want to try API changes and we have to make sure they're implemented in both versions.

I'd like to establish that the safe version will be a first-class citizen and the unsafe version a second-class citizen:

The safe version may have features / APIs that the unsafe version doesn't yet have.
If both versions are at feature parity, Masonry can switch on the unsafe version for best performance.
Otherwise, Masonry uses the safe version.

If that's what we go with, we should documented that pattern in a few places.

PoignardAzur · 2024-11-27T15:25:52Z

tree_arena/ARCHITECTURE.md

+## Architecture
+


In my view, the point of an ARCHITECTURE.md file is to give just enough context to understand everything about a codebase, not just the public API.

As such, I think this file should start with a description of the safe-and-unsafe implementations and why we include both.

PoignardAzur · 2024-11-27T15:26:00Z

tree_arena/ARCHITECTURE.md

+Of finding children: $O(1)$ - previously $O(\text{children})$
+
+Of finding deeper descendants: $O(\text{depth})$ - ideally will be made $O(1)$
+
+Access from the root: $O(1)$, previously $O(\text{depth})$ - improved as all nodes are known to be descended from the root


I'd rather avoid Latex for this section. ARCHITECTURE.md is mostly meant to be read in a code editor, you can use plain text.

Also, I think the descriptions could be a little clearer.

I can see an argument for using the GitHub Markdown MathJax extension. But for big-O notation, $$O(1)$$ or $$\mathcal{O}(1)$$ is not that much better than O(1)

On further thinking, given readme is so bare, does it make more sense to flesh this out more, but also move it into Readme rather than having a superfluous file?

Yeah. The general workflow I like is "README for things you need to understand the project, ARCHITECTURE for things you need to understand the project's internals that you wouldn't get from the README or the doc root".

PoignardAzur · 2024-11-27T15:30:31Z

tree_arena/ARCHITECTURE.md

+It is possible to get shared (immutable) access or exclusive (mutable) access to the tree. These return `ArenaRef<'arena, T>` or `ArenaMut<'arena, T>` respectively
+


I feel like this description could go into a little more detail about how the shared/exclusive access works, and why this crate is worth bothering with.

The general idea of TreeArena is to express a tree-like ownership graph, stored inside a flat data structure. The UnsafeCells are meant to bridge the gap between the two.

tree_arena/src/lib.rs

tree_arena/src/tree_arena_safe.rs

tree_arena/src/tree_arena_unsafe.rs

tree_arena/tests/basic_tests.rs

masonry/src/contexts.rs

KGrewal1 · 2024-11-27T19:11:45Z

Regarding testing with miri in CI, the invocation I think is:

cargo +nightly miri t -p tree_arena --no-fail-fast --no-default-features

but I'm unsure how best to access nightly rust in the workflow to run this

Imberflur · 2024-11-28T05:19:44Z

tree_arena/src/tree_arena_unsafe.rs

+    /// # SAFETY
+    ///
+    /// When using this on [`ArenaMutChildren`] associated with some node,
+    /// must ensure that `id` is a descendant of that node, otherwise can
+    /// obtain two mutable references to the same node
+    unsafe fn find_mut(&mut self, id: impl Into<NodeId>) -> Option<ArenaMut<'_, T>> {


Hi! I have a minor note on the safety documentation. This is sort of a comment out of nowhere, but I've developed an interest in reviewing unsafe code and I've been following the development in this repo.

Since this retains the full &mut self inside the returned ArenaMutChildren, I think the safety requirements are more extensive than the case described here. Any use of parent_arena in ArenaMutChildren needs to be careful to not invalidate the reference produced from unsafe { item.get().as_mut() } below. E.g. DataMap::find also must only be called on descendants, parent_arena.items.remove() must only operate on descendants, parent_arena.items.insert() must not overwrite existing nodes, parent_arena.items must be used to clear the whole structure, etc.

I was initially thinking the safety documentation could be reworked to note the overall requirement that the parent_arena field must not be used to invalidate the returned item reference and then include a few examples. However, I think all call sites would have the same safety note pointing to the details of ArenaMutChildren. So since all the details of ArenaMutChildren are private to this module, find_mut could be safe, and the unsafe block in it could be documented with the fact that all operations that ArenaMutChildren allows only access descendants and never invalidate the item reference created here.

Agree with the overall point that the safety requirement should be better put as that you can only access children or remove children of the current node (I'm not sure whether insert() comment would be needed as it is a panic to insert a node with the same name as the key, and not sure clearing is different to removal as there isn't a clear api, and I assume any would be an action on the Arena itself thus checked by the borrow checker as having a reference to an item in the tree prevents any mutable access of the tree itself)

Regarding safety, I think this makes sense, but can also see another argument that it being unsafe is correct in the case it did become public (though assume then the same should be true of the immutable method otherwise could obtain a immutable and mutable reference to the same node)

I'm not sure whether insert() comment would be needed as it is a panic to insert a node with the same name as the key,

there isn't a clear api, and I assume any would be an action on the Arena itself thus checked by the borrow checker as having a reference to an item in the tree prevents any mutable access of the tree itself

These are comments about what the safe code in ArenaMutChildren is allowed to do with the internal items hashmap (which has things such as insert without such checks and clear).

PoignardAzur

LGTM, pending changing the find methods and removing the children lists.

You can consider everything else optional before merging.

tree_arena/src/tree_arena_unsafe.rs

tree_arena/tests/basic_tests.rs

PoignardAzur · 2024-11-28T13:37:27Z

Another blocker before this is merged: I'd like for some version of my comment to show up in the documentation:

Having an unsafe implementation may slow us down if we want to try API changes and we have to make sure they're implemented in both versions.

I'd like to establish that the safe version will be a first-class citizen and the unsafe version a second-class citizen:
* The safe version may have features / APIs that the unsafe version doesn't yet have.

* If both versions are at feature parity, Masonry can switch on the unsafe version for best performance.

* Otherwise, Masonry uses the safe version.
If that's what we go with, we should documented that pattern in a few places.

KGrewal1 · 2024-11-28T14:22:55Z

Added to the readme and the doc comment at the root of the crate

PoignardAzur

LGTM.

I'd maybe like a comment noting that TreeNode::children needs to be removed but otherwise we're good.

KGrewal1 · 2024-11-29T11:25:16Z

I'm not sure TreeNode::children can be removed yet as it is being used on node removal (as we remove all descended nodes) - I think the alternative would be iterating over the whole hashmap to find which children have the current node as their parent but that would definitely be inefficient?

DJMcNab

I don't want to approve the unsafe code, as I don't fully understand it.

However, given that we use the entirely safe version by default, it seems low-risk enough to land this. Not approving because of some of the docs concerns

tree_arena/Cargo.toml

DJMcNab · 2024-12-03T13:38:06Z

tree_arena/src/lib.rs

+// Copyright 2024 the Xilem Authors
+// SPDX-License-Identifier: Apache-2.0
+
+//! This crate implements a tree data structure for use in Masonry


I'd like to use cargo-rmde here; it's already set up in CI.

That doesn't need to block this PR, but it would be a good follow-up

For clarity is that the content after the licence in the other lib.rs eg lines 4-22 in https://github.com/linebender/xilem/blob/main/xilem_core/src/lib.rs ?

Ah, right. This is confusing.

No, this is the setup used specifically in the Masonry crate. The version in Xilem Core is an old version of that

tree_arena/src/tree_arena_unsafe.rs

DJMcNab · 2024-12-03T13:56:23Z

tree_arena/src/tree_arena_unsafe.rs

+#[derive(Debug)]
+struct TreeNode<T> {
+    item: T,
+    children: Vec<NodeId>,


This is also a little bit surprising to me...

DJMcNab · 2024-12-03T14:22:37Z

tree_arena/src/tree_arena_unsafe.rs

+#[derive(Debug)]
+struct DataMap<T> {
+    /// The items in the tree
+    items: HashMap<NodeId, Box<UnsafeCell<TreeNode<T>>>>,


Would we be able to have something like an UnsafeMutex type, ideally which is implemented safely by default but with an unsafe passthrough version, to check some that invariants are being met.
That is, have this same implementation, but in a checked manner?

Yes but would require quite a few changes as the TreeNode contains the contents themselves and the list of children, but the API returns these in separate structs - the item and the ArenaChildren (mut or ref) so I think both would need to be wrapped in some form of mutex for run time checking separately ?

DJMcNab · 2024-12-05T10:18:49Z

I've done some spot checks of a few examples, and this all seems to work. As discussed on Zulip, we'll land this now, because the unsafe code is off by default.

Use `cargo rdme` for crate readme and check in CI as mentioned in #752 (comment)

KGrewal1 force-pushed the tree_arena branch 3 times, most recently from 582557d to 4f7a05f Compare November 25, 2024 11:56

KGrewal1 force-pushed the tree_arena branch from 4f7a05f to bbd9cd8 Compare November 26, 2024 10:54

PoignardAzur requested changes Nov 27, 2024

View reviewed changes

Imberflur reviewed Nov 28, 2024

View reviewed changes

PoignardAzur mentioned this pull request Nov 28, 2024

Support transforms for each widget #753

Draft

PoignardAzur reviewed Nov 28, 2024

View reviewed changes

KGrewal1 force-pushed the tree_arena branch from 00540f4 to cf910e9 Compare November 28, 2024 12:49

PoignardAzur approved these changes Nov 29, 2024

View reviewed changes

KGrewal1 force-pushed the tree_arena branch 4 times, most recently from 78ccfcd to 6f503e9 Compare December 3, 2024 12:24

DJMcNab reviewed Dec 3, 2024

View reviewed changes

KGrewal1 added 8 commits December 3, 2024 18:23

create arena tree

01430e6

improved short circuiting for checking descendants

393272d

update lib.rs

321df71

safe tree fixes

afe5060

initial unsafe edits

2a3fdde

fix import order

e4950e0

simplify roots

3ad1744

move architecture to readme

b58d28d

KGrewal1 added 8 commits December 3, 2024 18:23

improive mem_swap test

c97c5ed

fix lints

1a81308

breakdown option methods list and remove unneeded child array

1881aaa

safety comment edits

28f8b0e

find methods

e766db4

fix missed into

9cf5821

update docs

a72fc12

fix cargo.toml docs

af4b549

KGrewal1 force-pushed the tree_arena branch from e2077f1 to af4b549 Compare December 3, 2024 18:23

DJMcNab added this pull request to the merge queue Dec 5, 2024

Merged via the queue into linebender:main with commit a1d47a0 Dec 5, 2024
17 checks passed

KGrewal1 deleted the tree_arena branch December 5, 2024 10:53

KGrewal1 mentioned this pull request Dec 5, 2024

use cargo rdme for tree arena #769

Merged

github-merge-queue bot pushed a commit that referenced this pull request Dec 5, 2024

use cargo rdme for tree arena (#769)

3aebc98

Use `cargo rdme` for crate readme and check in CI as mentioned in #752 (comment)

KGrewal1 mentioned this pull request Dec 9, 2024

tree_arena: Use hashbrown::HashMap as drop-in replacement to optimize tree-access #774

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tree arena #752

Tree arena #752

KGrewal1 commented Nov 17, 2024

KGrewal1 commented Nov 17, 2024 •

edited

Loading

PoignardAzur commented Nov 18, 2024 •

edited

Loading

PoignardAzur commented Nov 18, 2024

PoignardAzur left a comment

PoignardAzur Nov 27, 2024

PoignardAzur Nov 27, 2024

DJMcNab Nov 27, 2024 •

edited

Loading

KGrewal1 Nov 27, 2024

PoignardAzur Nov 28, 2024

PoignardAzur Nov 27, 2024

KGrewal1 commented Nov 27, 2024

Imberflur Nov 28, 2024 •

edited

Loading

KGrewal1 Nov 28, 2024

KGrewal1 Nov 28, 2024

Imberflur Nov 28, 2024

PoignardAzur left a comment

PoignardAzur commented Nov 28, 2024

KGrewal1 commented Nov 28, 2024

PoignardAzur left a comment

KGrewal1 commented Nov 29, 2024 •

edited

Loading

DJMcNab left a comment •

edited

Loading

DJMcNab Dec 3, 2024

KGrewal1 Dec 3, 2024

DJMcNab Dec 3, 2024

DJMcNab Dec 3, 2024

DJMcNab Dec 3, 2024

KGrewal1 Dec 3, 2024

DJMcNab commented Dec 5, 2024

		It is possible to get shared (immutable) access or exclusive (mutable) access to the tree. These return `ArenaRef<'arena, T>` or `ArenaMut<'arena, T>` respectively

Tree arena #752

Tree arena #752

Conversation

KGrewal1 commented Nov 17, 2024

KGrewal1 commented Nov 17, 2024 • edited Loading

PoignardAzur commented Nov 18, 2024 • edited Loading

PoignardAzur commented Nov 18, 2024

PoignardAzur left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DJMcNab Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KGrewal1 commented Nov 27, 2024

Imberflur Nov 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PoignardAzur left a comment

Choose a reason for hiding this comment

PoignardAzur commented Nov 28, 2024

KGrewal1 commented Nov 28, 2024

PoignardAzur left a comment

Choose a reason for hiding this comment

KGrewal1 commented Nov 29, 2024 • edited Loading

DJMcNab left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DJMcNab commented Dec 5, 2024

KGrewal1 commented Nov 17, 2024 •

edited

Loading

PoignardAzur commented Nov 18, 2024 •

edited

Loading

DJMcNab Nov 27, 2024 •

edited

Loading

Imberflur Nov 28, 2024 •

edited

Loading

KGrewal1 commented Nov 29, 2024 •

edited

Loading

DJMcNab left a comment •

edited

Loading