Closed domoran closed 6 years ago
Requires a performance test to prevent regressions. Unsure how to achieve that besides using relative timing of the regex applied to some data, and a large chunk of text passed to this method.
I added a performance test, by crafting a "worst case" packet and measuring relative timing.
Verified that test fails on old version: git checkout e89dac73d56b18023b8e2e41074e64bb3abbdfda src/Response.php
Edit: Fixed the formatting.
Fixed formatting comments.
Hmm ... the test seems flaky, failed under PHP 7 for the last build ... I cannot think of a way to make the test more stable except by increasing its runtime. (More data, increase ratio) Any ideas?
@domoran flakiness can probably be avoided by repeating the test, but indeed, it's a time-based test, so it will always be a risky one. Make sure the first iteration is removed from measurement to avoid autolading and other possible performance issues there.
Sorry for the previous commits - previous review from me was never submitted.
I dont get it. There must be a logic error in the test or a problem on php7.
How can one more regexp iteration inside the decodeChunkedBody be factor 30-50(!) slower than the baseline? This happens only on travis-ci and only with php 7 ...
On my machine I cannot reproduce. Even with php7 I get a ratio of 3-5 ... But with php5.6 there is the expected runtime ratio of 1 ... Maybe an issue with the regexp implementation on PHP 7?
Oh boy. Committing to Zend Framework is hard work.
Oh boy. Committing to Zend Framework is hard work.
Nah, you just found an annoyingly hard edge case.
Did you check whether you are running with xdebug or not? Try running tests with php -n
(will probably fail on some curl-based scenarios).
If we can help out, let us know, we can certainly test on other environments as well.
PHP 7 has indeed changed many things in the regex engine, btw.
I've rebased and pushed back to your branch; let's see if the tests present the same issue...
Thanks, @domoron! We have updated the test suite recently, and those updates ensured your performance-based tests now pass reliably! Merged for next release.
In one of our legacy projects we download a ~200MB file in memory using chunked encoding. Calling getBody() on this (which calls decodeChunkedBody()) will take > 5 minutes for processing the response() since creates very inefficient copies of the body with each chunk (substr).
To make this more efficient we can use the offset parameter, to avoid copying the body during iteration.
Tests still passed.