Skip to content

Comments

Add a mean to get an index, allowing iterations of the queue.#1

Merged
eapache merged 3 commits intoeapache:masterfrom
aybabtme:get-index
Jun 18, 2014
Merged

Add a mean to get an index, allowing iterations of the queue.#1
eapache merged 3 commits intoeapache:masterfrom
aybabtme:get-index

Conversation

@aybabtme
Copy link
Contributor

With this change, one can iterate over the queue:

for i := 0; i < q.Length(); i++ {
    if q.Get(i).(int) != i {
        t.Errorf("index %d doesn't contain %d", i, i)
    }
}

Tests output:

$ go test -v 
=== RUN TestQueueLength
--- PASS: TestQueueLength (0.00 seconds)
=== RUN TestQueueGet
--- PASS: TestQueueGet (0.01 seconds)
=== RUN TestQueueGetOORPanic
--- PASS: TestQueueGetOORPanic (0.00 seconds)
    queue_test.go:51: got panic as expected: index out of range
    queue_test.go:65: got panic as expected: index out of range
PASS
ok      github.com/eapache/queue    0.022s

queue.go Outdated
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the godoc: "The queue implemented here is as fast as it is for two additional reasons: it is not thread-safe, and it intentionally does not follow go best-practices regarding errors - if you make a mistake with this queue (such as trying to remove an element from an empty queue) then who knows what will happen."

As such, I'm tempted to request you remove the panic, but I know that goes against the grain...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd benchmark the difference before doing that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This benchmark show 1ns difference (11% penalty):

func BenchmarkQueueGet(b *testing.B) {
    b.Logf("boundcheck=%v", boundCheck)

    q := New()
    for i := 0; i < b.N; i++ {
        q.Add(i)
    }
    b.ResetTimer()
    for i := 0; i < b.N; i++ {
        q.Get(i)
    }
}

With bound checks:

$ go test -bench BenchmarkQueueGet                                                         
PASS
BenchmarkQueueGet   100000000           15.8 ns/op
--- BENCH: BenchmarkQueueGet
    queue_test.go:95: boundcheck=true
    queue_test.go:95: boundcheck=true
    queue_test.go:95: boundcheck=true
    queue_test.go:95: boundcheck=true
    queue_test.go:95: boundcheck=true
ok      github.com/eapache/queue    7.622s

Without bound checks:

$ go test -bench BenchmarkQueueGet
PASS
BenchmarkQueueGet   100000000           14.1 ns/op
--- BENCH: BenchmarkQueueGet
    queue_test.go:95: boundcheck=false
    queue_test.go:95: boundcheck=false
    queue_test.go:95: boundcheck=false
    queue_test.go:95: boundcheck=false
    queue_test.go:95: boundcheck=false
ok      github.com/eapache/queue    7.475s

IMHO, the debugging mess I can see myself getting into is not worth the nanosec.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding bound checks to the other methods also has a much less significant impact:

Without bound check:

$ go test -bench .                                                                        
PASS
BenchmarkQueueSerial    50000000            74.7 ns/op
BenchmarkQueueGet   100000000           14.1 ns/op
BenchmarkQueueTickTock  100000000           29.8 ns/op
ok      github.com/eapache/queue    19.643s

With bound check:

$ go test -bench .
PASS
BenchmarkQueueSerial    50000000            74.9 ns/op
BenchmarkQueueGet   100000000           15.8 ns/op
BenchmarkQueueTickTock  50000000            30.0 ns/op
ok      github.com/eapache/queue    12.211s

I think it would definitely be a good idea to have bound checks.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ya, I guess. This was extracted from my channels package, and when I wrote it there the usage pattern was so simple I wasn't worried about debugging so I left them out, but now that it's its own package I guess I should add them in.

@aybabtme
Copy link
Contributor Author

I've added bound checks + tests for the checks.

queue.go Outdated
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

preferably prefix panic strings with "queue: "

also, this isn't really index out of range...

@aybabtme
Copy link
Contributor Author

addressed comments

queue.go Outdated
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

docs are now incorrect

@eapache
Copy link
Owner

eapache commented Jun 18, 2014

code looks fine now, just docs

@aybabtme
Copy link
Contributor Author

Updated the docs.

eapache added a commit that referenced this pull request Jun 18, 2014
Add a mean to get an index, allowing iterations of the queue.
@eapache eapache merged commit e423872 into eapache:master Jun 18, 2014
@aybabtme aybabtme deleted the get-index branch June 18, 2014 23:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants