search

filecoin-project / specs-actors

DEPRECATED Specification of builtin actors, in the form of executable code.
Other
86 stars 102 forks source link

Resolve publish storage deal addresses in place #1362

Closed austinabell closed 3 years ago

austinabell commented 3 years ago

So this issue is definitely optional and more for discussion, but the pattern tripped me up reading and I think it can be simplified. Resolved addresses are added to a hashmap:

https://github.com/filecoin-project/specs-actors/blob/86a310df1e63a4d635accbbff0cc32b82884273e/actors/builtin/market/market_actor.go#L215

Then a copy of the deal is modified in that loop before interacting, but I don't understand why you can't just modify the deal in the slice so you don't need to add to the map and lookup after. Lookup here:

https://github.com/filecoin-project/specs-actors/blob/86a310df1e63a4d635accbbff0cc32b82884273e/actors/builtin/market/market_actor.go#L258

Tested to be equivalent because we interop doing this: https://github.com/ChainSafe/forest/pull/948/files

I would get that maybe you don't want side effects, but this modified value is being put on chain, so it's almost doing the opposite of avoiding side effects.

ZenGround0 commented 3 years ago

This appears to be a case of go being go. range statements are nice but cause a copy to be created: https://play.golang.org/p/96XRE5gjIUw . It would likely be marginally faster to iterate deal indexes and directly change the deals in the slice but probably not noticeably. I'm going to close this down right now though might want to reopen when exploring market optimizations in the future.