Use zval storage for Stream instances created from strings #230
Conversation
nicolas-grekas
force-pushed
the
zval-stream
branch
3 times, most recently
from
943b525
to
f22ea02
Compare
|
PR is ready, I confirm this prevents the memcopy from happening. |
| { | ||
| static $wrapper; | ||
|
|
||
| $wrapper ?? \stream_wrapper_register('Nyholm-Psr7-Zval', $wrapper = \get_class(new class() { |
There was a problem hiding this comment.
Why the wrapper is registered twice - stream_wrapper_register?
Also any reason to not use ??= here / or regular if to improve the code readibility?
There was a problem hiding this comment.
It's not registered twice, where do you read that? About ??=, PHP 7.1 rulez. For the "if", I find the current style more readable, because less indentation.
There was a problem hiding this comment.
on l318 and then l407, has the 2nd register on l407 any coverage and how it is guaranteed the wrapped is not registered when the 1st fopen fails? I would say the if on l406 is not needed.
There was a problem hiding this comment.
That "if" guards against unwanted calls to stream_wrapper_unregister('Nyholm-Psr7-Zval').
There was a problem hiding this comment.
I would personally prefer to not do it as any unrelated code MUST never touch wrappers that are not owned by the code.
There was a problem hiding this comment.
I prefer keeping it, adding some guards against global scope manipulations.
|
|
||
| public function stream_truncate(int $new_size): bool | ||
| { | ||
| if ($new_size) { |
There was a problem hiding this comment.
the if condition and else should not be needed
There was a problem hiding this comment.
it's a small optimization
There was a problem hiding this comment.
I belive substr(expr, 0, 0) is optimized internally to return interned ''.
There was a problem hiding this comment.
the optimization is about removing the need to call any functions :)
nicolas-grekas
force-pushed
the
zval-stream
branch
3 times, most recently
from
c949d76
to
d978d61
Compare
nicolas-grekas
force-pushed
the
zval-stream
branch
from
d978d61
to
2f55e3d
Compare
|
PR rebased, still green. I updated the branch-alias also. |
Alternative to #227 so that we can compare both approaches.