Support a new Durability::NEVER_CHANGE
#1035
Conversation
This is a very dangerous option, but it is needed for rust-analyzer.
✅ Deploy Preview for salsa-rs canceled.
|
There was a problem hiding this comment.
I'd suggest to first add an Immortal durability (or NEVER_CHANGE) and simply default all the unnecessary fields and, in a separate PR, explore whether it's worth having a separate Memo for those types.
Not only does this allow us to evaluate the benefit of the added complexity, there are also cases where we can't use the never change memo as you already noted:
- Queries participating in a cycle. We have to store cycle metadata. It's very likely that it will become necessary to store additional cycle metadata even for finalized memos to fix an unsoundness in
maybe_changed_after - Queries with accumulated values
There might be more cases.
Also note that there's #1022
|
For discussion completeness sake, here is a minor rust zulip discussion: #t-compiler/rust-analyzer > Never-change durability - yes or no? |
Inputs with this durability are assumed to never change. This allows us to optimize their storage a lot and not store any data about them except for the value. That makes them very compact. This is done by bit-tagging the memo and using it as one of two types. The overhead of handling the possibility of a second type will be seen in synthetic benchmarks; in practice, if using this durability (for example, for library code), speed will increase, not decrease (in addition to improved memory usage), because we don't need to validate query dependencies if they are assumed to never change. Values participating in cycles cannot have `Durability::NEVER_CHANGE`: while it is possible to support this (and in fact, an early revision of the code did) it introduces a lot of complications to the code, as we need to replace the type of a memo after it has been inserted.
ChayimFriedman2
force-pushed
the
never-change-durability-v3
branch
from
b94beb4 to
4c10d32
Compare
|
I feel stupid for not checking that :) On rust-analyzer, running on rust-analyzer the full version saves 33mb more, and on a bigger codebase (omicron) it saves 100mb more. It's less than #1031, which we rejected, so by logic we should reject this as well. OTOH I don't know if the reject for #1031 was justified :P |
I'm still open to the idea. I just find it easier to judge the complexity by having separate PRs and it avoids muddying the decision whether we want this new durability with whether the complexity for custom Memo is worth it (which is very cool and something similar might even be potentially interesting for cycle handling) |
3 participants
Inputs with this durability are assumed to never change. This allows us to optimize their storage a lot and not store any data about them except for the value. That makes them very compact.
This is done by bit-tagging the memo and using it as one of two types.
The overhead of handling the possibility of a second type will be seen in synthetic benchmarks; in practice, if using this durability (for example, for library code), speed will increase, not decrease (in addition to improved memory usage), because we don't need to validate query dependencies if they are assumed to never change.
Values participating in cycles cannot have
Durability::NEVER_CHANGE: while it is possible to support this (and in fact, an early revision of the code did) it introduces a lot of complications to the code, as we need to replace the type of a memo after it has been inserted.The two commits are completely separated and can be sent as different PRs if wanted.
The code is correct and ready to review, but I still need to review the failing tests: many now use this durability unintentionally because they do not read inputs, causing them to fail.