New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ctrie Iterator #89

Merged
merged 7 commits into from May 18, 2015

Conversation

Projects
None yet
5 participants
@tylertreat-wf
Contributor

tylertreat-wf commented May 15, 2015

Adds Iterator, Size, and Clear operations to the Ctrie. Also adds a Map function to the persistent list.

@dustinhiatt-wf @alexandercampbell-wf @stevenosborne-wf @beaulyddon-wf @rosshendrickson-wf @tannermiller-wf

Show outdated Hide outdated trie/ctrie/ctrie.go
// computation is amortized across the update operations that occurred
// since the last snapshot.
size := uint(0)
for _ = range c.Iterator() {

This comment has been minimized.

@alexandercampbell-wf

alexandercampbell-wf May 15, 2015

Contributor

This can just be for range c.Iterator()

@alexandercampbell-wf

alexandercampbell-wf May 15, 2015

Contributor

This can just be for range c.Iterator()

This comment has been minimized.

@tylertreat-wf

tylertreat-wf May 15, 2015

Contributor

Is that compatible with go 1.3? Are we still trying to ensure compatibility with 1.3?

@tylertreat-wf

tylertreat-wf May 15, 2015

Contributor

Is that compatible with go 1.3? Are we still trying to ensure compatibility with 1.3?

This comment has been minimized.

@alexandercampbell-wf

alexandercampbell-wf May 15, 2015

Contributor

Good point-- the new syntax was not adopted until 1.4

@alexandercampbell-wf

alexandercampbell-wf May 15, 2015

Contributor

Good point-- the new syntax was not adopted until 1.4

Show outdated Hide outdated trie/ctrie/ctrie.go
close(ch)
}()
return ch
}

This comment has been minimized.

@alexandercampbell-wf

alexandercampbell-wf May 15, 2015

Contributor

What happens if you only iterate over the first, say, 5 elements of the trie? Would dead channel remain open in the background?

@alexandercampbell-wf

alexandercampbell-wf May 15, 2015

Contributor

What happens if you only iterate over the first, say, 5 elements of the trie? Would dead channel remain open in the background?

This comment has been minimized.

@dustinhiatt-wf

dustinhiatt-wf May 15, 2015

Contributor

Which would probably also keep the goroutine live. This might be leaky in the case where the iterator wasn't exhausted during iteration.

@dustinhiatt-wf

dustinhiatt-wf May 15, 2015

Contributor

Which would probably also keep the goroutine live. This might be leaky in the case where the iterator wasn't exhausted during iteration.

This comment has been minimized.

@tylertreat-wf

tylertreat-wf May 15, 2015

Contributor

Damn, you're right, this is problematic because if the user doesn't read from the channel the goroutine will block. Any ideas on making a nicer iterator which doesn't have this problem?

@tylertreat-wf

tylertreat-wf May 15, 2015

Contributor

Damn, you're right, this is problematic because if the user doesn't read from the channel the goroutine will block. Any ideas on making a nicer iterator which doesn't have this problem?

This comment has been minimized.

@tylertreat-wf

tylertreat-wf May 15, 2015

Contributor

Will probably just go with a stateful iterator with HasNext() and Next(), which means you can't range. Go is bullshit.

@tylertreat-wf

tylertreat-wf May 15, 2015

Contributor

Will probably just go with a stateful iterator with HasNext() and Next(), which means you can't range. Go is bullshit.

This comment has been minimized.

@dustinhiatt-wf

dustinhiatt-wf May 15, 2015

Contributor

Switch to C# or Rust :).

You can also just return an object that supports Next and Value and do for iter := c.Iter(); iter.Next(); { val := iter.Value() }

@dustinhiatt-wf

dustinhiatt-wf May 15, 2015

Contributor

Switch to C# or Rust :).

You can also just return an object that supports Next and Value and do for iter := c.Iter(); iter.Next(); { val := iter.Value() }

This comment has been minimized.

@dustinhiatt-wf

dustinhiatt-wf May 15, 2015

Contributor

Damn, beat me, and yes this is another case where's Go's demos about channels look better than they are.

@dustinhiatt-wf

dustinhiatt-wf May 15, 2015

Contributor

Damn, beat me, and yes this is another case where's Go's demos about channels look better than they are.

Allow way to cancel Ctrie iterator
This allows a cancel channel to be passed in to the Ctrie Iterator. When
the channel is closed, the iterator channel will close, freeing up the
goroutine.
@tylertreat

This comment has been minimized.

Show comment
Hide comment
@tylertreat

tylertreat May 16, 2015

Contributor

Think I found a decent compromise on the iterator nonsense @dustinhiatt-wf @alexandercampbell-wf. See last commit.

Contributor

tylertreat commented May 16, 2015

Think I found a decent compromise on the iterator nonsense @dustinhiatt-wf @alexandercampbell-wf. See last commit.

@beaulyddon-wf

This comment has been minimized.

Show comment
Hide comment
@beaulyddon-wf

beaulyddon-wf May 18, 2015

lol. The rule of Go.... when all else fails just add another channel.

+1

beaulyddon-wf commented May 18, 2015

lol. The rule of Go.... when all else fails just add another channel.

+1

@alexandercampbell-wf

This comment has been minimized.

Show comment
Hide comment
@alexandercampbell-wf

alexandercampbell-wf May 18, 2015

Contributor

+1, but I can't say I like introducing another channel.

Contributor

alexandercampbell-wf commented May 18, 2015

+1, but I can't say I like introducing another channel.

@dustinhiatt-wf

This comment has been minimized.

Show comment
Hide comment
@dustinhiatt-wf

dustinhiatt-wf May 18, 2015

Contributor

I can merge this, but I have to agree with @alexandercampbell-wf, I'd much prefer to use a stateful iterator instead of leaving it up to the consumer to ensure memory isn't leaked. Especially for people coming from other languages, the duty of having to create a channel and then cancel is a burden people probably aren't used to for a simple iteration.

@tylertreat, I'll merge as is if you want, but I'm just worried people will just forget to cancel or pass in nil and this thing will leak memory. Up to you.

Contributor

dustinhiatt-wf commented May 18, 2015

I can merge this, but I have to agree with @alexandercampbell-wf, I'd much prefer to use a stateful iterator instead of leaving it up to the consumer to ensure memory isn't leaked. Especially for people coming from other languages, the duty of having to create a channel and then cancel is a burden people probably aren't used to for a simple iteration.

@tylertreat, I'll merge as is if you want, but I'm just worried people will just forget to cancel or pass in nil and this thing will leak memory. Up to you.

@tylertreat-wf

This comment has been minimized.

Show comment
Hide comment
@tylertreat-wf

tylertreat-wf May 18, 2015

Contributor

The stateful iterator will be surprisingly nasty with this type of trie, at least if you want to do it in a "generator" style.

Contributor

tylertreat-wf commented May 18, 2015

The stateful iterator will be surprisingly nasty with this type of trie, at least if you want to do it in a "generator" style.

dustinhiatt-wf added a commit that referenced this pull request May 18, 2015

@dustinhiatt-wf dustinhiatt-wf merged commit 9257431 into Workiva:master May 18, 2015

@tylertreat-wf tylertreat-wf deleted the tylertreat-wf:iterator branch May 18, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment