-
-
Notifications
You must be signed in to change notification settings - Fork 425
Rewrite of producer-consumer example #4740
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
base: main
Are you sure you want to change the base?
Conversation
5ccf98e to
0dba8d0
Compare
addresses bugs and add assertions to verify correctness: - no internal fifo buffer overflows - the ordering invariant is maintained, such that the consumer consumes, in sequential order, the complete set of products produced.
0dba8d0 to
fb0e50f
Compare
| - to maximize throughput; | ||
|
|
||
| - while avoiding overwhelming either end. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| - to maximize throughput; | |
| - while avoiding overwhelming either end. | |
| - to maximize throughput; | |
| - while avoiding overwhelming either end. |
| ## How to Run | ||
|
|
||
| Once `producer-consumer` has been compiled, in the same directory as this README file run `./producer-consumer`. You should a set of messages from: Main, Buffer, Producer, and Consumer actors. | ||
| Once `producer-consumer` has been compiled, in the same directory as this README file run `./producer-consumer`. You should a see of a log like: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
appears to be a typo here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you keep the basic wording from before but update to include the new names of things.
| fifo: consumerRequestsNext about to provide = 1 ; now _buf.size = 2 | ||
| producer: has produced 2 | ||
| consumer: has consumed 1 | ||
| consumer: about to ask for _next = 2 | ||
| fifo: consumerRequestsNext() has _buf.size = 2 | ||
| fifo: consumerRequestsNext about to provide = 2 ; now _buf.size = 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we discussed during sync and we had a 2 to 1 "i find the Thing" easier to read. Please change the output from "thing:" to "Thing".
| 1. Read the assertions. Do they assert that the | ||
| consumer never consumes the same product twice? | ||
| If not, add an assertion to this effect. | ||
|
|
||
| 2. Verify that the ordering invariant is maintained | ||
| in this code. Can we assert this in other places? | ||
|
|
||
| In other words, assert that the reader consumes products | ||
| in the same sequential order that the producer produced them. | ||
|
|
||
| (Formally, the consumption and production sequences should map one-to-one | ||
| to the natural numbers.) | ||
|
|
||
| 3. Try adding more than one consumer. How do the invariants change? | ||
|
|
||
| 4. Try adding more than one producer. How do the invariants change? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for proper markdown editor handling, there should be no spaces between the the list items.
| 2. Verify that the ordering invariant is maintained | ||
| in this code. Can we assert this in other places? | ||
|
|
||
| In other words, assert that the reader consumes products | ||
| in the same sequential order that the producer produced them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
markdown parsing on this will break the "this is a list". most parsers dont recognize multi-line list items. please turn this into a single line item.
|
|
||
| # advanced | ||
|
|
||
| 8. Optimistic batching |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
each section has its own list. so this should be starting at 1.
| be consumeThis(prod: Product val) => // class Product | ||
| Assert.equal[I64](prod.id, _saw+1) | ||
| _saw = _saw + 1 | ||
| _out.print("consumer: has consumed " + prod.string()) | ||
| _next = _next + 1 // should == prod.id + 1 | ||
| if _next < _n then | ||
| _out.print("consumer: about to ask for _next = " + _next.string()) | ||
| _fifo.consumerRequestsNext(this, _next) | ||
| else | ||
| // notify parent | ||
| _parent.consumerDone(_h, _t0) | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
formatting is off here.
| // limit to 10 seconds | ||
| let sec10: U64 = 10_000_000_000 | ||
| h.long_test(sec10) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
formatting is off here
| // fun ref set_up(h: TestHelper val)? => | ||
| // fun ref tear_down(h: TestHelper val) => | ||
|
|
||
| fun box label(): String val => "fifo" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| fun box label(): String val => "fifo" | |
| fun label(): String val => "fifo" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is more "idiomatic"
|
|
||
| class iso _TestFifoBasic is UnitTest | ||
|
|
||
| fun name():String => "fifo/basic" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| fun name():String => "fifo/basic" | |
| fun name(): String => "fifo/basic" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that is the standard formatting from the style guide. so it is more "idiomatic" for things in ponylang org repos. you can as you want in your own.
| new create(env: Env) => PonyTest(env, this) | ||
| new make() => None | ||
| fun tag tests(test: PonyTest) => | ||
| test(_TestFifoBasic) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style guide is two spaces for indentation.
|
@glycerine i will spend some time looking at this and grab you to discuss the best way to test this for correctness. |
|
@jemc how do you feel about the assertions and having tests in the example? we don't do tests in examples at the moment but @glycerine made the excellent point that they would add support for knowing you didn't break anything if you follow the "modifications you can do" instructions. |
| use @fflush[I32](stream:Pointer[U8]) | ||
| use @write[USize](fd:I32, buf:Pointer[U8] tag, sz:USize) | ||
|
|
||
| primitive Assert |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get why you created this Assert primitive and I've done something similar before in my applications.
And I also know that this kind of facility is missing from the Pony standard library, because it it violates capability security.
But I think it's really distracting to have this kind of code as part of an example, so I'd like to see this use more idiomatic Pony test helper uses if possible. If that's not feasible, let's talk about it.
I agree its a little distracting. And I was surprised that I could not locate equivalent functionality elsewhere. But that could just be me being new. Is there equivalent already available? I tried pony_test. It does not exit immediately on assertion, making debugging difficult. To me, the ideal situation would be that the pony_assert file readily available for inclusion with a use; perhaps in the standard lib. I don't know how to summarize or finesse what seems to be a fundamental tension between correctness assertions (critical to safe and correct code and the ability to maintain it long term, in my experience; witness that the prior/existing producer/consumer example is buggy, and I'd bet good money there are other bugs in the examples, since none of them appear to be tested), and the refcap safety which is fundamental to production-time race/deadlock freedom. Most systems seem to end up with a "debug mode" and a "release mode". Even Go ended up there, despite disliking "debug mode". The race detector is exactly debug mode. edit to add: Zig even has 4 modes; Debug, ReleaseSafe, ReleaseSmall, and ReleaseFast. |
addresses bugs and add assertions to verify correctness:
no internal fifo buffer overflows
the ordering invariant is maintained, such that the consumer
consumes, in sequential order, the complete set of products produced.