Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improve events heap allocations #27

Merged
merged 2 commits into from
Sep 2, 2021
Merged

Improve events heap allocations #27

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5536a8a
Significantly lower number of heap allocations that should be require…
ValarDragon Sep 2, 2021
4826797
Use EmitEvent instead of EmitEvents
ValarDragon Sep 2, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 20 additions & 13 deletions types/events.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,18 +28,28 @@ func NewEventManager() *EventManager {
return &EventManager{EmptyEvents()}
}

// Increase Capacity increases the capacity of the EventManager,
// which is quite useful when many events are planned to be created.
func (em *EventManager) IncreaseCapacity(capacity int) {
events := em.events.getUnderlyingSlice()
if cap(events) < capacity {
em.events = make(Events, 0, capacity)
em.events = append(em.events, events...)
}
}

func (em *EventManager) Events() Events { return em.events }

// EmitEvent stores a single Event object.
// Deprecated: Use EmitTypedEvent
func (em *EventManager) EmitEvent(event Event) {
em.events = em.events.AppendEvent(event)
em.events = append(em.events, event)
}

// EmitEvents stores a series of Event objects.
// Deprecated: Use EmitTypedEvents
func (em *EventManager) EmitEvents(events Events) {
em.events = em.events.AppendEvents(events)
em.events = append(em.events, events...)
}

// ABCIEvents returns all stored Event objects as abci.Event objects.
Expand Down Expand Up @@ -153,7 +163,10 @@ type (
// NewEvent creates a new Event object with a given type and slice of one or more
// attributes.
func NewEvent(ty string, attrs ...Attribute) Event {
e := Event{Type: ty}
e := Event{
Type: ty,
Attributes: make([]abci.EventAttribute, 0, len(attrs)),
}

for _, attr := range attrs {
e.Attributes = append(e.Attributes, attr.ToKVPair())
Expand All @@ -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)}
Copy link
Member Author

@ValarDragon ValarDragon Sep 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

// AppendAttributes adds one or more attributes to an Event.
Expand Down Expand Up @@ -210,15 +223,9 @@ func (e Events) ToABCIEvents() []abci.Event {
return res
}

func toBytes(i interface{}) []byte {
switch x := i.(type) {
case []uint8:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this case is never actually used?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, and it was bad to be casting to interface and then reflecting.

return x
case string:
return []byte(x)
default:
panic(i)
}
// getUnderlyingSlice converts Events to its underlying []Event
func (e Events) getUnderlyingSlice() []Event {
return e
}

// Common event types and attribute keys
Expand Down
17 changes: 17 additions & 0 deletions types/events_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,23 @@ func (s *eventsTestSuite) TestEventManagerTypedEvents() {
s.Require().Equal(hasAnimal.Animal.String(), response.Animal.String())
}

func (s *eventsTestSuite) TestIncreaseCapcity() {
// Ensure that appends still work after increase capacity is called.
// This does not check that the underlying capacity was increased,
// however this has been ensured via benchmarks.
em := sdk.NewEventManager()
e1 := sdk.NewEvent("transfer", sdk.NewAttribute("sender", "foo"))
e2 := sdk.NewEvent("transfer", sdk.NewAttribute("sender", "bar"))
a := sdk.Events{e1}
b := sdk.Events{e2}
em.EmitEvents(a)
em.IncreaseCapacity(1024)
em.EmitEvents(b)
s.Require().Equal(em.Events(), sdk.Events{e1, e2})
s.Require().Equal(em.Events(), sdk.Events{e1}.AppendEvent(sdk.NewEvent("transfer", sdk.NewAttribute("sender", "bar"))))
s.Require().Equal(em.Events(), sdk.Events{e1}.AppendEvents(sdk.Events{e2}))
}

func (s *eventsTestSuite) TestStringifyEvents() {
e := sdk.Events{
sdk.NewEvent("message", sdk.NewAttribute("sender", "foo")),
Expand Down
22 changes: 10 additions & 12 deletions x/bank/keeper/send.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,18 +138,16 @@ func (k BaseSendKeeper) InputOutputCoins(ctx sdk.Context, inputs []types.Input,
// SendCoins transfers amt coins from a sending account to a receiving account.
// An error is returned upon failure.
func (k BaseSendKeeper) SendCoins(ctx sdk.Context, fromAddr sdk.AccAddress, toAddr sdk.AccAddress, amt sdk.Coins) error {
ctx.EventManager().EmitEvents(sdk.Events{
sdk.NewEvent(
types.EventTypeTransfer,
sdk.NewAttribute(types.AttributeKeyRecipient, toAddr.String()),
sdk.NewAttribute(types.AttributeKeySender, fromAddr.String()),
sdk.NewAttribute(sdk.AttributeKeyAmount, amt.String()),
),
sdk.NewEvent(
sdk.EventTypeMessage,
sdk.NewAttribute(types.AttributeKeySender, fromAddr.String()),
),
})
ctx.EventManager().EmitEvent(sdk.NewEvent(
types.EventTypeTransfer,
sdk.NewAttribute(types.AttributeKeyRecipient, toAddr.String()),
sdk.NewAttribute(types.AttributeKeySender, fromAddr.String()),
sdk.NewAttribute(sdk.AttributeKeyAmount, amt.String()),
))
ctx.EventManager().EmitEvent(sdk.NewEvent(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh wait, you're not actually getting rid of it lol, just separating it into two EmitEvents. The diff got cut off lol.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah woops, I should've copied more of the diff in the comment

sdk.EventTypeMessage,
sdk.NewAttribute(types.AttributeKeySender, fromAddr.String()),
))

err := k.SubtractCoins(ctx, fromAddr, amt)
if err != nil {
Expand Down