[WIP] Immediate writes to underlying stream until buffer is filled #54
Conversation
clue
changed the title
|
Rebased now that #52 is in. Ready for review |
|
LGTM |
|
Can we get this merged? It bit me today and wasted many hours. |
|
Agreed @davidwdan lets get this in. @clue what is holding us back from merging this and related issues? |
| { | ||
| $stream = fopen('php://temp', 'r+'); | ||
| $loop = $this->createWriteableLoopMock(); | ||
| $loop->preventWrites = true; | ||
| $loop = $this->createLoopMock(); | ||
|
|
||
| $buffer = new Buffer($stream, $loop); | ||
| $buffer->softLimit = 4; | ||
| $buffer->on('error', $this->expectCallableNever()); | ||
| $buffer->on('drain', $this->expectCallableOnce()); |
There was a problem hiding this comment.
Does it really make sense to emit a drain event here?
After reconsidering this, we should probably not trigger this event if the data can be flushed immediately. Even though the data may exceed the buffer size, this is an atomic step that can not be observed from the outside.
Instead, we should probably only trigger this event if the buffer returned false previously and it is now ready again.
Absolutely! 👍
Huh, care to elaborate? |
clue
force-pushed
the
immediate-writes
branch
from
96cc409 to
44dbe38
Compare
|
Rebased now that #55 is in. I've moved this PR to the next milestone because it requires an update to not emit a |
|
Removing the milestone for now until the |
clue
changed the title
clue
dismissed
WyriHaximus’s stale review
Turns out we need to reconsider the drain event first
|
The main focus is currently on getting the v1.0.0 out. As such, there are currently no immediate plans to build this from my end, so I'm closing this for now 👍 |
|
@clue - Any chance of getting this going again? |
|
@mbonneau I'm currently busy working on the other components and this feature does not seem to have a high priority right now, but in case anybody feels like picking this up, I'm happy to help |
With this change applied, the
Bufferwill immediately write to the underlying stream instead of waiting for the loop to say it it writable. This is safe because the stream is non-blocking anyway and the Buffer class will return to normal buffer behavior once the data can not be flushed immediately.Running the examples/benchmark-throughput.php script, this change alone increases performance from ~100 MiB/s to ~160 MiB/s on my (rather mediocre) test system, using PHP 7.0.8.
Combined with #53, it increases performance from ~100 MiB/s to ~1900 MiB/s.
Marking this as WIP because this needs some additional tests once #52 is in.Edit: done.