benchmark and deep merge optimization #307
Conversation
There was a problem hiding this comment.
This report is a fantastic start.
Things I'd love to expand on:
- Generalizing the benchmarking process enough that we could write a suite of tests for other functionality rather than just competing middleware prototype implementations
- Looking at the Node.js ecosystem to see if there are preexisting tools we can use to make that easier; no new dependencies doesn't mean no new devDependencies!
- Running that generalized benchmarking tool in GitHub Actions or Buildkite so that we could compare results from Linux, Mac and Windows as well as different Node.js versions
- Generating more granular stats (like tracking heap over time, as I mentioned in a comment)
- Outputting all collected stats as JSON so we could track changes over time
I don't think you need to do all of that in this PR, unless it seems like a simple improvement. We can iterate toward that using this as a starting place.
As for the optimized deep merge: there are probably other similar strategies we could play with if we want to take the time. I know some libraries like Immutable have done a ton of work in this space that we could draw from.
The problem, of course, is avoiding new dependencies. Doing it ourselves might make the code more verbose and harder to maintain, so we'll have to decide where to draw the line. Perf optimization is addictive, but we also have to be realistic about avoiding micro-optimization. 😆
| - Gzip compression: ~240ms (90%) | ||
| - Middleware overhead: ~20-25ms (10%) | ||
|
|
||
| **Memory improvement**: 94% less heap vs baseline (0.03MB vs 0.49MB) |
There was a problem hiding this comment.
This is so cool! Do you have any theories about why heap usage went down so dramatically?
I'd love to explore this more to see if we can pinpoint an exact cause to help guide the deep merge optimization work, as well as potentially guiding other future perf improvements.
| const endMem = process.memoryUsage() | ||
|
|
||
| const durationMs = Number(endTime - startTime) / 1_000_000 | ||
| const heapDiff = endMem.heapUsed - startMem.heapUsed |
There was a problem hiding this comment.
It might also be useful to be tracking the changes in heapUsed every x milliseconds while the iterations are running. If graphing that looks like a GC sawtooth we know CPU usage could be a concern.
| initialContext: MiddlewareContext | ||
| ): Promise<{ context: MiddlewareContext, error?: Error }> { | ||
| let currentContext = initialContext | ||
| const syncPhase = (phase + 'Sync') as keyof Middleware |
There was a problem hiding this comment.
I think it would feel more natural to detect async by its return value rather than using a naming scheme to determine this.
| function deepMergeOptimized (current: MiddlewareContext, updates: NonNullable<MiddlewareResult['context']>): MiddlewareContext { | ||
| if (updates.request == null && updates.shared == null) { | ||
| return current | ||
| } | ||
|
|
||
| let mergedRequest = current.request | ||
| if (updates.request != null) { | ||
| const mergedHeaders = updates.request.headers != null | ||
| ? { ...current.request.headers, ...updates.request.headers } | ||
| : current.request.headers | ||
|
|
||
| mergedRequest = { | ||
| ...current.request, | ||
| ...updates.request, | ||
| headers: mergedHeaders | ||
| } | ||
| } | ||
|
|
||
| return { | ||
| ...current, | ||
| request: mergedRequest, | ||
| shared: updates.shared ?? current.shared | ||
| } | ||
| } |
There was a problem hiding this comment.
This is a great first step toward optimization. 👍
|
A couple links I had saved that I forgot to share: |
2 participants
Uh oh!
There was an error while loading. Please reload this page.