search

pchampin / sophia_rs

Sophia: a Rust toolkit for RDF and Linked Data
Other
210 stars 23 forks source link

`RcTerm::Triple` and `ArcTerm::Triple` do not live up to the "cheap clone" promise #160

Closed pchampin closed 4 months ago

pchampin commented 5 months ago

One reason for using RcTerm or ArcTerm is that they are cheaper to clone than SimpleTerms (SimpleTerm uses MownStr internally, which may require a new allocation on cloning -- when they own their data).

However, in their current implementation, the Triple variant of RcTerm and ArcTerm are still expensive to clone, as they contain a Box (which also needs a new allocation when cloned). Instead, RcTerm::Triple should contain a Rc<[RcTerm; 3]> and ArcTerm::Triple should contain a Arc<[ArcTerm; 3]>.

This should probably be achieved by adding a second generic parameter to GenericTerm, for the content of the Triple variant.

pchampin commented 5 months ago

This should probably be achieved by adding a second generic parameter to GenericTerm, for the content of the Triple variant.

In fact, using Self in generic types seems to be forbidden. An alternative option is to turn GenericTerm into a macro generating the implementation of ArcTerm and RcTerm...

damooo commented 5 months ago

As a side-note, more than ArcTerm, it would be great to have an EcoTerm using EcoString . EcoString has phenomenal properties. It is cheap to clone. And can also be mutable (clone-on-write), and can have small strings (upto 15 bytes) inlined without allocation.

It also has correspondent EcoVec, with same properties. The crate is from typst team, and also recommonded by rust-analyzer author. Works great in Manas too.

pchampin commented 4 months ago

@damoo EcoTerm would indeed be nice. I would rather keep ArcTerm and RcTerm as they don't require additional dependencies, but adding EcoTerm in sophia_term, behind a feature gate, would be a very nice addition...

pchampin commented 4 months ago

adding EcoTerm in sophia_term, behind a feature gate, would be a very nice addition...

That would still be a separate issue, though.