New database design #538
New database design #538
Conversation
✅ Deploy Preview for salsa-rs canceled.
|
nikomatsakis
force-pushed
the
spindle2
branch
6 times, most recently
from
f28dbb0
to
5fd60e4
Compare
We only ever need to upcast to shared references. This change isn't necessary, just dead code cleanup.
Also, move to a blanket impl'd trait. Overall cleaner approach.
The traits are now quite simple: * Database is the external trait * ZalsaDatabase is the internal one, implemented by `#[salsa::db]`. It adds two methods, `zalsa` and `zalsa_mut`. Those give access to our internal methods. For now I've hidden the methods behind `&dyn Zalsa`. This is nice and clean but it may be worth later refactoring to a `struct Zalsa`.
The distinction is dumb and should go away. But we still need it for a bit.
Creating events if nobody is listening has always bugged me.
Under this design, *all* databases are a `DatabaseImpl<U>`, where the `U` implements `UserData` (you can use `()` if there is none). Code would default to `&dyn salsa::Database` but if you want to give access to the userdata, you can define a custom database trait `MyDatabase: salsa::Databse` so long as you * annotate `MyDatabase` trait definition of impls of `MyDatabase` with `#[salsa::db]` * implement `MyDatabase` for `DatabaseImpl<U>` where `U` is your userdata (this could be a blanket impl, if you don't know the precise userdata type). The `tests/common/mod.rs` shows the pattern.
Separate handles are no longer needed.
Each clone gets an independent local state.
CodSpeed Performance ReportMerging #538 will not alter performanceComparing Summary
|
|
@MichaReiser I've been AFK for a bit...hopefully will be getting more reliable internet access soon. I've been thinking it over and I think I can produce a variation of this branch where we have the old setup (i.e., you define a struct that contains the Storage), which should address the various concerns you've raised. |
|
No worries. I hope you're having a great time. I don't have a strong preference over either approach. I think both is fine. The only real blocking item right now is that I don't know how to implement |
|
@MichaReiser I retooled it now, take a look. I think you convinced me that the old approach was better. We now have:
|
This should really be synchronized with the codespaces and github configuration but... I'm not clever enough to do all that.
and add more comments
|
I went ahead and pushed some other commits I had that make things miri-safe again (at least I think they do, let's see what CI has to say about it). |
Co-authored-by: Micha Reiser <[email protected]>
Prototype new database design. See the final commit for a write-up. This is ready to land but I was planning to do a bit more work, in particular removing the use of thread-local state.
Update: I've removed the thread-local state now.
KEY CHANGES
Handle-- just clone the database; when you try to make mutation, it will cancel other clonessalsa::DatabaseImplas a default database if you don't need customizationssalsa::dbis unchanged, you can still define your own database traits and types