perf: eliminate conditional check in json strict mode hot path#651
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the JSON parser logic by extracting the parse function into a separate helper function createJsonParser. The refactoring improves code organization by separating parser creation from middleware setup, and fixes an incorrect JSDoc return type annotation.
Key Changes
- Extracted inline
parsefunction into a newcreateJsonParserhelper function that returns the appropriate parser based on thestrictoption - Corrected JSDoc return type annotation for
firstcharfunction from{function}to{string | undefined}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Ref: expressjs/perf-wg#77 A (Baseline):
B (This branch):
My local load test results: cc: @wesleytodd |
|
Ah awesome, yeah the perf tests are still a work in progress. So there is inconsistencies between runs and sometimes you may want to change the test requests or server implementation a bit to get real results. I just threw together some low effort requests with bodies in that PR but feel free to write ones that show the perf improvement even more than this did on your machine. And any feedback on running these is welcome! |
Refactor JSON parser creation to return specialized parser functions based on strict mode setting, removing the per-request `if (strict)` conditional check in the parsing hot path.
c7dda3b to
6e28122
Compare
|
Hey @UlisesGascon @jonchurch 👋 |
|
Hey @UlisesGascon @jonchurch. A review would be appreciated |
|
@UlisesGascon @jonchurch I plan to merge this next week. Maybe you have some time to check this out. |
jonchurch
left a comment
There was a problem hiding this comment.
willing to give it a shot, if the duplication is still bothering you later we can land this where we extract out the biggest chunk that doesnt change per parser (make a parseBody function):
function parseBody (body, reviver) {
try {
debug('parse json')
return JSON.parse(body, reviver)
} catch (e) {
throw normalizeJsonSyntaxError(e, {
message: e.message,
stack: e.stack
})
}
}
/**
* Create a JSON parse function
*
* @param {object} [options]
* @return {function}
* @private
*/
function createJsonParser (options) {
const reviver = options?.reviver
const strict = options?.strict !== false
if (strict) {
return function parseStrict (body) {
if (body.length === 0) {
// special-case empty json body, as it's a common client-side mistake
// TODO: maybe make this configurable or part of "strict" option
return {}
}
const first = firstchar(body)
if (first !== '{' && first !== '[') {
debug('strict violation')
throw createStrictSyntaxError(body, first)
}
return parseBody(body, reviver)
}
}
return function parse (body) {
if (body.length === 0) {
// special-case empty json body, as it's a common client-side mistake
// TODO: maybe make this configurable or part of "strict" option
return {}
}
return parseBody(body, reviver)
}
}
Let's maybe add this in a follow up PR. |
This change refactors the JSON parser to eliminate the per-request
if (strict)conditional check from the parsing hot path.Instead of checking strict mode during parsing, we now create specialized parser functions at middleware initialization based on the strict option.
I'm aware that this introduces some code duplication between the strict and non-strict parser implementations, but I think that is a necessary trade-off.