Remove bounds and type checks from IngredientCache
#937
Conversation
✅ Deploy Preview for salsa-rs canceled.
|
CodSpeed Performance ReportMerging #937 will improve performances by 5.77%Comparing Summary
Benchmarks breakdown
|
|
Sounds reasonable to me
Would be interesting if we could create a weird test for this (though not necessary for this PR of course) |
| }) | ||
| // SAFETY: `lookup_jar_by_type` returns a valid ingredient index, and the only | ||
| // ingredient created by our jar is the struct ingredient. | ||
| unsafe { |
There was a problem hiding this comment.
I think it's technically possible that a salsa user implements their own Ingredient. Could a malicious Ingredient implementation invalidate the SAFETY assumption?
There was a problem hiding this comment.
No, this just accesses the ingredient created by this jar.
| /// Equivalent to the `downcast` methods on `any`. | ||
| /// Equivalent to the `downcast` methods on `Any`. | ||
| /// | ||
| /// Because we do not have dyn-upcasting support, we need this workaround. |
There was a problem hiding this comment.
We could consider bumping the MSRV.
There was a problem hiding this comment.
bumping msrv seems reasonable; rust-analyzer shouldn't have an issue with that (we're generally current-1).
There was a problem hiding this comment.
Upcasting was stabilized in 1.86, which is quite recent. FWIW this is an old comment, so I'm not sure it makes sense to do in this PR (but I'm not opposed to it).
There was a problem hiding this comment.
FWIW this is an old comment, so I'm not sure it makes sense to do in this PR (but I'm not opposed to it).
Agree, I think it's better to do in a separate PR. I only wanted to point out that bumping the MSRV is a possiblity
@Veykril might need to verify my logic here, but I think this is correct given either the
inventoryfeature or the nonce checks, asIngredientCachealways loads the index directly from the database.The safety arguments for unchecked access are less clear for other uses of
lookup_ingredient. The most notable one is inDatabaseKeyIndex::maybe_changed_after, where I think it's actually possible that a function records a dependency on an ingredient from a different database, in which case we should panic. It would be nice if we could guarantee that ingredient indices are stable even with manual registration, but that sort of defeats the purpose.