feat(lens): Optimize StateGetActor calls. #214
Conversation
Add a utility function called "OptimizedStateGetActorWithFallback()" which gets the current tipset's state by trying to load the child's state and using ParentState() on that, so that we avoid recomputing the state for the current tipset by applying messages. If anything goes wrong, the function falls back to the original, more expensive StateGetActor, and logs a warning.
There was a problem hiding this comment.
LGTM. I'd recommend giving this a read for more context on the nuance of the lotus API methods: https://protocollabs.slack.com/archives/CKT1X7YFN/p1603386966321800
lens/util.go
Outdated
| return nil, xerrors.Errorf("Failed to load tipset: %w", err) | ||
| } | ||
|
|
||
| child, err := api.ChainGetTipSetByHeight(ctx, ts.Height()+1, types.NewTipSetKey()) |
There was a problem hiding this comment.
why pass types.NewTipSetKey() here? Is this intended to be types.EmptyTSK?
|
@hsanjuan I have cherry-picked and tweaked these changes to handle null rounds in #238 -- see defb12a#diff-a771f3c826cfe5ebe05c1a003873af47456ee3e16188c1e5d0a1162c0b3e27acR41. In the same PR I have fixed what I believe to be an off by one error (see #238 (comment)) I discovered while reviewing this. Without the off-by-one fix many of the calls to StateGetActor will continue to fail which is why I (sloppy) did this all in a single PR. Feel free to merge the changes in defb12a here and I will land the bug fix as a follow on, or just land #238 directly in your AM. |
Cherry pick from #238.
|
@frrist I have imported your changes and added some smaller fixes. |
|
My approval is sticky, this LGTM. |
|
Hey @hsanjuan is there anything preventing you from merging this? |
|
I don't remember. Either I forgot or there were discussions about not calling these methods at all and this felt like more crust. Should we merge (I can rebase), or close? |
|
|
hsanjuan
force-pushed
the
fix/196-state-get-actor-perf
branch
from
586c06a to
f44e816
Compare
hsanjuan
force-pushed
the
fix/196-state-get-actor-perf
branch
from
f44e816 to
1b286c1
Compare
There was a problem hiding this comment.
@frrist ok, this should be good to go.
Please review again. The problem was that there was a TODO because I did not manage to enable the optimization for the lotus-api lens as I did not find how to access a store. I somehow managed now.
| // pre-computed ParentState(). | ||
| // | ||
| // TODO: Remove. See: https://github.com/filecoin-project/sentinel-visor/issues/196 | ||
| func OptimizedStateGetActorWithFallback(ctx context.Context, store cbor.IpldStore, chainAPI full.ChainModuleAPI, fallback full.StateModuleAPI, actor address.Address, tsk types.TipSetKey) (*types.Actor, error) { |
There was a problem hiding this comment.
This could be simpler if FullNodeAPI actually implemented FullNode but it doesn't:
lens/util/repo.go:109:74: cannot use ra.FullNodeAPI (type impl.FullNodeAPI) as type api.FullNode in argument to lens.OptimizedStateGetActorWithFallback:
impl.FullNodeAPI does not implement api.FullNode (AuthNew method has pointer receiver)
Add a utility function called "OptimizedStateGetActorWithFallback()" which
gets the current tipset's state by trying to load the child's state and using
ParentState() on that, so that we avoid recomputing the state for the current
tipset by applying messages.
If anything goes wrong, the function falls back to the original, more
expensive StateGetActor, and logs a warning.