Speedup ChainAnteHandlers by making it non-recursive

[ValarDragon]

Summary

ChainAnteHandlers surprisingly is a measurable quantity of delay in some of our slow tests in Osmosis. Heres a snippet of it from our IBC tests:

I suggest we make this method non-recursive, to mitigate this. (This is a function created in run-time, so many compiler optimizations don't get applied) I'm in the process of eliminating more of the other overhead. (Raw-time is ~5x'd in our CI)

Here's ChatGPT's code suggestion:

func ChainAnteDecorators(chain ...AnteDecorator) AnteHandler {
	if len(chain) == 0 {
		return nil
	}

	// handle non-terminated decorators chain
	if (chain[len(chain)-1] != Terminator{}) {
		chain = append(chain, Terminator{})
	}

	return func(ctx Context, tx Tx, simulate bool) (Context, error) {
		// initialize current decorator and result with the first decorator in the chain
		currentDecorator := chain[0]
		ctx, err := currentDecorator.AnteHandle(ctx, tx, simulate, nil)
                if err != nil {
                         return ctx, err
                }

		// iterate over the remaining decorators in the chain
		for _, decorator := range chain[1:] {
			// update the current decorator and result with the next decorator in the chain
			currentDecorator = decorator
			ctx, err = currentDecorator.AnteHandle(ctx, tx, simulate, result)
                        if err != nil {
                              return ctx, err
                        }
		}

		return result
	}
}

[tac0turtle]

Love the usage of gpt3 here, do you want to submit a pr with the suggested code? Quickly looking at the code it looks like it wont compile.

[alexanderbez]

Generally I prefer iteration over tail recursion in almost all instances. So I'm in favor of this.

[tac0turtle]

@alexanderbez want to do some quick bench marks or profiling to see if there is a diff?

[elias-orijtech]

Putting aside the performance issue, I don't see how ChainAnteDecorators can ever be made non-recursive while preserving the semantics of AnteDecorator. Here's the definition of AnteDecorator:

type AnteDecorator interface {
	AnteHandle(ctx Context, tx Tx, simulate bool, next AnteHandler) (newCtx Context, err error)
}

which is passed a Context, a Tx and the next AnteHandler. The problem is that the decorator have to be able to call next before returning, forcing the chain to be recursive.

To illustrate the problem, consider this imlementation:

type DifficultDecorator struct{}

func (*DifficultDecorator) AnteHandle(ctx Context, tx Tx, simulate bool, next AnteHandler) (Context, error) {
    nextCtx, nextErr := next(ctx, tx, simulate)
    // do something to the nextCtx before returning it.
    return nextCtx.WithBlockGasMeter(...), nextErr
}

Now, you could decide that DifficultDecorator is an invalidid implementation and that only tail-recursion is allowed, where the result from next is not manipulated:

func (*OKDecorator) AnteHandle(ctx Context, tx Tx, simulate bool, next AnteHandler) (Context, error) {
    // do something to the nextCtx before passing it on.
    ctx := ctx.WithBlockGasMeter(...)
    // tail recursion is ok and can be made serial in ChainAnteDecorators.
    return next(ctx, tx, simulate)
}

but that's a subtle restriction, and then why have AnteDecorator at all?

I suggest replacing ChainAnteDecorators with a clear and more straightforward ChainAnteHandlers:

// ChainAnteHandlers calls each handler in chain, passing the result of
// each call to the next handler. It returns the context from the last handler,
// or the original context if the chain is empty.
// Note that an IsZero Context result is treated as if the passed
// in context was returned.
func ChainAnteHandlers(chain ...AnteHandler) AnteHandler {
	return func(ctx Context, tx Tx, simulate bool) (Context, error) {
		for _, h := range chain {
			newCtx, err := h(ctx, tx, simulate)
			if err != nil {
				return ctx, err
			}
			if !newCtx.IsZero() {
				ctx = newCtx
			}
		}
		return ctx, nil
	}
}

The advantage of ChainAnteHandlers is that it doesn't need AnteDecorator interface, and that it makes the tail recursion explicit.

[alexanderbez]

Yes, exactly. The entire premise of how they run is based on this recursive context. Not sure how much merit there is in keeping this issue open, at least not without strong evidence through benchmarks.

[robert-zaremba]

What's wrong with recursion? Usually it's simpler and more manageable. Compiler should sort it out for an efficient code execution... as long as the recursion is correctly coded.

I don't think the performance issues is in recursion. It's rather related to logic or the way how we handle it.

[yihuang]

In one of the profiling I did, for relatively simple transactions, ante handler are heavier than msg execution itself, other than one obvious hotspot of signature verification, the next one is actually GetParams, it do json decoding internally and get called multiple times by different decorators, it's tempting to collapse all the decorators into a flat sequential code so we can share those duplicated calls, not sure how much performance can be gained from that.

[alexanderbez]

Params don't use JSON anymore just an FYI. They're proto just like everything else in state.

[ValarDragon]

I think #16076 solves the production speed here, going to close this, and can re-investigate if this appears further in benchmarks's.