improve deal accounting performance #309

whyrusleeping · 2020-04-21T18:03:16Z

This should accomplish the same thing, except now payments for storage deals only happen once every hundred blocks or so (not bucketed though, otherwise that would create hot spots)

zixuanzh · 2020-04-22T03:30:40Z

actors/builtin/market/market_actor.go

-	// TODO: award some small portion of slashed to caller as incentive
-
-	// Restore verified dataset allowance for verified clients.
-	for _, deal := range verifiedDeals {


we will need to put this logic in processDealInitTimedOut

actors/builtin/market/market_actor.go

anorth

Less code and better code 💯. This approach looks great, but some details are not quite right yet.

Some other notes:
We ended up with the cron actor's fixed-at-construction table not in this repo, but I guess replicated in each impl. The default parameters to the cron constructor probably belong here somewhere (and the market actor needs to be added).

Would a bitfield be strictly more efficient for storing a list of deal IDs, like in this ops queue, even though they are not expected to be as contiguous as sectors IDs? Even with large gaps, I'd expect the deltas to be less than the magnitudes.

anorth · 2020-04-22T03:20:41Z

actors/builtin/market/market_actor.go

@@ -82,7 +81,6 @@ func (a Actor) WithdrawBalance(rt Runtime, params *WithdrawBalanceParams) *adt.E
 	rt.State().Transaction(&st, func() interface{} {
 		// Before any operations that check the balance tables for funds, execute all deferred
 		// deal state updates.


Replace this comment with something like "The withdrawable amount might be slightly less than nominal depending on whether all relevant entries have been processed by cron".

anorth · 2020-04-22T03:22:28Z

actors/builtin/market/market_actor.go

@@ -195,7 +192,7 @@ func (a Actor) PublishStorageDeals(rt Runtime, params *PublishStorageDealsParams
 			rt.Abortf(exitcode.ErrIllegalState, "failed to load proposals array: %s", err)
 		}

-		dbp, err := AsSetMultimap(adt.AsStore(rt), st.DealIDsByParty)
+		dbp, err := AsSetMultimap(adt.AsStore(rt), st.DealOpsByEpoch)


Nit: rename dbp to dealOps or similar

anorth · 2020-04-22T03:26:18Z

actors/builtin/market/market_actor.go

-	builtin.RequireSuccess(rt, code, "failed to burn funds")
-	return nil
-}
+		for i := st.LastCron; i <= rt.CurrEpoch(); i++ {


Using the first key from the dbe AMT would avoid the need to store this, and be more robust in case there's an error somewhere and this gets out of sync, leaving trash in the AMT.

But its not an AMT, so thats really doable

Ack, ok.

Not necessarily in this PR, but I'm increasingly thinking we should use an AMT for things indexed by epoch, so that we can traverse in order. The same goes for the cron callback table in the power actor. I think there's decent scope for bugs leading to missed events and junk state left behind otherwise, especially given that cron doesn't necessarily execute on the epoch scheduled (due to null rounds).

In this case, with the values as integers, it could be an AMT of bitfields; the power actor needs to store more metadata tho.

anorth · 2020-04-22T03:27:29Z

actors/builtin/market/market_actor.go

-	Deals []abi.DealID // TODO: RLE
-}
+				if nextEpoch != 0 {
+					Assert(nextEpoch <= rt.CurrEpoch())


I think this should be nextEpoch > rt.CurrEpoch()

anorth · 2020-04-22T03:30:00Z

actors/builtin/market/market_actor.go


-	_, code := rt.Send(builtin.BurntFundsActorAddr, builtin.MethodSend, nil, slashedAmount)
-	builtin.RequireSuccess(rt, code, "failed to burn funds")
+	_, e := rt.Send(builtin.BurntFundsActorAddr, 0, nil, amountSlashed)


No reason I can see not to use builtin.MethodSend for clarity here

anorth · 2020-04-22T03:43:41Z

actors/builtin/market/market_state.go

@@ -511,7 +465,7 @@ func dealGetPaymentRemaining(deal *DealProposal, epoch abi.ChainEpoch) abi.Token
 }

 func (st *State) MutateDealIDs(rt Runtime, f func(dbp *SetMultimap) error) {


You deleted the only caller of this from deleteDeals, but we should be careful because that means obsolete deal ids can be left in the ops queue state here.

anorth · 2020-04-22T03:50:29Z

actors/builtin/market/set_multimap.go

@@ -38,9 +37,9 @@ func (mm *SetMultimap) Root() (cid.Cid, error) {
 	return mm.mp.Root()
 }

-func (mm *SetMultimap) Put(key address.Address, v abi.DealID) error {
+func (mm *SetMultimap) Put(epoch abi.ChainEpoch, v abi.DealID) error {


Aside: since this is now used for only one purpose, we could rename it to reflect that, as DealOpQueue or similar.

anorth · 2020-04-22T03:52:59Z

actors/builtin/market/market_state.go

 		}
-		return
+		return amountSlashed, 0


As I understand it, returning a zero epoch here means the deal will be forgotten about forever in terms of state updates. There's no longer a manual path by which a client or miner could trigger it. So returning zero should be the special, rare case though this method, only when a deal is deleted from state. All the other cases should return some future processing epoch.

I actually think the case youre commenting on will never be hit, it would mean that we called this method for a deal that hasnt been proven, before its starting deadline. Nothing registers a callback for deals at that point, so we really shouldnt ever hit that. I could call abort here, but i'm not 100% sure

anorth · 2020-04-22T03:53:27Z

actors/builtin/market/market_state.go

 	}

 	if deal.StartEpoch > epoch {
-		return
+		return amountSlashed, 0


Like here, I think the deal should be next processed at the start epoch.

Actually, i'd say theres nothing to do even at the start epoch, we probably want to schedule this to run again 100 blocks after the start epoch.

And I also think this case can never be hit, as we are explicitly scheduling the update to happen at StartEpoch, this shouldnt be able to happen.

actors/builtin/market/market_state.go

anorth · 2020-04-27T01:05:35Z

We ended up with the cron actor's fixed-at-construction table not in this repo, but I guess replicated in each impl. The default parameters to the cron constructor probably belong here somewhere (and the market actor needs to be added).

See #326

actors/builtin/market/market_actor.go

anorth

Thanks for this. Just minor comments.

actors/builtin/market/market_actor.go

actors/builtin/market/market_state.go

actors/builtin/market/market_test.go

anorth · 2020-05-05T03:05:29Z

Thanks. While approved, I request you don't land this for a couple of days so as not to introduce any uncertainty of un-exercised code into our current interop efforts. We may still need to push through some quick fixes that we want to consume quickly and minimally.

If you object to that, or if this lasts more than a few days, we can pick up the effort of maintaining a release branch for interop (which we will do later anyway).

magik6k

Last commit LGTM too

@anorth

* improve deal accounting performance * rerun cborgen * clean up method tables * address review feedback * Correctly handle deal state not found * a bit more cleanup * properly process timed out verified deal reimbursements * reuse balance table instances * finish addressing feedback from @anorth * add to cron tick * fix handling of deal state initialization

whyrusleeping requested review from anorth, magik6k and zixuanzh April 21, 2020 18:03

whyrusleeping force-pushed the feat/faster-deal-accounting branch from 40e0feb to 7062902 Compare April 21, 2020 19:52

zixuanzh suggested changes Apr 22, 2020

View reviewed changes

zixuanzh reviewed Apr 22, 2020

View reviewed changes

actors/builtin/market/market_actor.go Show resolved Hide resolved

anorth reviewed Apr 22, 2020

View reviewed changes

whyrusleeping requested review from anorth and zixuanzh April 29, 2020 19:49

zixuanzh approved these changes Apr 30, 2020

View reviewed changes

actors/builtin/market/market_actor.go Show resolved Hide resolved

actors/builtin/market/market_actor.go Show resolved Hide resolved

anorth approved these changes Apr 30, 2020

View reviewed changes

whyrusleeping mentioned this pull request May 4, 2020

Make verified registrys RestoreBytes able to do batch operations #341

Open

whyrusleeping force-pushed the feat/faster-deal-accounting branch from 0d28bae to 8abe31a Compare May 4, 2020 20:31

magik6k approved these changes May 5, 2020

View reviewed changes

whyrusleeping added 11 commits May 5, 2020 15:31

improve deal accounting performance

5e88235

rerun cborgen

48c210d

clean up method tables

267567e

address review feedback

3cbae9a

Correctly handle deal state not found

45ddc63

a bit more cleanup

1f5665b

properly process timed out verified deal reimbursements

1e9cd44

reuse balance table instances

a92c6b6

finish addressing feedback from @anorth

6d4cdec

add to cron tick

a283152

fix handling of deal state initialization

dc3d6cc

whyrusleeping force-pushed the feat/faster-deal-accounting branch from ccc878f to dc3d6cc Compare May 5, 2020 23:06

magik6k approved these changes May 5, 2020

View reviewed changes

anorth approved these changes May 6, 2020

View reviewed changes

whyrusleeping merged commit d5e9b63 into master May 6, 2020

whyrusleeping deleted the feat/faster-deal-accounting branch May 6, 2020 00:02

anorth mentioned this pull request Jun 10, 2020

Enforced static analysis of probable bugs (e.g. ignored errors) #456

Closed

whyrusleeping mentioned this pull request Jun 17, 2020

Publish successive deals fails (possible incorrect assertion in SMA market_state) #265

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improve deal accounting performance #309

improve deal accounting performance #309

whyrusleeping commented Apr 21, 2020

zixuanzh Apr 22, 2020

anorth left a comment

anorth Apr 22, 2020

anorth Apr 22, 2020

anorth Apr 22, 2020

whyrusleeping Apr 24, 2020

anorth Apr 25, 2020

anorth Apr 22, 2020

anorth Apr 22, 2020

anorth Apr 22, 2020

anorth Apr 22, 2020

anorth Apr 22, 2020

whyrusleeping Apr 24, 2020

anorth Apr 22, 2020

whyrusleeping Apr 24, 2020

whyrusleeping Apr 24, 2020

anorth commented Apr 27, 2020

anorth left a comment

anorth commented May 5, 2020

magik6k left a comment

		@@ -511,7 +465,7 @@ func dealGetPaymentRemaining(deal *DealProposal, epoch abi.ChainEpoch) abi.Token
		}

		func (st State) MutateDealIDs(rt Runtime, f func(dbp SetMultimap) error) {

improve deal accounting performance #309

improve deal accounting performance #309

Conversation

whyrusleeping commented Apr 21, 2020

Choose a reason for hiding this comment

anorth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anorth commented Apr 27, 2020

anorth left a comment

Choose a reason for hiding this comment

anorth commented May 5, 2020

magik6k left a comment

Choose a reason for hiding this comment