-
-
Notifications
You must be signed in to change notification settings - Fork 517
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[experiment] chore: performance updates #1905
Conversation
@@ -59,7 +59,9 @@ export abstract class SetupApi<EventsMap extends EventMap> extends Disposable { | |||
), | |||
) | |||
|
|||
this.currentHandlers.unshift(...runtimeHandlers) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested a case of applying a milion handlers, which failed due to the stack size, since spread places all of the array items onto the stack
@@ -138,7 +139,7 @@ export class GraphQLHandler extends RequestHandler< | |||
* If the request doesn't match a specified endpoint, there's no | |||
* need to parse it since there's no case where we would handle this | |||
*/ | |||
const match = matchRequestUrl(new URL(args.request.url), this.endpoint) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
URL parsing is kind of expensive, and we can memoize this easily
* We don't want to copy this for _every_ handler, as it | ||
* is expensive to do so. | ||
*/ | ||
const mainRequestRef = (() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ugly for now, but cloning on every handler is actually quite expensive (one of the most expensive operations, attributing to the growth of both event listeners (for aborts), memory and time
/** | ||
* Determines if a given request can be considered a GraphQL request. | ||
* Does not parse the query and does not guarantee its validity. | ||
*/ | ||
export async function parseGraphQLRequest( | ||
request: Request, | ||
): Promise<ParsedGraphQLRequest> { | ||
if (cache.has(request)) return cache.get(request) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
graphql parsing is shared across all graphql handlers, and onUnhandledRequest
, so doing this once per requst is a great optimization
/** | ||
* Returns the result of matching given request URL against a mask. | ||
*/ | ||
export function matchRequestUrl(url: URL, path: Path, baseUrl?: string): Match { | ||
const key = `${url}|${path}|${baseUrl}` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
request matching is expensive, but for any url/path/base triplet, the result is always identical, so we can eat this cost once and cache it
This cache works across handlers, since handlers can fallthrough to the same urls
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also would want to use a better key than this, since we'll strip url params/hashes/etc before mathching. Maybe we can extract some logic here and only do this 'url preparation' once per request vs per attempt to match
@@ -0,0 +1,9 @@ | |||
const cache = new Map<string, URL>() | |||
|
|||
export function memoizedUrl(url: string, base?: string): URL { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new URL is costly, but we have the same urls frequently since every handler calls this
We could probably generate this before executing handlers, and pass it around instead of caching it (my preference)
@@ -37,7 +38,9 @@ export function getRequestCookies(request: Request): Record<string, string> { | |||
} | |||
} | |||
|
|||
const cache = new WeakMap<Request, Record<string, string>>() | |||
export function getAllRequestCookies(request: Request): Record<string, string> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reading cookies isn't that expensive but it's idempotent to a single request, so we should be good to cache this
What areas would be next for improvements? Currently potential handlers are evaluated linearly based on when they were defined. This puts a minimum cost bottleneck that scales with the number of handlers (and the relative location in those handlers of the one that finally matches). What does this mean? the first active handler is always going to resolve almost immediately. The last handler is always going to resolve in at minimum Can we re-structure this to be more efficient? Express and similar tools utilize trie structures to handle matching. A This would be a signficiant improvement in some cases, and likely a minimal cost in others, especially since the evaluation cost should be lower than just path extraction. We'll have some slightly interesting considerations here to still maintain the |
Expanding from: #1900
I was interested in trying to optimize the
handleRequest
pipeline without making substantial changes. I hope this can help inform some future updates suggested in #1901 and other related areas.I don't expect this branch to land directly at all, but solely to be used as discovery
Takeaways
We can make significant improvements to performance and scalability with a few relatively small code changes.
requestUrl = new URL(request.url) and
mainRequestRef = request.clone()passing them directly to each handler. We may not even need a
mainRequestRefat all, as it doesn't seem used anywhere directly. We might not need to memoize
new URL` if we extract that outside of the handlers, but we'd still have this cost where we might not need it as frequentlymatchRequestUrl
lazily. Every request has to hit all paths, but once that occurs we'll be able to precompute the matches later, so we should be ok to re-use those after initial matching, giving a good cache keyparseGraphQLRequest
results across handlers for a given request. Should we also cache the actual query parse here, which may be the same for multiple variables, where that input might currently not be identical - we should verify this (I did not try that yet)getAllRequestCookies
- this is actually cheap compared to everything else discussed aboveTest structure
Before: MSW v2.0.9 (with the
unshift
fix from this branch)After: this branch
Results are described as a single run, and not a many run average, but they are very stable results so they're indicative of larger test windows
Using my m1 macbook pro, latest chrome. 2 tabs open (this issue and a vite sample project - code below)
100,000 handlers configured.
Fresh page load:
http
request to the first handler.http
request to the last handler.http
request to the last handlerFresh page load:
graphql
request to the first handler.graphql
request to the last handler.graphql
request to the last handlerBefore
Http
1st handler: 5ms
Last handler (1x): 10.42s
Last handler (2x): 10.97s
Graphql
1st handler: 6ms
Last handler (1x): 13.20s
Last handler (2x): 13.15s
After
Http
1st handler: 5ms
Last handler (1x): 2.41s
Last handler (2x): 818ms
Graphql
1st handler: 6ms
Last handler (1x): 1.83s
Last handler (2x): 1.81s
Findings
1st handler results stay about identical.
Last handler results improve drastically
Obviously these results are extreme because of the number of handlers, but even for more moderate handler numbers these results are very promising
When a smaller, 50 handler set is used (raw numbers only)
Before:
After:
Description of changes
Setup changes, equal across tests
src/core/SetupApi.ts
andsrc/core/utils/internal/requestHandlerUtils.ts
: Instead if calling unshift with a spread, loop through the handlers in reverse and unshift them individually. This avoids maximum stack issues when a large number of handlers are 'use'd. Spreading forces the arguments all onto the stack, which is why it overflows. [1] [2]tsconfig.base.json
: Updated the TypeScript target version from ES6 to ES2020. These made debugging/testing simpler, since we don't create generators for promise resolutionPerformance improvements:
src/core/utils/memoizedUrl.ts
: Introduced amemoizedUrl
function to create and cache URL objects, reducing the overhead of repeatedly creating new URL objects.src/core/handlers/RequestHandler.ts
: Reduced unnecessary request cloning by introducing amainRefCache
to store a reference to the original request. We probably don't need to clone this at all, but minimizing changes to call signatures meant we needed to provide a clone to the execution result for the one handler that matches. We don't seem to read this, so maybe we don't need it. If we do can we do this prior to starting the resolution pipeline? [1] [2] [3]src/core/utils/internal/parseGraphQLRequest.ts
: Added a caching mechanism to theparseGraphQLRequest
function to store and reuse the parsed result of a GraphQL request. [1] [2] [3]src/core/utils/matching/matchRequestUrl.ts
: Introduced a cache in thematchRequestUrl
function to store and reuse the result of URL-path matching. [1] [2]src/core/utils/request/getRequestCookies.ts
: Added a cache to thegetAllRequestCookies
function to store and reuse the parsed cookies from a request. [1] [2]Code used in test setup: