more efficient UnsetIterator implementation#501
Conversation
Also add some property-based tests with a small corpus of values. Signed-off-by: Roger Peppe <rogpeppe@gmail.com>
rogpeppe
force-pushed
the
004-new-unsetbits-pr
branch
from
c165abe to
9ebb1c3
Compare
arraycontainer.go
Outdated
| return &shortIterator{ac.content, 0} | ||
| } | ||
|
|
||
| type arrayContainerUnsetIterator struct { |
There was a problem hiding this comment.
So, in arraycontainer, I think we don't define iterator types, they are in shortiterator.go. I think it would be more consistent to move the code there, with the other iterators.
(Of course, this changes nothing with respect to the functionality.)
arraycontainer.go
Outdated
|
|
||
| func newArrayContainerUnsetIterator(a *arrayContainer) *arrayContainerUnsetIterator { | ||
| acui := &arrayContainerUnsetIterator{content: a.content, pos: 0, nextVal: 0} | ||
| for acui.pos < len(acui.content) && uint16(acui.nextVal) == acui.content[acui.pos] { |
There was a problem hiding this comment.
| for acui.pos < len(acui.content) && uint16(acui.nextVal) == acui.content[acui.pos] { | |
| for acui.pos < len(acui.content) && uint16(acui.nextVal) >= acui.content[acui.pos] { |
I think that this would be clearer?
arraycontainer.go
Outdated
| if acui.pos < 0 { | ||
| acui.pos = -acui.pos - 1 | ||
| } | ||
| for acui.pos < len(acui.content) && uint16(acui.nextVal) == acui.content[acui.pos] { |
There was a problem hiding this comment.
| for acui.pos < len(acui.content) && uint16(acui.nextVal) == acui.content[acui.pos] { | |
| for acui.pos < len(acui.content) && uint16(acui.nextVal) >= acui.content[acui.pos] { |
arraycontainer.go
Outdated
| func (acui *arrayContainerUnsetIterator) next() uint16 { | ||
| val := acui.nextVal | ||
| acui.nextVal++ | ||
| for acui.pos < len(acui.content) && uint16(acui.nextVal) == acui.content[acui.pos] { |
There was a problem hiding this comment.
| for acui.pos < len(acui.content) && uint16(acui.nextVal) == acui.content[acui.pos] { | |
| for acui.pos < len(acui.content) && uint16(acui.nextVal) >= acui.content[acui.pos] { |
There was a problem hiding this comment.
Technically, this could be inefficient because it may force you to scan all the values in the array for naught in the user only wanted the unset bits in a range that does not contain a lot of set bits. I think that this might be fine in practice.
arraycontainer.go
Outdated
| type arrayContainerUnsetIterator struct { | ||
| content []uint16 | ||
| pos int | ||
| nextVal int |
There was a problem hiding this comment.
Could be useful to add a comment. pos is the position of a set bit that is larger than nextVal. Once nextVal gets to content[pos], we must increment pos. Something like that.
arraycontainer.go
Outdated
| return | ||
| } | ||
| acui.nextVal = int(minval) | ||
| acui.pos = binarySearch(acui.content, minval) |
There was a problem hiding this comment.
Ok. So if pos is positive after this, then we have that acui.content[pos] = minval, but if not, then we have that acui.content[pos] > minval (assuming that acui.content[pos] exists).
So we have that minval >= acui.content[pos]
and
So we have that acui.nextVal >= acui.content[pos]
There was a problem hiding this comment.
Is this a suggestion for a change, to add a comment, or just an aside explaining your understanding of the logic?
There was a problem hiding this comment.
It explains my other suggestions.
runcontainer.go
Outdated
| func (rui *runUnsetIterator16) next() uint16 { | ||
| val := rui.nextVal | ||
| rui.nextVal++ | ||
| if rui.curIndex < len(rui.rc.iv) && uint16(rui.nextVal) == rui.rc.iv[rui.curIndex].start { |
There was a problem hiding this comment.
| if rui.curIndex < len(rui.rc.iv) && uint16(rui.nextVal) == rui.rc.iv[rui.curIndex].start { | |
| if rui.curIndex < len(rui.rc.iv) && uint16(rui.nextVal) >= rui.rc.iv[rui.curIndex].start { |
|
This looks good to me. I have minor comments that you may consider. Otherwise, we shall merge. |
Signed-off-by: Roger Peppe <rogpeppe@gmail.com>
2 participants
This change aligns the efficiency of UnsetIterator with that of
Iterator: it runs in a time proportional to the number
of unset bits, not the number of set bits.
The benchmark added in this test ran at about 30s per iteration
previously and now runs in about 1.2ms.
Also add some property-based tests with a small corpus of values.
Description
Please provide a brief description of the changes made in this pull request.
Type of Change
Changes Made
What was changed?
Why was it changed?
How was it changed?
Performance Impact
Related Issues
Fixes #497