Expose core gas utilities for external consumption #7851
Conversation
Will be used in `cairo-profiler`
|
This change is |
There was a problem hiding this comment.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion
a discussion (no related file):
what is the specific usage?
for calculating gas there's already existing functionalities.
There was a problem hiding this comment.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @orizi)
a discussion (no related file):
Previously, orizi wrote…
what is the specific usage?
for calculating gas there's already existing functionalities.
core_libfunc_cost is used there
There was a problem hiding this comment.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @ksew1)
a discussion (no related file):
Previously, ksew1 wrote…
core_libfunc_cost is used there
the result here would just be wrong using it.
this is the function for deprecated way of computing costs.
There was a problem hiding this comment.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @ksew1)
a discussion (no related file):
Previously, orizi wrote…
the result here would just be wrong using it.
this is the function for deprecated way of computing costs.
i was wrong - but - it seems that you would have some wrong data in any case.
the correct full way to calculate total gas usage is at:
crates/cairo-lang-sierra-to-casm/src/invocations/mod.rs:344.
which isn't fully equivalent.
There was a problem hiding this comment.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @orizi)
a discussion (no related file):
Previously, orizi wrote…
i was wrong - but - it seems that you would have some wrong data in any case.
the correct full way to calculate total gas usage is at:
crates/cairo-lang-sierra-to-casm/src/invocations/mod.rs:344.
which isn't fully equivalent.
At the moment, we only need to get builtin costs of libfuncs (when tracking sierra gas in snforge).
As we depend on snforge generated traces (which still uses cairo vm for execution), we are still kind of tracking steps and then convert to sierra gas (a refactor will be needed on snforge and profiler first to change it - which is planned, but no eta; then we will probably go the route you've suggested in profiler for getting the most accurate costs), so this is actually good enough at the moment for us.
This seems to be the easiest way for us to have sierra gas estimations correctly include builtin costs without said refactors.
There was a problem hiding this comment.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @ksew1 and @THenry14)
a discussion (no related file):
Previously, THenry14 (Wojciech Szymczyk) wrote…
At the moment, we only need to get builtin costs of libfuncs (when tracking sierra gas in snforge).
As we depend on snforge generated traces (which still uses cairo vm for execution), we are still kind of tracking steps and then convert to sierra gas (a refactor will be needed on snforge and profiler first to change it - which is planned, but no eta; then we will probably go the route you've suggested in profiler for getting the most accurate costs), so this is actually good enough at the moment for us.
This seems to be the easiest way for us to have sierra gas estimations correctly include builtin costs without said refactors.
if this is aimed more as a temporary hack - just have a local implementation that is similar to this, and you'd be able to locally delete (unlike adding pub to things that shouldn't really be pub)
There was a problem hiding this comment.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @orizi)
a discussion (no related file):
Previously, orizi wrote…
if this is aimed more as a temporary hack - just have a local implementation that is similar to this, and you'd be able to locally delete (unlike adding
pubto things that shouldn't really bepub)
I see your point, but I don't think it is that simple - core\_libfunc\_cost is quite large, and from what I can see it still is getting some modifications (I can see some commits from last month) - not only it would mean we basically need to copy over bigger part of cairo-lang-sierra-gas crate (at least), that would also mean we need to constantly track upstream changes to make sure we do not omit something new accidentally. Given that there is no eta on the refactors (I don't think it will happen any time soon for various reasons), and plans to put a bit less effort into profiler in the near future, I'm sure you see how this is somewhat suboptimal from our PoV.
Btw do you mind sharing what are the reasons why these should not be public? that would perhaps help me better understand the exact issue/pain point here
There was a problem hiding this comment.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @ksew1 and @THenry14)
a discussion (no related file):
Previously, THenry14 (Wojciech Szymczyk) wrote…
I see your point, but I don't think it is that simple -
core\_libfunc\_costis quite large, and from what I can see it still is getting some modifications (I can see some commits from last month) - not only it would mean we basically need to copy over bigger part of cairo-lang-sierra-gas crate (at least), that would also mean we need to constantly track upstream changes to make sure we do not omit something new accidentally. Given that there is no eta on the refactors (I don't think it will happen any time soon for various reasons), and plans to put a bit less effort into profiler in the near future, I'm sure you see how this is somewhat suboptimal from our PoV.Btw do you mind sharing what are the reasons why these should not be public? that would perhaps help me better understand the exact issue/pain point here
you don't need to duplicate the large logic at all - you just need to implement simple implementations for:
InvocationApChangeInfoProvider and InvocationCostInfoProvider.
this would allow you to get the basic prices - no need to duplicate the full logic.
Will be used in
cairo-profiler