perf(middleware-stack): optimize cloneTo, cache middleware list, fix .reverse() mutation#1883
Open
TrevorBurnham wants to merge 1 commit intosmithy-lang:mainfrom
Open
Conversation
….reverse() mutation - Add _addBulk internal fast path for cloneTo to skip validation on empty stacks, with fallback to add()/addRelativeTo() for duplicate/override handling on non-empty targets - Cache getMiddlewareList result, invalidate on any mutation, return shallow copy - Replace map+reduce with flatMap in getMiddlewareList - Replace .reverse() calls with reverse iteration in expandRelativeMiddlewareList and resolve() to avoid array mutation and intermediate allocations - Add comprehensive tests for bulk add, cache invalidation, and expand stability
Contributor
|
what is the performance comparison of control, this PR, and |
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
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.
Problem
Every
client.send()call rebuilds the middleware stack from scratch —concat()clones all entries viacloneTo, thenresolve()callsgetMiddlewareList()which normalizes, sorts, and builds a closure chain.Additionally,
expandRelativeMiddlewareListcalls.reverse()on theafterarray, which mutates it in place. IfgetMiddlewareListis ever called twice on the same stack (e.g. viaidentify()thenresolve()), the mutation corrupts the ordering on the second call.Changes
1.
_addBulkfast path incloneTocloneTocopies entries from one stack to a new (typically empty) stack. Previously it calledtoStack.add()for each entry, which runs duplicate-name checks, override logic,getAllAliases()allocations, and object spread reconstruction — all unnecessary when the target is empty.The new
_addBulkinternal method skips validation for entries with no name conflicts and falls back to the fulladd()/addRelativeTo()path when a duplicate is detected, preserving override semantics for non-empty targets (i.e.applyToStackon a populated stack).Cross-version compatibility is maintained:
cloneTochecks for_addBulkon the target and falls back to the public API if absent.2. Cache
getMiddlewareListresultThe sorted/expanded middleware list is now cached after the first
resolve()call. The cache is invalidated on any mutation (add,addRelativeTo,remove,removeByName,removeByReference,removeByTag,_addBulk). A shallow copy is returned from the cache to prevent external consumers from corrupting it.3.
flatMapinstead ofmap+reduceReplaces the
sort().map(expand).reduce(push)chain withsort().flatMap(expand): single pass, no intermediate arrays, equivalent semantics.4. Reverse iteration instead of
.reverse()expandRelativeMiddlewareList: replacesfrom.after.reverse().forEach(...)with a reverseforloop. This fixes a latent bug where.reverse()mutates theafterarray, corrupting results on repeated calls togetMiddlewareList.resolve(): replaces.map(e => e.middleware).reverse()with a reverseforloop, avoiding two intermediate array allocations.