R4R: Refactor Iterator Gas Consumption #2357

alexanderbez · 2018-09-19T15:06:12Z

Cleanup gas type and store (godocs, consts, and receivers).
Remove gas consumption on gasIterator#Key and gasIterator#Value.
Add gas consumption for iterator initialization (gasKVStore#iterator) and gasIterator#Next. Next() include the dynamic cost of the value's length.
- I chose to set the flat cost for Next() to be the sum of the old flat costs of Key() (5) and Value() (10).

I did not choose to implement UnsafeGetParent as I think that should be a for a separate PR if we so decide to want it.

closes: #2017

Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
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)

store/gaskvstore.go

ValarDragon

Is the Gas Iterator based off of the CacheKVStore?

Under the current implementation, iterator creation should be charging n log n in size due to the sorting, and we don't really need to charge for next calls. If we go with the optimizations mentioned in #2286 then we'd have to make inserting into there cost log_2 n, and ignore what I said about n log n. Not sure how to do this without having cache kv store communicate more than whats in the store interface though.

If this is directly going to IAVL, this implementation makes sense.

alexanderbez · 2018-09-20T13:32:42Z

Is the Gas Iterator based off of the CacheKVStore?

Yes, well it can be -- BaseApp calls CacheMultiStore on the commit multi-store, which then a context is created based off of. When KVStore(key) is called, you get a cache-wrapped, gas metering/consuming store.

Under the current implementation, iterator creation should be charging n log n in size due to the sorting, and we don't really need to charge for next calls.

Due to items := ci.dirtyItems(ascending)? True, but can we make assumptions about the gasKVStore's parent?

... we don't really need to charge for next calls.

Why? Ultimately, a DB read could be performed, no?

cwgoes

Thanks @alexanderbez, see comments.

cwgoes · 2018-09-20T15:03:25Z

store/gaskvstore.go

 	}
-	return newGasIterator(gi.gasMeter, gi.gasConfig, parent)
+
+	gs.gasMeter.ConsumeGas(gs.gasConfig.IterInitCostFlat, sdk.GasIterInitCostFlatDesc)


I think when we call parent.Iterator (or parent.ReverseIterator) it already seeks to the first value, so we should charge key and value cost as with .Next(). I don't know if we need a flat cost but having that too is definitely safe.

Will amend to have initialization incur a cost of akin to .Next() -- this will actually result in the removal of the IterInitCostFlat.

also you have to double-check Valid() here and maybe below too before calling .Value()

otherwise it will panic!

store/gaskvstore.go

cwgoes · 2018-09-20T15:15:09Z

Is the Gas Iterator based off of the CacheKVStore?

No, the cost model is based off an underlying IAVL tree. I think we can just ignore the cache layer for now, although in the future it would be nice if we could charge less for cacheable reads or repeated writes to the same location.

ValarDragon · 2018-09-20T16:20:43Z

... we don't really need to charge for next calls.

Why? Ultimately, a DB read could be performed, no?

Iirc, when doing an iterator on a cache kv store, the dirty items are all in memory and sorted. So reading from IAVL isn't happening on next right now if its in the set of things getting sorted, as its already in memory. (Could be wrong, I have not spent that long digging through cache kv store)

No, the cost model is based off an underlying IAVL tree. I think we can just ignore the cache layer for now, although in the future it would be nice if we could charge less for cacheable reads or repeated writes to the same location.

This feels very odd that we're charging for computation thats not happening but your right, thats #postlaunch able. I'll take another look at the PR, what Bez wrote from what I remember is correct with that mindset.

alexanderbez · 2018-09-20T16:47:40Z

Gas gets called on a cacheKVStore from the context. Which returns a gasKVStore who's parent is a cacheKVStore who's parent is a iavlStore. cacheKVStore returns an iterator of type cacheMergeIterator. Which may call Next() on the iavlStore. So a read from disk may happen as I see it. Correct me if I'm wrong.

codecov · 2018-09-20T17:02:56Z

Codecov Report

❗ No coverage uploaded for pull request base (develop@91cac96). Click here to learn what that means.
The diff coverage is 79.24%.

@@            Coverage Diff            @@
##             develop   #2357   +/-   ##
=========================================
  Coverage           ?     65%           
=========================================
  Files              ?     135           
  Lines              ?    8423           
  Branches           ?       0           
=========================================
  Hits               ?    5475           
  Misses             ?    2587           
  Partials           ?     361

alexanderbez · 2018-09-21T13:30:10Z

Addressed your comments @cwgoes 👍

cwgoes

utACK

LGTM but not merging yet so @ValarDragon has another chance to review.

alexanderbez · 2018-09-25T16:10:19Z

@ValarDragon bump.

ValarDragon · 2018-09-25T16:45:24Z

store/gaskvstore.go

 	// No gas costs for deletion
-	gi.parent.Delete(key)
+	gs.parent.Delete(key)


I know you didn't change this, but why is deleting 0 cost? Shouldn't it be at least as complex as Has?

This is a good observation. We're freeing up space, but I agree it should charge the same price as Has. Thoughts @cwgoes ?

We don't charge because space is freed - but actually this might be a DoS vector though, good catch. Let's charge the same as Has.

cwgoes

utACK - thanks @alexanderbez

Implement initial iterator gas updates + cleanup

aec978b

alexanderbez added prelaunch labels Sep 19, 2018

alexanderbez requested review from jaekwon and cwgoes September 19, 2018 15:06

alexanderbez requested review from ebuchman and rigelrozanski as code owners September 19, 2018 15:06

Aleksandr Bezobchuk added 3 commits September 19, 2018 11:38

Update gas iterator costs and godocs

8892bb6

Update Next godoc

915d3c7

Reintroduce nolint to NewGasKVStore (returns unexported type)

0522a53

alexanderbez removed request for ebuchman and rigelrozanski September 19, 2018 16:57

Aleksandr Bezobchuk added 2 commits September 19, 2018 13:43

Fix gas store tests

6d56017

Update pending log

be863a0

alexanderbez changed the title ~~WIP: Refactor Iterator Gas Consumption~~ R4R: Refactor Iterator Gas Consumption Sep 19, 2018

alexanderbez added ready-for-review and removed wip labels Sep 19, 2018

alexanderbez requested a review from ValarDragon September 19, 2018 21:47

ValarDragon reviewed Sep 19, 2018

View reviewed changes

store/gaskvstore.go Outdated Show resolved Hide resolved

ValarDragon reviewed Sep 20, 2018

View reviewed changes

cwgoes suggested changes Sep 20, 2018

View reviewed changes

Update gas consumption for gasIterator

4e50ecb

cwgoes approved these changes Sep 21, 2018

View reviewed changes

ValarDragon reviewed Sep 25, 2018

View reviewed changes

alexanderbez removed the prelaunch label Sep 25, 2018

Aleksandr Bezobchuk added 2 commits September 26, 2018 09:34

Charge gas for store delete calls

ea8f457

Merge branch 'develop' into bez/2017-gas-iter-refactor

570c500

cwgoes approved these changes Sep 26, 2018

View reviewed changes

cwgoes merged commit 8ca8acf into develop Sep 26, 2018

cwgoes deleted the bez/2017-gas-iter-refactor branch September 26, 2018 14:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

R4R: Refactor Iterator Gas Consumption #2357

R4R: Refactor Iterator Gas Consumption #2357

alexanderbez commented Sep 19, 2018 •

edited

Loading

ValarDragon left a comment •

edited

Loading

alexanderbez commented Sep 20, 2018

cwgoes left a comment

cwgoes Sep 20, 2018

alexanderbez Sep 20, 2018

cwgoes Sep 20, 2018

cwgoes Sep 20, 2018

alexanderbez Sep 20, 2018

cwgoes commented Sep 20, 2018 •

edited

Loading

ValarDragon commented Sep 20, 2018 •

edited

Loading

alexanderbez commented Sep 20, 2018 •

edited

Loading

codecov bot commented Sep 20, 2018 •

edited

Loading

alexanderbez commented Sep 21, 2018

cwgoes left a comment

alexanderbez commented Sep 25, 2018

ValarDragon Sep 25, 2018 •

edited

Loading

alexanderbez Sep 25, 2018

cwgoes Sep 26, 2018

cwgoes left a comment

R4R: Refactor Iterator Gas Consumption #2357

R4R: Refactor Iterator Gas Consumption #2357

Conversation

alexanderbez commented Sep 19, 2018 • edited Loading

ValarDragon left a comment • edited Loading

Choose a reason for hiding this comment

alexanderbez commented Sep 20, 2018

cwgoes left a comment

Choose a reason for hiding this comment

cwgoes Sep 20, 2018

Choose a reason for hiding this comment

alexanderbez Sep 20, 2018

Choose a reason for hiding this comment

cwgoes Sep 20, 2018

Choose a reason for hiding this comment

cwgoes Sep 20, 2018

Choose a reason for hiding this comment

alexanderbez Sep 20, 2018

Choose a reason for hiding this comment

cwgoes commented Sep 20, 2018 • edited Loading

ValarDragon commented Sep 20, 2018 • edited Loading

alexanderbez commented Sep 20, 2018 • edited Loading

codecov bot commented Sep 20, 2018 • edited Loading

Codecov Report

alexanderbez commented Sep 21, 2018

cwgoes left a comment

Choose a reason for hiding this comment

alexanderbez commented Sep 25, 2018

ValarDragon Sep 25, 2018 • edited Loading

Choose a reason for hiding this comment

alexanderbez Sep 25, 2018

Choose a reason for hiding this comment

cwgoes Sep 26, 2018

Choose a reason for hiding this comment

cwgoes left a comment

Choose a reason for hiding this comment

alexanderbez commented Sep 19, 2018 •

edited

Loading

ValarDragon left a comment •

edited

Loading

cwgoes commented Sep 20, 2018 •

edited

Loading

ValarDragon commented Sep 20, 2018 •

edited

Loading

alexanderbez commented Sep 20, 2018 •

edited

Loading

codecov bot commented Sep 20, 2018 •

edited

Loading

ValarDragon Sep 25, 2018 •

edited

Loading