Do manual trait casting #922
Conversation
✅ Deploy Preview for salsa-rs canceled.
|
CodSpeed Performance ReportMerging #922 will improve performances by 5.84%Comparing Summary
Benchmarks breakdown
|
Veykril
force-pushed
the
veykril/push-tsxunswownmm
branch
4 times, most recently
from
2073e08 to
28acf8c
Compare
|
Benchmarks look fairly noisy again to me |
|
Not sure, 5% is a bit more than usual. |
|
Well the increase seems to stem from ingredient fetching which means its likely the |
Veykril
force-pushed
the
veykril/push-tsxunswownmm
branch
2 times, most recently
from
0b13ad9 to
e50ed3a
Compare
|
Okay the regression came from a dumb mistake |
Veykril
changed the title
Veykril
force-pushed
the
veykril/push-tsxunswownmm
branch
from
e50ed3a to
523d4c8
Compare
|
Some context for reviewing aid, there are couple of noisy changes (that I could've split out into separate commits, sorry):
|
| } | ||
| } | ||
|
|
||
| /// Upcast to a `dyn Database`. | ||
| /// | ||
| /// Only required because upcasts not yet stabilized (*grr*). | ||
| /// Only required because upcasting does not work for unsized generic parameters. |
There was a problem hiding this comment.
I am quite annoyed by this but that's a limitation of Rust, a &T of T: ?Sized + SomeTrait cannot be coerced to a &dyn SomeTrait (or a supertrait) due to T possibly being an unrelated unsized type like str
There was a problem hiding this comment.
Can we add the Sized requirement?
There was a problem hiding this comment.
No, specifically this is used solely for attach which needs to take a dyn database of some form.
Veykril
force-pushed
the
veykril/push-tsxunswownmm
branch
from
f553491 to
70bdaef
Compare
There was a problem hiding this comment.
The changes to avoid dynamic dispatch make sense to me. I'm not sure about renaming the operation to upcasting. Upcasting would be going from a &dyn DatabaseView to &dyn Database, where Database is a supertrait of DatabaseView. What we're doing is going from a &dyn Database to a &dyn DatabaseView -- the other way around. It's similar to downcasting Any to a concrete type, but instead we're downcasting to a "more concrete" subtrait.
| } | ||
| } | ||
|
|
||
| /// Upcast to a `dyn Database`. | ||
| /// | ||
| /// Only required because upcasts not yet stabilized (*grr*). | ||
| /// Only required because upcasting does not work for unsized generic parameters. |
There was a problem hiding this comment.
Can we add the Sized requirement?
Hmm fair, I think i focused too much on the second step of the casting procedure, going from concrete back to dyn Trait. |
Veykril
force-pushed
the
veykril/push-tsxunswownmm
branch
from
471f240 to
0b385e7
Compare
Veykril
force-pushed
the
veykril/push-tsxunswownmm
branch
from
0b385e7 to
8c0ec58
Compare
This removes occurences of
dyn Databaseinternally where only the zalsas are needed for (hopefully) a perf increase.Lastly it renames downcasting to upcasting, because thats what we are doing,
dyn Database--downcast->Concrete--upcast-->dyn CustomDatabaseTrait