Add single-collection gkvlite iterator.

Add single-collection gkvlite iterator.

Multi-collection iterator is up next.

R=dnj@google.com, dnj@chromium.org, estaab@chromium.org, tandrii@chromium.org, vadimsh@chromium.org
BUG=

Committed: https://github.com/luci/gae/commit/92ebff557e996bc9f26dda987cff263e81cfd9a7

[dnj (Google), 2015-08-15 02:00:31]

dnj@google.com changed reviewers:
+ dnj@google.com

[dnj (Google), 2015-08-15 02:00:31]

What am I missing here? The iterator is clearly not goroutine-safe, since you
don't lock around its internals. You have a command channel set up between the
public methods and a goroutine, but the overall flow is serial (main routine
blocks on goroutine completion).

Is the goroutine just a way to keep internal iteration state (Python generator
style)? Seems odd and expensive; can we just store the state in a struct?

https://codereview.chromium.org/1300493002/diff/1/impl/memory/gkvlite_iter.go
File impl/memory/gkvlite_iter.go (right):

https://codereview.chromium.org/1300493002/diff/1/impl/memory/gkvlite_iter.go...
impl/memory/gkvlite_iter.go:99: defer func() { recover() }()
If your goal is to close the channel once, wrap it in a sync.Once.

https://codereview.chromium.org/1300493002/diff/1/impl/memory/gkvlite_iter.go...
impl/memory/gkvlite_iter.go:114: } else if bytes.Compare(t.prev, targ) >= 0 {
Is this a necessary check? Could be expensive doing an extra byte comparison
each round.

https://codereview.chromium.org/1300493002/diff/1/impl/memory/gkvlite_iter.go...
impl/memory/gkvlite_iter.go:128: close(waiter)
Defer this close, dawg.

[iannucci, 2015-08-15 02:05:13]

On 2015/08/15 02:00:31, dnj (Google) wrote:
> What am I missing here? The iterator is clearly not goroutine-safe, since you
> don't lock around its internals. 

Not supposed to be

> You have a command channel set up between the
> public methods and a goroutine, but the overall flow is serial (main routine
> blocks on goroutine completion).
> 

Yes.

> Is the goroutine just a way to keep internal iteration state (Python generator
> style)? Seems odd and expensive; can we just store the state in a struct?

Iteration in gkvlite is done via a callback (VisitItemsAscend). The only way to
store the state 
would be to freeze the stack, which is what I'm doing (by having a blocked
goroutine).

The alternative would be to call VisitItemsAscend once per iteration, but that's
quite expensive,
as it creates a new snapshot of the collection, reference counts it, and then
adds a bunch of
guard/checking logic around the subsequent iteration.

> 
> https://codereview.chromium.org/1300493002/diff/1/impl/memory/gkvlite_iter.go
> File impl/memory/gkvlite_iter.go (right):
> 
>
https://codereview.chromium.org/1300493002/diff/1/impl/memory/gkvlite_iter.go...
> impl/memory/gkvlite_iter.go:99: defer func() { recover() }()
> If your goal is to close the channel once, wrap it in a sync.Once.
> 
>
https://codereview.chromium.org/1300493002/diff/1/impl/memory/gkvlite_iter.go...
> impl/memory/gkvlite_iter.go:114: } else if bytes.Compare(t.prev, targ) >= 0 {
> Is this a necessary check? Could be expensive doing an extra byte comparison
> each round.
> 
>
https://codereview.chromium.org/1300493002/diff/1/impl/memory/gkvlite_iter.go...
> impl/memory/gkvlite_iter.go:128: close(waiter)
> Defer this close, dawg.

[iannucci, 2015-08-15 02:10:11]

done

https://chromiumcodereview.appspot.com/1300493002/diff/1/impl/memory/gkvlite_...
File impl/memory/gkvlite_iter.go (right):

https://chromiumcodereview.appspot.com/1300493002/diff/1/impl/memory/gkvlite_...
impl/memory/gkvlite_iter.go:99: defer func() { recover() }()
On 2015/08/15 02:00:31, dnj (Google) wrote:
> If your goal is to close the channel once, wrap it in a sync.Once.

Done.

https://chromiumcodereview.appspot.com/1300493002/diff/1/impl/memory/gkvlite_...
impl/memory/gkvlite_iter.go:114: } else if bytes.Compare(t.prev, targ) >= 0 {
On 2015/08/15 02:00:31, dnj (Google) wrote:
> Is this a necessary check? Could be expensive doing an extra byte comparison
> each round.

it's only when you skip

https://chromiumcodereview.appspot.com/1300493002/diff/1/impl/memory/gkvlite_...
impl/memory/gkvlite_iter.go:128: close(waiter)
On 2015/08/15 02:00:31, dnj (Google) wrote:
> Defer this close, dawg.

Done.

[dnj (Google), 2015-08-15 02:10:39]

I see, okay thanks for the explanation. Added an idea.

https://chromiumcodereview.appspot.com/1300493002/diff/1/impl/memory/gkvlite_...
File impl/memory/gkvlite_iter.go (right):

https://chromiumcodereview.appspot.com/1300493002/diff/1/impl/memory/gkvlite_...
impl/memory/gkvlite_iter.go:120: waiter := make(chan struct{})
Idea:

itemC := make(chan *gkvlite.Item)
t.ch <- &cmd(targ, itemC)
for i := range itemC {
  cb(i)
  i.prev = i.Key
}
cb(nil)
t.stop()

[dnj (Google), 2015-08-15 02:12:10]

On 2015/08/15 02:10:39, dnj (Google) wrote:
> I see, okay thanks for the explanation. Added an idea.
> 
>
https://chromiumcodereview.appspot.com/1300493002/diff/1/impl/memory/gkvlite_...
> File impl/memory/gkvlite_iter.go (right):
> 
>
https://chromiumcodereview.appspot.com/1300493002/diff/1/impl/memory/gkvlite_...
> impl/memory/gkvlite_iter.go:120: waiter := make(chan struct{})
> Idea:

Er nm forget the idea, misses the point. lgtm

[iannucci, 2015-08-15 02:12:20]

On 2015/08/15 02:10:11, iannucci wrote:
> done
> 
>
https://chromiumcodereview.appspot.com/1300493002/diff/1/impl/memory/gkvlite_...
> File impl/memory/gkvlite_iter.go (right):
> 
>
https://chromiumcodereview.appspot.com/1300493002/diff/1/impl/memory/gkvlite_...
> impl/memory/gkvlite_iter.go:99: defer func() { recover() }()
> On 2015/08/15 02:00:31, dnj (Google) wrote:
> > If your goal is to close the channel once, wrap it in a sync.Once.
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/1300493002/diff/1/impl/memory/gkvlite_...
> impl/memory/gkvlite_iter.go:114: } else if bytes.Compare(t.prev, targ) >= 0 {
> On 2015/08/15 02:00:31, dnj (Google) wrote:
> > Is this a necessary check? Could be expensive doing an extra byte comparison
> > each round.
> 
> it's only when you skip
> 

actually, took this out anyway.

>
https://chromiumcodereview.appspot.com/1300493002/diff/1/impl/memory/gkvlite_...
> impl/memory/gkvlite_iter.go:128: close(waiter)
> On 2015/08/15 02:00:31, dnj (Google) wrote:
> > Defer this close, dawg.
> 
> Done.

[iannucci, 2015-08-15 02:16:52]

took out the rewind check

[iannucci, 2015-08-15 21:54:53]

Committed patchset #3 (id:40001) manually as
92ebff557e996bc9f26dda987cff263e81cfd9a7 (presubmit successful).