Remove defers from getTypeInfo functions #227
Conversation
These defers were taking up 1% of the gaia state machines time, due to their usage within MarshalJSON and UnmarshalBinaryBare. There is no safety concern for removing these defers, as none of the methods within the functions should panic.
There was a problem hiding this comment.
You are right that these two defers aren't absolutely necessary. As a note and in general, I believe that we shouldn't do this kind of optimizations. I would assume that very few go-lang versions later this might be optimized at the go runtime level. And even if now this looks like an innocent change because no panic occurs on the way, it's just too easy to forget an explicit Unlock in one of the exec. paths in the future.
I'm still OK with merging this. The performance gain seems worth it (I see the time consumed dropped from 1% total time to 0.5%? Do you have any absolute numbers maybe?)
I think we should add a comment, where the defers were, though. As a "warning" or to be cautious here. And that this is a optimzation that might be unnecessary in the future.
E.g.
// We do not use defer cdc.mtx.Unlock() here as explicitly unlocking the mtx on each return is currently faster (tested with go1.10.?? and go1.11.?). Please, be careful if you change anything below: make sure that the changes won't panic.
After that, we are good to merge.
Referencing related discussions in golang: golang/go#14939
golang/go#6980
|
I agree with adding comments here. I do disagree with your point about generally doing this sort of optimization. It is a significant improvement now. Amino is already slow, and if we want our libraries to gain adoption they should be fast. I do hope defer speed gets fixed in a new version soon! I can benchmark these functions here to get numbers. Its a bit hard to read these numbers off the overall SDK benchmarks, are is there some fluctuation in the randomized simulations and we can't run long enough to get reliable benchmarks as we fail due to SDK bugs (lol). Hence the discrepancy between the original post and updated, I wanted to have a less alarmist number due to the randomized process returning .5% in one case. |
|
In general, I feel there are other bottlenecks here (and in in the software using amino) where we could better spent our time on. But OK, let's add comments here and merge this. Don't spend additional time getting benchmark numbers on this. Thanks a lot @ValarDragon! |
|
I think we should only remove defers in performance critical code (+ provide benchmarks). If this is one of those cases, then it's fine by me. |
|
After signature verification and database queries, amino is the most performance sensitive part of the code as far as the SDK is concerned. The defers in these functions alone accounted for a non-negligible part of the sdk's total time. (Hence they are performance critical) |
|
@ValarDragon I know, some time passed etc. But finally: golang/go@fff4f599fe |
These defers were taking up around .5% of the gaia state machines time,
due to their usage within MarshalJSON and UnmarshalBinaryBare. (exact percentage is a bit unclear)
There is no safety concern for removing these defers, as none of
the methods within the functions should panic.