Build Entity instances, not Term* instances. (#674)#778
Conversation
It's not great to keep lifting functionality higher and higher up the crate hierarchy, but we really do want to intern while we parse. Eventually, I expect that we will split the `edn` crate into `types` and `parsing`, and the `types` crate can depend on a more efficient interning dependency.
We haven't observed performance issues using `Arc` instead of `Rc`, and we want to be able to include things that are interned (including, soon, `TempId` instances) in errors coming out of the transactor. (And `Rc` isn't `Sync`, so it can't be included in errors directly.)
This pattern is generally how newtype wrappers (like `struct Foo(Bar)`) are implemented in Rust.
This is all part of moving the entity builder away from building term instances and toward building entity instances. One of the nice things that the existing term interface does is allow consumers to use lightweight reference counted tempid handles; I don't want to lose that, so we'll build it into the entity data structures directly.
ncalexan
requested review from
bgrins and
grigoryk
and removed request for
bgrins
grigoryk
left a comment
grigoryk
left a comment
There was a problem hiding this comment.
You say "we haven't observed" - which I assume to mean "at some point we've profiled, and didn't find the added cost of Arc to be an issue given our usage patterns"?
There is a cost, but I don't have a good enough sense of the flow of things to guess if/when it'll be a problem.
What about somehow shifting the burden to the somewhat rare (once things application code stabilizes in its ways) error cases, maybe? e.g. clone whatever's is in Rc's if things go wrong, maybe?
Correct. That happened in #659 and friends; see #659 (comment). This is more analogy than science in this instance.
Exactly.
I'm happy to do that at some point, and considered it for this -- cloning names out of |
|
|
||
| /// Entity and value places embed values, either directly (i.e., `ValuePlace::Atom`) or indirectly | ||
| /// (i.e., `EntityPlace::LookupRef`). In order to maintain the graph of `Into` and `From` | ||
| /// relations, we need to ensure that `{Value,Entity}Place` can't match as a potential value. This |
There was a problem hiding this comment.
On first read, the dual meaning of "value" in this comment makes things a little confusing - but perhaps that's just my unfamiliarity with the subject. Unsure what you'd call "embedded values" other than values though. A "concrete value"?
Stepping back from the graph of Intos, perhaps this can also be talked about in terms of being able to guarantee termination of value resolution? E.g. "{Entity,Value}Place need to resolve to a concrete value, which could happen either directly (i.e. ValuePlace::Atom, concrete value is embedded in the enum) or indirectly (i.e. EntityPlace::LookupRef, concrete value is a hop away). We need to ensure that the resolution is guaranteed to terminate, etc etc".
grigoryk
left a comment
grigoryk
left a comment
There was a problem hiding this comment.
I don't claim to understand everything that's going on, but generally I think this makes some sense to me. I thought the use of TransactableValueMarker was quite elegant.
Rubber stamp if you need this landed!
There are a few tricky details to call out here. The first is the
`TransactableValueMarker` trait. This is strictly a marker (like
`Sized`, for example) to give some control over what types can be used
as value types in `Entity` instances. This expression is needed due
to the network of `Into` and `From` relations between the parts of
valid `Entity` instances. This allows to drop the `IntoThing`
work-around trait and use the established patterns. (Observe that
`KnownEntid` makes this a little harder, due to the cross-crate
consistency restrictions.)
The second is that we can get rid `{add,retract}_kw`, since the
network of relations expresses the coercions directly.
The third is that this commit doesn't change the name `TermBuilder`,
even though it is now building `Entity` instances. This is because
there's _already_ an `EntityBuilder` which fixes the `EntityPlace`.
It's not clear whether the existing entity building interface should
be removed or whether both should be renamed. That can be follow-up.
The `core` create didn't exist when the `db` was started, but this type is clearly part of the public interface of Mentat.
…nctions. These are functions on `TermBuilder` itself to prevent mixing mutable and immutable references in the most natural style. That is, ``` builder.add(e, a, builder.lookup_ref(...)) ``` fails because `add` borrows `builder` mutably and `lookup_ref` borrows `builder` immutably. There's nothing here that requires a specific builder (since we're not interning lookup refs on the builder, like we are tempids) so we don't need an instance.
2 participants
I haven't tried to rename
EntityBuilder(or remove it), and I haven't tried to renameTermBuildertoEntityBuilder. Those can come later.This also doesn't intern
TempIdhandles at parse time; we can get to that.This addresses #674.