Skip to content
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

perf: use dyn Database to reduce churn and binary size #8404

Closed
DaniPopes opened this issue Jul 10, 2024 · 3 comments · Fixed by #8924
Closed

perf: use dyn Database to reduce churn and binary size #8404

DaniPopes opened this issue Jul 10, 2024 · 3 comments · Fixed by #8924
Assignees
Labels
A-internals Area: internals T-feature Type: feature T-perf Type: performance

Comments

@DaniPopes
Copy link
Member

DaniPopes commented Jul 10, 2024

Component

Other (please describe)

Describe the feature you would like

Currently we pass around DB generic everywhere for very little gain. I believe if we used Box<dyn Database> or similar we could remove all of these generics, make Cheatcode object-safe unifying it with DynCheatcode, and generally deduplicate code.

Made possible by recent improvements by @klkvr

Additional context

No response

@DaniPopes DaniPopes added the T-feature Type: feature label Jul 10, 2024
@zerosnacks zerosnacks added A-internals Area: internals T-perf Type: performance labels Jul 10, 2024
@klkvr
Copy link
Member

klkvr commented Jul 10, 2024

Generics currently allow us to recurse inside of inspectors by using such references:

db: &mut ccx.ecx.db as &mut dyn DatabaseExt,

how would we handle this when ccx.ecx.db would be a concrete type?

I think this is the only blocker rn, and one of the biggest pain points which I've also encountered when looking into custom precompiles, because ContextPrecompiles<DB> couldn't be reused when creating those references

@DaniPopes
Copy link
Member Author

In that case ccx.ecx.db is &mut dyn Db or Box<dyn Db> which you can just re-borrow, unless I'm misunderstanding

@klkvr
Copy link
Member

klkvr commented Jul 10, 2024

right, I think It'd have to be &mut dyn Db then? I believe you can't obtain another Box<dyn Db> without moving out of the ccx.ecx.db

@zerosnacks zerosnacks changed the title Use dyn Database to reduce churn and binary size perf: use dyn Database to reduce churn and binary size Jul 16, 2024
@zerosnacks zerosnacks added this to the v1.0.0 milestone Jul 26, 2024
@jenpaff jenpaff removed this from the v1.0.0 milestone Sep 24, 2024
@klkvr klkvr added this to Foundry Oct 7, 2024
@github-project-automation github-project-automation bot moved this to Todo in Foundry Oct 7, 2024
@klkvr klkvr moved this from Todo to Done in Foundry Oct 7, 2024
@grandizzy grandizzy moved this from Done to Completed in Foundry Oct 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-internals Area: internals T-feature Type: feature T-perf Type: performance
Projects
Status: Completed
Development

Successfully merging a pull request may close this issue.

4 participants