Skip to main content
Code Review

Return to Answer

added 155 characters in body
Source Link
J_H
J_H
  • 42.3k
  • 3
  • 38
  • 157

versionitis

This submission did not include a maven pom.xml file. Consider adding one, and using that to deal with SemVer details. Names like BoundedBufferV2 seem less convenient than e.g. BoundedBuffer.

documentation

This submission contains no /** javadoc */ remarks.

Dealing with concurrent processing can be a bit tricky, and typically the key to correct code is carefully documenting the class invariants, the things that are always true across all interleavings of thread execution. That didn't happen here.

Given such a spec, we could then reason about each method in isolation, asking if it preserves our invariants.

automated test suite

There isn't one.

More than one library would offer a convenient framework for structuring your tests so others could verify that invariants hold, even when run against future JVMs and future libraries that may be released in the coming years.

invariants

It appears the key predicate you wish to enforce is bounded memory consumption, so the deque will never have more than e.g. 10 entries.

Yet we have two semaphores plus a mutex, in addition to the deque. Which seems like more coordination datastructures than necessary. The names empty and full are less helpful than a numRecords (or numRemainingSlots) coordination variable would be.

We do not see the synchronized keyword in this source code, though it could have simplified the code. If that was a deliberate design decision, it would be worth writing that down explicitly, as comments in the source.

records per second

Throughput is calibrated to be ~ 1 record produced per second, and ~ 1 record consumed per second. I was expecting that one or the other would be slightly higher, so we settle into steady state of either mostly empty or full.

Or perhaps you'd care to randomly perturb the rate every so often.

Also, bursts of producing, and bursts of consuming, would be of interest. Where a burst might fill the deque, or completely drain it.

battery burnerlatency

I do not understand why we're polling with .tryAcquire(), rather than blocking indefinitely or blocking with a timeout. Polling means that consumer sees records that are more stale than necessary. If we block, then we can obtain a fresh record immediately after it is produced. Consider producing timestamp records, rather than random numbers. Then the consumer could report on how long records lingered in the queue.

Given the while (true) loops, it seems reasonable for at least one side to patiently wait.

There are some hidden assumptions behind this code. Please make them explicit -- write them down within the source. Then future maintenance engineers will be in a frame of mind similar to how you're currently viewing the problem space.

versionitis

This submission did not include a maven pom.xml file. Consider adding one, and using that to deal with SemVer details. Names like BoundedBufferV2 seem less convenient than e.g. BoundedBuffer.

documentation

This submission contains no /** javadoc */ remarks.

Dealing with concurrent processing can be a bit tricky, and typically the key to correct code is carefully documenting the class invariants, the things that are always true across all interleavings of thread execution. That didn't happen here.

Given such a spec, we could then reason about each method in isolation, asking if it preserves our invariants.

automated test suite

There isn't one.

More than one library would offer a convenient framework for structuring your tests so others could verify that invariants hold, even when run against future JVMs and future libraries that may be released in the coming years.

invariants

It appears the key predicate you wish to enforce is bounded memory consumption, so the deque will never have more than e.g. 10 entries.

Yet we have two semaphores plus a mutex, in addition to the deque. Which seems like more coordination datastructures than necessary. The names empty and full are less helpful than a numRecords (or numRemainingSlots) coordination variable would be.

We do not see the synchronized keyword in this source code, though it could have simplified the code. If that was a deliberate design decision, it would be worth writing that down explicitly, as comments in the source.

records per second

Throughput is calibrated to be ~ 1 record produced per second, and ~ 1 record consumed per second. I was expecting that one or the other would be slightly higher, so we settle into steady state of either mostly empty or full.

Or perhaps you'd care to randomly perturb the rate every so often.

Also, bursts of producing, and bursts of consuming, would be of interest. Where a burst might fill the deque, or completely drain it.

battery burner

I do not understand why we're polling with .tryAcquire(), rather than blocking indefinitely or blocking with a timeout.

Given the while (true) loops, it seems reasonable for at least one side to patiently wait.

There are some hidden assumptions behind this code. Please make them explicit -- write them down within the source. Then future maintenance engineers will be in a frame of mind similar to how you're currently viewing the problem space.

versionitis

This submission did not include a maven pom.xml file. Consider adding one, and using that to deal with SemVer details. Names like BoundedBufferV2 seem less convenient than e.g. BoundedBuffer.

documentation

This submission contains no /** javadoc */ remarks.

Dealing with concurrent processing can be a bit tricky, and typically the key to correct code is carefully documenting the class invariants, the things that are always true across all interleavings of thread execution. That didn't happen here.

Given such a spec, we could then reason about each method in isolation, asking if it preserves our invariants.

automated test suite

There isn't one.

More than one library would offer a convenient framework for structuring your tests so others could verify that invariants hold, even when run against future JVMs and future libraries that may be released in the coming years.

invariants

It appears the key predicate you wish to enforce is bounded memory consumption, so the deque will never have more than e.g. 10 entries.

Yet we have two semaphores plus a mutex, in addition to the deque. Which seems like more coordination datastructures than necessary. The names empty and full are less helpful than a numRecords (or numRemainingSlots) coordination variable would be.

We do not see the synchronized keyword in this source code, though it could have simplified the code. If that was a deliberate design decision, it would be worth writing that down explicitly, as comments in the source.

records per second

Throughput is calibrated to be ~ 1 record produced per second, and ~ 1 record consumed per second. I was expecting that one or the other would be slightly higher, so we settle into steady state of either mostly empty or full.

Or perhaps you'd care to randomly perturb the rate every so often.

Also, bursts of producing, and bursts of consuming, would be of interest. Where a burst might fill the deque, or completely drain it.

latency

I do not understand why we're polling with .tryAcquire(), rather than blocking indefinitely or blocking with a timeout. Polling means that consumer sees records that are more stale than necessary. If we block, then we can obtain a fresh record immediately after it is produced. Consider producing timestamp records, rather than random numbers. Then the consumer could report on how long records lingered in the queue.

Given the while (true) loops, it seems reasonable for at least one side to patiently wait.

There are some hidden assumptions behind this code. Please make them explicit -- write them down within the source. Then future maintenance engineers will be in a frame of mind similar to how you're currently viewing the problem space.

deleted 1 character in body
Source Link
J_H
J_H
  • 42.3k
  • 3
  • 38
  • 157

versionitis

This submission did not include a maven pom.xml file. Consider adding one, and using that to deal with SemVer details. Names like BoundedBufferV2 seem less convenient than e.g. BoundedBuffer.

documentation

This submission contains no /** javadoc */ remarks.

Dealing with concurrent processing can be a bit tricky, and typically the key to correct code is carefully documenting the class invariants, the things that are always true across all interleavings of thread execution. That didn't happen here.

Given such a spec, we could then reason about each method in isolation, asking if it preserves our invariants.

automated test suite

There isn't one.

More than one library would offer a convenient framework for structuring your tests so others could verify that invariants hold, even when run against future JVMs and future libraries that may be released in the coming years.

invariants

It appears the key predicate you wish to enforce is bounded memory consumption, so the deque will never have more than e.g. 10 entries.

Yet we have two semaphores plus a mutex, in addition to the deque. Which seems like more coordination datastructures than necessary. The names empty and full are less helpful than a numRecords (or numRemainingSlots) coordination variable would be.

We do not see the synchronized keyword in this source code, though it could have simplified the code. If that was a deliberate design decision, it would be worth writing that down explicitly, as comments in the source.

records per second

Throughput is calibrated to be ~ 1 record produced per second, and ~ 1 record consumed per second. I was expecting that one or the other would be slightly higher, so we settle into steady state of either mostly empty or full.

Or perhaps you'd care to randomly perturb the rate every so often.

Also, bursts of producing, and bursts of consuming, would be of interest. Where a burst might fill the deque, or completely drain it.

battery burner

I do not understand why we're polling with .tryAcquire(), rather than blocking indefinitely or blocking with a timeout.

Given the while (true) loops, it seems reasonable for at least one side to patiently wait.

There are some hidden assumptions behind this code. Please make them explicit -- write them down within the source. Then future maintenance engineers will be in a similar frame of mind tosimilar to how you areyou're currently viewing the problem space.

versionitis

This submission did not include a maven pom.xml file. Consider adding one, and using that to deal with SemVer details. Names like BoundedBufferV2 seem less convenient than e.g. BoundedBuffer.

documentation

This submission contains no /** javadoc */ remarks.

Dealing with concurrent processing can be a bit tricky, and typically the key to correct code is carefully documenting the class invariants, the things that are always true across all interleavings of thread execution. That didn't happen here.

Given such a spec, we could then reason about each method in isolation, asking if it preserves our invariants.

automated test suite

There isn't one.

More than one library would offer a convenient framework for structuring your tests so others could verify that invariants hold, even when run against future JVMs and future libraries that may be released in the coming years.

invariants

It appears the key predicate you wish to enforce is bounded memory consumption, so the deque will never have more than e.g. 10 entries.

Yet we have two semaphores plus a mutex, in addition to the deque. Which seems like more coordination datastructures than necessary. The names empty and full are less helpful than a numRecords (or numRemainingSlots) coordination variable would be.

We do not see the synchronized keyword in this source code, though it could have simplified the code. If that was a deliberate design decision, it would be worth writing that down explicitly, as comments in the source.

records per second

Throughput is calibrated to be ~ 1 record produced per second, and ~ 1 record consumed per second. I was expecting that one or the other would be slightly higher, so we settle into steady state of either mostly empty or full.

Or perhaps you'd care to randomly perturb the rate every so often.

Also, bursts of producing, and bursts of consuming, would be of interest. Where a burst might fill the deque, or completely drain it.

battery burner

I do not understand why we're polling with .tryAcquire(), rather than blocking indefinitely or blocking with a timeout.

Given the while (true) loops, it seems reasonable for at least one side to patiently wait.

There are some hidden assumptions behind this code. Please make them explicit -- write them down within the source. Then future maintenance engineers will be in a similar frame of mind to how you are currently viewing the problem space.

versionitis

This submission did not include a maven pom.xml file. Consider adding one, and using that to deal with SemVer details. Names like BoundedBufferV2 seem less convenient than e.g. BoundedBuffer.

documentation

This submission contains no /** javadoc */ remarks.

Dealing with concurrent processing can be a bit tricky, and typically the key to correct code is carefully documenting the class invariants, the things that are always true across all interleavings of thread execution. That didn't happen here.

Given such a spec, we could then reason about each method in isolation, asking if it preserves our invariants.

automated test suite

There isn't one.

More than one library would offer a convenient framework for structuring your tests so others could verify that invariants hold, even when run against future JVMs and future libraries that may be released in the coming years.

invariants

It appears the key predicate you wish to enforce is bounded memory consumption, so the deque will never have more than e.g. 10 entries.

Yet we have two semaphores plus a mutex, in addition to the deque. Which seems like more coordination datastructures than necessary. The names empty and full are less helpful than a numRecords (or numRemainingSlots) coordination variable would be.

We do not see the synchronized keyword in this source code, though it could have simplified the code. If that was a deliberate design decision, it would be worth writing that down explicitly, as comments in the source.

records per second

Throughput is calibrated to be ~ 1 record produced per second, and ~ 1 record consumed per second. I was expecting that one or the other would be slightly higher, so we settle into steady state of either mostly empty or full.

Or perhaps you'd care to randomly perturb the rate every so often.

Also, bursts of producing, and bursts of consuming, would be of interest. Where a burst might fill the deque, or completely drain it.

battery burner

I do not understand why we're polling with .tryAcquire(), rather than blocking indefinitely or blocking with a timeout.

Given the while (true) loops, it seems reasonable for at least one side to patiently wait.

There are some hidden assumptions behind this code. Please make them explicit -- write them down within the source. Then future maintenance engineers will be in a frame of mind similar to how you're currently viewing the problem space.

Source Link
J_H
J_H
  • 42.3k
  • 3
  • 38
  • 157

versionitis

This submission did not include a maven pom.xml file. Consider adding one, and using that to deal with SemVer details. Names like BoundedBufferV2 seem less convenient than e.g. BoundedBuffer.

documentation

This submission contains no /** javadoc */ remarks.

Dealing with concurrent processing can be a bit tricky, and typically the key to correct code is carefully documenting the class invariants, the things that are always true across all interleavings of thread execution. That didn't happen here.

Given such a spec, we could then reason about each method in isolation, asking if it preserves our invariants.

automated test suite

There isn't one.

More than one library would offer a convenient framework for structuring your tests so others could verify that invariants hold, even when run against future JVMs and future libraries that may be released in the coming years.

invariants

It appears the key predicate you wish to enforce is bounded memory consumption, so the deque will never have more than e.g. 10 entries.

Yet we have two semaphores plus a mutex, in addition to the deque. Which seems like more coordination datastructures than necessary. The names empty and full are less helpful than a numRecords (or numRemainingSlots) coordination variable would be.

We do not see the synchronized keyword in this source code, though it could have simplified the code. If that was a deliberate design decision, it would be worth writing that down explicitly, as comments in the source.

records per second

Throughput is calibrated to be ~ 1 record produced per second, and ~ 1 record consumed per second. I was expecting that one or the other would be slightly higher, so we settle into steady state of either mostly empty or full.

Or perhaps you'd care to randomly perturb the rate every so often.

Also, bursts of producing, and bursts of consuming, would be of interest. Where a burst might fill the deque, or completely drain it.

battery burner

I do not understand why we're polling with .tryAcquire(), rather than blocking indefinitely or blocking with a timeout.

Given the while (true) loops, it seems reasonable for at least one side to patiently wait.

There are some hidden assumptions behind this code. Please make them explicit -- write them down within the source. Then future maintenance engineers will be in a similar frame of mind to how you are currently viewing the problem space.