-
Notifications
You must be signed in to change notification settings - Fork 192
Do manual trait casting #922
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
✅ Deploy Preview for salsa-rs canceled.
|
CodSpeed Performance ReportMerging #922 will improve performances by 5.84%Comparing Summary
Benchmarks breakdown
|
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 |
0b13ad9 to
e50ed3a
Compare
|
Okay the regression came from a dumb mistake |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add the Sized requirement?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, specifically this is used solely for attach which needs to take a dyn database of some form.
f553491 to
70bdaef
Compare
ibraheemdev
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
471f240 to
0b385e7
Compare
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