Speedup IAVL iterator by removing defers when unneeded.

[ValarDragon]

Note each defer occurs a 30ish ns overhead here, which is significant
for IAVL iteration. We were previously using around 3-4 defers. (2 in
next, one in Valid, one in Key, one in Value) This slows down the entire
application quite significantly, as we require fast iteration.

This benchmark is just for the next operation, we will see 50% of this performance gain in the Valid and Value functions as well, so the performance gain in practice may be 2x this.

Before:

BenchmarkIAVLIteratorNext-8   	 3000000	       534 ns/op	       0 B/op	       0 allocs/op

After:

BenchmarkIAVLIteratorNext-8   	 3000000	       466 ns/op	       0 B/op	       0 allocs/op

This means this change could give us around 140 ns of improvement per iteration -- quite significant.

• Targeted PR against correct branch (see CONTRIBUTING.md)

• Wrote tests

• Updated relevant documentation (docs/)

• Added entries in PENDING.md with issue #

• rereviewed Files changed in the github PR explorer

---

For Admin Use:

• Added appropriate labels to PR (ex. wip, ready-for-review, docs)
• Reviewers Assigned
• Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

[codecov]

Codecov Report

Merging #2143 into develop will decrease coverage by 0.06%.
The diff coverage is 88.88%.

@@             Coverage Diff             @@
##           develop    #2143      +/-   ##
===========================================
- Coverage    63.91%   63.85%   -0.07%     
===========================================
  Files          134      134              
  Lines         8194     8180      -14     
===========================================
- Hits          5237     5223      -14     
- Misses        2604     2607       +3     
+ Partials       353      350       -3

[alexanderbez]

utACK -- thanks!

[alexanderbez]

Mhmmm, I think it's finally worthwhile committing to a change log format standard.

/cc @rigelrozanski @cwgoes

[ValarDragon]

Can we not block this pr on that tho, and decide it in a separate pr and format accordingly there.

[alexanderbez]

I'm not suggesting we block (I approved actually). I'm just stating it here for reference

[ValarDragon]

Thanks! I misunderstood, totally agree with standardizing this

[codecov]

Codecov Report

Merging #2143 into develop will decrease coverage by <.01%.
The diff coverage is 88.88%.

@@             Coverage Diff             @@
##           develop    #2143      +/-   ##
===========================================
- Coverage    63.91%   63.91%   -0.01%     
===========================================
  Files          134      134              
  Lines         8194     8199       +5     
===========================================
+ Hits          5237     5240       +3     
- Misses        2604     2606       +2     
  Partials       353      353

[cwgoes]

Are we sure all of these removals are safe (are the underlying IAVL functions guaranteed not to panic)? Keep in mind that many IAVL store calls (e.g. when running transactions) may be recovered at a higher level, so the daemon would continue to run even if the IAVL read failed (although arguably we should crash in that case...)

[ValarDragon]

The only thing where that's a concern is receive next. However the point of panicking there is the iavl stating this is irrecoverable, otherwise it should be erroring

[ValarDragon]

To be more clear, the mutex remains on that iterator. If the next method panicked, we should not keep on using that iterator, even if we recover. (hence the mutex remaining locked in that condition is fine)

For every other scenario, we unlock in all situations where we may panic (unlock in assertIsValid)

[cwgoes]

To be more clear, the mutex remains on that iterator. If the next method panicked, we should not keep on using that iterator, even if we recover. (hence the mutex remaining locked in that condition is fine)
For every other scenario, we unlock in all situations where we may panic (unlock in assertIsValid)

👍