benchmark: set NODE_ENV and stop dropping wrk errors on the floor - #367
Closed
nigrosimone wants to merge 2 commits into
Closed
benchmark: set NODE_ENV and stop dropping wrk errors on the floor#367nigrosimone wants to merge 2 commits into
nigrosimone wants to merge 2 commits into
Conversation
Three ways the harness could publish a number that does not mean what it says.
NODE_ENV is never set for the benchmark step. art-template keys `debug` off
process.env.NODE_ENV, and in debug mode it forces cache:false and re-reads and
recompiles the view from disk on every request, so engines/art spends most of
its time in the template compiler on both frameworks. The same variable also
decides express 4's view cache in defaultConfiguration(), which a later
app.set('env', 'production') does not revisit - so the harness setting only took
effect on uExpress and the row was asymmetric on top of being dominated by dead
work. Set on the step rather than the job so npm install still pulls
devDependencies.
A missing `-s` script does not make wrk fail. It falls back to GET / on the
target URL, which is how readable-hash-4mb published a 404 for 34 consecutive
runs at an implied 112 GB/sec of upload. Check the script exists before running.
wrk's error lines were parsed into `wrkErrors` and then only ever used in a
message that could not be reached: the sole gate was requestsPerSec === 0, and a
run answering 404 to every request still reports a perfectly healthy
Requests/sec. Non-2xx/3xx responses now fail the run.
Response validation had the same problem one level up - it ran, printed to
stderr and was stored on the row, but buildMarkdown never read it, so a scenario
where the two servers returned different bodies still rendered as a clean row.
It now marks the row and gets a section under the table, as do socket errors,
which are reported rather than failed since keep-alive teardown can produce a
few legitimately.
Contributor
Author
|
One prediction that did not materialise: I expected this might flip the row toward express, since in isolated measurements express was the faster renderer once out of debug mode. It did not the row moved slightly further in uExpress's favour. 39 timeouts on the express arm of routes-1000. Under the old harness that line was parsed into wrkErrors and then never reached any output, the row would have published 4.60k req/sec with nothing to indicate part of the run had timed out. Benchmark Comparison
Socket Errors
|
Five rows sit at 0.94x-1.15x and read as "the two frameworks are equivalent", when what they actually say is that the scenario spends its budget somewhere neither framework is involved. body-json-512kb is JSON.parse and utf8 decode of half a megabyte; the two streaming rows are per-byte copying of a 5 MiB response; readable-hash-4mb is OpenSSL sha256 over 4 MiB; compression-file is zlib, reached through the same middleware on both sides. Those ratios are capped by arithmetic - roughly 1.01x for the streaming rows and 1.02x for the JSON one - so no amount of work on either framework moves them. Scenarios can now declare what bounds them, and the table marks those rows and explains them underneath. Keeping them and labelling them is the point. Dropping them would make the table look curated, and they are real workloads even if they do not discriminate.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three ways the harness can publish a number that does not mean what the table says it means.
NODE_ENVis never setbenchmark.ymlhas noenv:block, so the benchmark runs withNODE_ENVunset. Two consequences:art-templatekeysdebugoffprocess.env.NODE_ENV !== 'production', and in debug mode it forcescache: falseand re-reads and recompiles the view from disk on every request, for both frameworks.engines/arttherefore spends most of its time in the template compiler rather than in anything either framework does.defaultConfiguration(), which a laterapp.set('env', 'production')does not revisit. uExpress honours the harness setting, express 4 does not, so express does anew View()and astatSyncper request and uExpress does not. That row was asymmetric on top of being dominated by dead work.Set on the step rather than the job, so
npm installstill pulls devDependencies.Worth knowing before reading the next
engines/artnumber: this changes the row substantially, and not necessarily in uExpress's favour.A missing wrk script does not fail
-spointing at a file that does not exist does not make wrk exit non-zero. It quietly falls back toGET /on the target URL. That is howstreaming/readable-hash-4mbmeasured a 404 for 34 consecutive published runs after the script was renamed at an implied 112 GB/sec of upload, which is the tell nobody was looking for. The script's existence is now checked before the run.wrk's error lines were parsed and then discarded
parseErrorLines()collectedSocket errors:andNon-2xx or 3xx responses:intowrkErrors, which was then only ever interpolated into a message behindif (requestsPerSec === 0). That gate cannot fire for the case it needed to catch: a run that answers 404 to every request still reports a perfectly healthyRequests/sec. Feeding a real wrk output withNon-2xx or 3xx responses: 1448400to the existing parser returnsrequestsPerSec = 72420and passes.Non-2xx/3xx responses now fail the run. Socket errors are reported rather than failed, since keep-alive teardown can produce a few legitimately.
Response validation was computed and never shown
validateScenarioResponses()already compares status and body hash between the two servers, logs to stderr and stores the result on the row andbuildMarkdownnever read it. A scenario where the two returned different bodies still rendered as a clean row with a bold speedup. It now marks the row with a warning and gets a section under the table.