Feature-gate threadsafe syntax trees
Make Send and Sync impls for SyntaxXY a feature and see which parts of the implementation can be simplified for the single threaded case.
From the top of my head, I would start with
- the ref count,
-
NodeData'schildren, together withSyntaxNode::{read, write}, -
NodeData'sdata.
#2 should happen first before we start working on this.
Would you be open to using archery for this? We can add it as the last generic type and default to ArcTK, and have it not be a breaking change.
https://lib.rs/rpds does this and it's neat
So, we can definitely use something like this if it proves to be a useful abstraction, I don't have a strong opinion on whether this is better or worse than switching between Rc and Arc based on a feature flag (I believe rustc does something like this to support work on parallelizing the compiler, basically you do type SomeRc = /* depends on feature flag */. I think most users need one or the other, not both - but I could be wrong there), other than that we already have quite a lot of generics on everything and so if your suggested route involves adding more of them I'd want to provide type aliases to reduce the amount of having to specify which version you want.
With that said, I don't think it's quite as easy to support this as dropping in an abstraction over the refcounting pointer type:
- They support
triomphe'sArc, which is good, but we also useThinArcto store the children in green nodes, which according to the README they don't currently support (this is minor since we mostly want to support this for syntax nodes, not green nodes). - We don't actually use any RC type for the syntax nodes currently, as they manage their own refcount as an
AtomicU32becauseSyntaxNodeis already a handle / pointer type to the underlyingNodeData. UsingRcorArcinstead would make everySyntaxNodebigger, which would be a big perf hit. - Probably the biggest win we could get from this idea would be if we can get rid of the locks currently required to access a node's children (this is the
child_locks: Vec<RwLock<()>>inNodeData, and in the same way but less impactful theRwLockfordata). Since these are locks, not smart pointers,archerywon't help with that.
IMO tree building and traversal performance (and maybe memory usage) are the main motivation for this change, since other than that removing Send / Sync makes our types strictly less useful than they are now. A partial solution for only the pointer types likely won't buy us much here, especially if it increases the API surface (by adding yet another generic).