owned this note
owned this note
Published
Linked with GitHub
---
title: "Salsa Meeting 2025-01-22"
date: 2025-01-22
url: https://hackmd.io/rEk8dr95Q8C9eVz-uxlzMA
---
# Agenda
## rust-analyzer Integration Updates
We've opened PR! https://github.com/rust-lang/rust-analyzer/pull/18964. Few notes:
- Memory usage has gotten under control: it's now only a 250 megabyte regression.
- Chayim's [enum changes](https://github.com/ChayimFriedman2/salsa/tree/fix-enums-bugs) reduce memory usage by ~300 megabytes and implement [salsa#578](https://github.com/salsa-rs/salsa/issues/578). We need to port this over to use `macro_rules` + `quote` instead of just `quote`.
There's a few remaining questions:
### Globalizing Sync/Memo Tables
[Globalizing sync tables](https://github.com/salsa-rs/salsa/pull/650) reduces rust-analyzer's by 240 megabytes, but might harm parallelism (by how much? unclear). As an alternative, Chayim reduced memory usage by shrinking `SyncState` from 16 bytes to 2 bytes. *Question*: what was the intention behind per-ingredient sync/memo tables? simplify parallelism, or...?
**Conclusion:**
* Overall goal should be to merge sync table into memos
* But landing this PR would be ok as an intermediate step so long as it doesn't interfere with Carl's work
### Interned structs containing references to other structs
After talking to Jack Huey, he mentioned it'd make migrating rust-analyzer to the new trait solver easier if he could write:
```rust
#[salsa::interned]
struct Ty<'db> {
kind: &'db TyKind
}
```
To me (davidbarsky), this smells like a combination of https://github.com/salsa-rs/salsa/pull/633 and https://github.com/salsa-rs/salsa/issues/498. While tracked structs allow the following already:
```rust
#[salsa::tracked]
struct MyTracked1<'db> {
field: MyTracked2<'db>,
}
#[salsa::tracked]
struct MyTracked2<'db> {
field: u32,
}
```
However, this doesn't have value semantics/equality, and `Ty`/`TyKind` need to be interned.
**Conclusion:**
* Supporting *references* is hard but ought to be possible to include `'db` lifetime without a problem
## Address/Discuss Remaining Open PRs
- See discussion above on sync tables.
- TK
## Garbage Collection
- How should GC interact with interned values and non-default durabilities? [Github comment](https://github.com/salsa-rs/salsa/pull/602#issuecomment-2598225695)
## LRU eviction on new revision
The LRU evicting within query computations can result in frequent (re-)evictions. This can be noticable in rust-analyzer when we process multiple requests doing similar-ish work where they will potentially start trashing the macro parse caches of another. Which gave me the idea that we could have caches evict on a new revision instead (as well as allowing manual eviction by the database owner). In other words we could offer LRU flavors:
- No LRU
- Current/Automatic LRU (always evict immediately to stay at the limit)
- Manual LRU (new, trigger manually to have it evict until it reaches its limit again)
- Revision LRU (new, trigger evictions when a new revision starts)
**Conclusion:**
* Move LRU eviction to `&mut` since in reality we don't actually drop the memory until `&mut` anyway
* it will also align with interned data being released, in this way, and generally simplify
## open PRs
### interned-sans-lifetime
@davidbarsky has locally updated [salsa#632](https://github.com/salsa-rs/salsa/pull/632) to substantially reduce the surface area of the changes by making use `salsa::plumbing::macro_if`. Usage is now `#[salsa::interned(no_lifetime)]` instead of a standalone macro.
Niko suggested reducing the usage of `macro_if` through the following:
* `db_lt` = `'db` always
* `$($db_lt_arg:lifetime)?` may be empty
and then do
```rust
impl<$db_lt> $Struct< $($db_lt_arg)? > {
}
```
**Conclusion:**
- @davidbarsky will close [salsa#632](https://github.com/salsa-rs/salsa/pull/632) and open a new PR with the more minimal changes.
### untracked-by-default
* good to land
* one challenge: need an ingredient for every field
* something depends on the fields being in specific ordering
### interning / ibraheem's PR
* only interning adds edges
* we need to support interning outside a query (with immortal semantics) *internally*
* plan:
* when you intern a value, record the durability of the query thus far in the interned struct (taking the max durability if there are multiple)
* you record a dependency on the interned value with current durability of interned value
* when you read from interned value
* no edge is created
* when you recycle an interned value
* have to mark its durability as having changed
* how would we do GC
* what is the best heuristic?
* one cute one is the color scheme
* red/yellow/green with tunable sizes
* to promote from yellow to green:
* swap some green entry with your yellow one
* to promote red to green:
* swap some red with yellow
* then some yellow with green above
* then we can just recycle the red generation always, for example, the question will be how we tune the sizes of those bins, I think it'd be cool to observe and analyze them
* rust-analyzer has a fork/version of this algorithm: https://github.com/rust-lang/rust-analyzer/blob/1f23156f51e6a064becb1103eac078c2c9d14a80/crates/ra-salsa/src/lru.rs#L150
https://github.com/salsa-rs/salsa/blob/650545363ef1508be1b7e84a2d6db976c9eb4455/tests/interned-revisions.rs#L19
### fixpoint iteration
- Got the multiple-thread tests passing.
- When a query is about to return a provisional value, which is provisional on a cycle head that isn't on the active-query stack in that same thread, we have to insert our provisional result in the memo table (so it's available to the other thread) then block on the thread that owns execution of the cycle head, to let that other thread fixpoint-iterate and resolve the cycle. Then we can fetch the updated non-provisional memo for our own query, and return it.
- Need to resolve some code organization issues, add some more complex multi-threaded cases (nested cycles, with 3+ threads and each head owned by a different thread) to make sure they work too, and then work on performance.
- Right now we are making Memo quite a bit larger, making ClaimGuard more expensive, and there are some other potential optimization opportunities Lukas pointed out.
- I'll post in Zulip when I think I've reached functional correctness (hopefully today or tomorrow?); at that point review and help/suggestions/exploration on performance issues would be very welcome.
can we setup a testing framework
https://github.com/awslabs/shuttle
## rust-analyzer PR prioritization
### 1.0 alpha 1
* interned sans lifetime
* other memory use reduction?
* 'salsa supertype' ([#578](https://github.com/salsa-rs/salsa/issues/578))
* tracked by default
### 1.0 alpha 2
* fixed point iteration
* intern GC
*