Improve events heap allocations #27
Conversation
| @@ -178,7 +191,7 @@ func (a Attribute) String() string { | |||
|
|
|||
| // ToKVPair converts an Attribute object into a Tendermint key/value pair. | |||
| func (a Attribute) ToKVPair() abci.EventAttribute { | |||
| return abci.EventAttribute{Key: toBytes(a.Key), Value: toBytes(a.Value)} | |||
| return abci.EventAttribute{Key: []byte(a.Key), Value: []byte(a.Value)} | |||
There was a problem hiding this comment.
This had a signficant impact on performance!
However its still silly that we have to keep re-allocating heap space for the keys, when ~all the time, they are actually constants.
I think we should change the data-backend here to be using []byte, at least until abci events use strings.
| sdk.NewAttribute(types.AttributeKeySender, fromAddr.String()), | ||
| sdk.NewAttribute(sdk.AttributeKeyAmount, amt.String()), | ||
| )) | ||
| ctx.EventManager().EmitEvent(sdk.NewEvent( |
There was a problem hiding this comment.
Kind of annoying, but this also actually improved performance of the entire benchmark by a couple percent, and moreso the event sub-set of it. It avoids extra heap allocations on the argument, and then avoids a slice copy, instead you just append a stack element.
There was a problem hiding this comment.
Hmm is the extra EventTypeMessage actually used anywhere? I assume that's a general event emitted on basically all transactions that have a sender. It definitely seems like it makes more sense to just have the more specific EventTypeTransfer here, but are we sure no one is indexing / searching by the more general type?
There was a problem hiding this comment.
Oh wait, you're not actually getting rid of it lol, just separating it into two EmitEvents. The diff got cut off lol.
There was a problem hiding this comment.
ah woops, I should've copied more of the diff in the comment
| @@ -210,15 +223,9 @@ func (e Events) ToABCIEvents() []abci.Event { | |||
| return res | |||
| } | |||
|
|
|||
| func toBytes(i interface{}) []byte { | |||
| switch x := i.(type) { | |||
| case []uint8: | |||
There was a problem hiding this comment.
I assume this case is never actually used?
There was a problem hiding this comment.
Yeah, and it was bad to be casting to interface and then reflecting.
| sdk.NewAttribute(types.AttributeKeySender, fromAddr.String()), | ||
| sdk.NewAttribute(sdk.AttributeKeyAmount, amt.String()), | ||
| )) | ||
| ctx.EventManager().EmitEvent(sdk.NewEvent( |
There was a problem hiding this comment.
Hmm is the extra EventTypeMessage actually used anywhere? I assume that's a general event emitted on basically all transactions that have a sender. It definitely seems like it makes more sense to just have the more specific EventTypeTransfer here, but are we sure no one is indexing / searching by the more general type?
| sdk.NewAttribute(types.AttributeKeySender, fromAddr.String()), | ||
| sdk.NewAttribute(sdk.AttributeKeyAmount, amt.String()), | ||
| )) | ||
| ctx.EventManager().EmitEvent(sdk.NewEvent( |
There was a problem hiding this comment.
Oh wait, you're not actually getting rid of it lol, just separating it into two EmitEvents. The diff got cut off lol.
* Significantly lower number of heap allocations that should be required by events * Use EmitEvent instead of EmitEvents
* Significantly lower number of heap allocations that should be required by events * Use EmitEvent instead of EmitEvents
* Significantly lower number of heap allocations that should be required by events * Use EmitEvent instead of EmitEvents
* Significantly lower number of heap allocations that should be required by events * Use EmitEvent instead of EmitEvents
* Significantly lower number of heap allocations that should be required by events * Use EmitEvent instead of EmitEvents
Description
closes: #XXXX
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/) or specification (x/<module>/spec/)godoccomments.Unreleasedsection inCHANGELOG.mdFiles changedin the Github PR explorerCodecov Reportin the comment section below once CI passes