fix: additional caching for improved handler performance #1953

mattcosta7 · 2024-01-06T05:54:55Z

The intent of this PR is to help discover if there's a good general purpose cache api we can expose from the handlers, and if not we can see where caching improves performance further and how much.

These are mostly changes first looked at in #1905

This is a majority of the low-hanging request cycle optimizations I think we can do with good benefit and little overhead.
Further performance could be gained with a slightly modified storage structure, potentially (instead of solely linear request parsing, we could order them at registration into a more treelike structure, however this seems fairly complex compared to the low-hanging performance we can unlock from the small changes in this pr

This pull request introduces changes primarily aimed at improving the efficiency of URL and cookie parsing in the codebase. The changes involve the creation of a caching mechanism to store parsed URLs and cookies from requests, reducing the need for repeated parsing of the same data. The changes affect several files, with the most significant changes in src/core/handlers/GraphQLHandler.ts, src/core/handlers/HttpHandler.ts, and src/core/handlers/RequestHandler.ts.

URL and Cookie Parsing Improvements:

src/core/handlers/GraphQLHandler.ts: The matchRequestUrl function has been replaced with a new method matchRequestURLOrGetMatchFromCache that retrieves the match from the cache if it exists, otherwise it matches the request URL and stores the result in the cache. The getAllRequestCookies function has been replaced with a new method parseAllRequestCookiesOrGetFromCache that retrieves the cookies from the cache if they exist, otherwise it parses the request cookies and stores the result in the cache. [1] [2] [3] [4] [5]
src/core/handlers/HttpHandler.ts: Similar changes to GraphQLHandler.ts have been made, with the matchRequestUrl function replaced with matchRequestURLOrGetMatchFromCache and the getAllRequestCookies function replaced with parseAllRequestCookiesOrGetFromCache. [1] [2]
src/core/handlers/RequestHandler.ts: Added two new methods parseAllRequestCookiesOrGetFromCache and matchRequestURLOrGetMatchFromCache to handle the caching of parsed cookies and matched URLs. [1] [2]
src/core/utils/internal/parseGraphQLRequest.ts: Replaced the direct creation of a new URL object from the request URL with a call to the urlFromRequestOrCache function. [1] [2]
src/core/utils/logging/serializeRequest.ts: Replaced the direct creation of a new URL object from the request URL with a call to the urlFromRequestOrCache function. [1] [2]
src/core/utils/request/getPublicUrlFromRequest.ts: Replaced the direct creation of a new URL object from the request URL with a call to the urlFromRequestOrCache function. [1] [2]
src/core/utils/request/getRequestCookies.ts: Replaced the direct creation of a new URL object from the request URL with a call to the urlFromRequestOrCache function. [1] [2]
src/core/utils/request/urlFromRequestOrCache.ts: Added a new utility function urlFromRequestOrCache that retrieves a URL object from a cache if it exists, otherwise it creates a new URL object from the request URL and stores it in the cache.
src/core/utils/request/urlFromRequestOrCache.test.ts: Added new tests to verify the functionality of the urlFromRequestOrCache function.

mattcosta7 · 2024-01-06T05:56:31Z

src/core/handlers/RequestHandler.ts

+    Request,
+    Record<string, string>
+  >()
+  protected static parseAllRequestCookiesOrGetFromCache(


cookies can be parsed once per request, not required once per handler

mattcosta7 · 2024-01-06T05:57:49Z

src/core/handlers/RequestHandler.ts

+  }
+
+  private static matchRequestUrlCache = new Map<string, Match>()
+  protected static matchRequestURLOrGetMatchFromCache(


URL matching can be computed once per string comparison of URL/match/baseUrl - and reused for subsequent requests across all handlers. There's some memory cost to this map, but even at very large sizes it should be fine

mattcosta7 · 2024-01-06T05:58:52Z

src/core/utils/request/urlFromRequestOrCache.ts

+  const requestUrl = request.url
+  let url = urlCache.get(requestUrl)
+  if (!url) {
+    url = new URL(requestUrl)


URL allocations aren't cheap as they do expensive string parsing to create a URL object. we can cache this per string url, and avoid those object allocations and parses, which may occur multiple times for a single request lookup

Okay, I see. Thanks for the brief explanation!

They're also not super expensive, so definitely a micro-optimization we could totally skip

It's definitely the least important change. on the order of 10ms for 10,000 requests of overhead. doubtful that it'll matter to most!

I was mostly just trying to get a "what's left that is low hanging" branch together.

I'd say 10ms save on 10,000 requests is not worth the price we are paying here. Would you agree that we can skip this pull request as of now?

this was just the url part but I agree we don't need to ship this right now as they're generally small improvements. Very much just a "what other things can be optimized easily" experiment

src/core/handlers/GraphQLHandler.ts

mattcosta7 · 2024-01-06T13:47:35Z

tsconfig.base.json

@@ -1,7 +1,7 @@
 {
  "compilerOptions": {
    "strict": true,
-    "target": "es6",
+    "target": "es2020",


this is a perf fix, because async/await no longer become generators
see #1780

Can you please move this to a separate pull request?

Can you please move this to a separate pull request?

yep - it's part of 1780, so I think we can just let that land (it sets esnext, which i think should be ok?)

I believe so!

kettanaito · 2024-01-07T13:08:23Z

src/core/handlers/GraphQLHandler.ts

@@ -164,21 +165,27 @@ 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)
+    const match = this.matchRequestURLOrGetMatchFromCache(


Suggested change

const match = this.matchRequestURLOrGetMatchFromCache(

const match = this.matchRequestOrGetFromCache(

kettanaito · 2024-01-07T13:08:39Z

src/core/handlers/GraphQLHandler.ts

+      urlFromRequestOrCache(args.request),
+      this.endpoint,
+    )
+    const cookies = this.parseAllRequestCookiesOrGetFromCache(args.request)


Suggested change

const cookies = this.parseAllRequestCookiesOrGetFromCache(args.request)

const cookies = this.parseRequestCookiesOrGetFromCache(args.request)

kettanaito · 2024-01-07T13:09:35Z

src/core/handlers/RequestHandler.ts

@@ -184,6 +186,36 @@ export abstract class RequestHandler<
    })
  }

+  private static cookiesForRequestCache = new WeakMap<


Suggested change

private static cookiesForRequestCache = new WeakMap<

private static requestCookiesCache = new WeakMap<

kettanaito · 2024-01-07T13:09:43Z

src/core/handlers/RequestHandler.ts

+    Request,
+    Record<string, string>
+  >()
+  protected parseAllRequestCookiesOrGetFromCache(


Suggested change

protected parseAllRequestCookiesOrGetFromCache(

protected parseRequestCookiesOrGetFromCache(

kettanaito · 2024-01-07T13:10:09Z

src/core/handlers/RequestHandler.ts

+    return cookies
+  }
+
+  private static matchRequestUrlCache = new Map<string, Match>()


Why not WeakMap here also? Are we planning on freeing these resources manually?

WeakMap can't accept string keys, but mostly because this cache is actually more valuable if it persists between requests. Since multiple requests can have the same URL string, this maintains the cache across requests, avoiding the new URL overhead for multiple requests.

It's a slight extra optimization, but these shouldn't be so huge where they become an issue since it should be relatively small

We could later consider something more like an lru here if we need to evict things

Got it. The choice of Map is clear.

My main concern is that these changes wouldn't hurt the readability of the code. The benefit they provide is tangible but rather minimal. I'm fine with having them as long as the code doesn't become a jungle. I suggested a cached helper to make this common cached logic reused, I'd like to know what you think about it.

kettanaito · 2024-01-07T13:10:17Z

src/core/handlers/RequestHandler.ts

+  }
+
+  private static matchRequestUrlCache = new Map<string, Match>()
+  protected matchRequestURLOrGetMatchFromCache(


Suggested change

protected matchRequestURLOrGetMatchFromCache(

protected matchRequestOrGetFromCache(

kettanaito · 2024-01-07T13:10:54Z

src/core/utils/internal/parseGraphQLRequest.ts

@@ -92,7 +93,7 @@ function extractMultipartVariables<VariablesType extends GraphQLVariables>(
 async function getGraphQLInput(request: Request): Promise<GraphQLInput | null> {
  switch (request.method) {
    case 'GET': {
-      const url = new URL(request.url)
+      const url = urlFromRequestOrCache(request)


Are URL constructors really that costly? I suspect this optimization isn't worth the cost of readability and debugging.

kettanaito · 2024-01-07T13:11:08Z

src/core/utils/internal/requestHandlerUtils.ts

@@ -4,7 +4,10 @@ export function use(
  currentHandlers: Array<RequestHandler>,
  ...handlers: Array<RequestHandler>
 ): void {
-  currentHandlers.unshift(...handlers)
+  // we don't spread the handlers to avoid maximum stack errors on very large sets of handlers


Can we please add a test for this use case?

kettanaito · 2024-01-07T13:11:58Z

src/core/utils/internal/requestHandlerUtils.ts

@@ -4,7 +4,10 @@ export function use(
  currentHandlers: Array<RequestHandler>,
  ...handlers: Array<RequestHandler>
 ): void {
-  currentHandlers.unshift(...handlers)
+  // we don't spread the handlers to avoid maximum stack errors on very large sets of handlers
+  for (let i = handlers.length - 1; i >= 0; i--) {


I wonder if .reduceRight wouldn't be more readable here?

handlers.reduceRight((_, handler) => { currentHandlers.unshift(handler) })

kettanaito · 2024-01-07T13:12:24Z

src/core/utils/request/urlFromRequestOrCache.ts

@@ -0,0 +1,17 @@
+const urlCache = new Map<string, URL>()


Why not WeakMap<Request, URL>?

Mostly to keep URL objects between requests, likely a slight over optimization, which keeps subsequent requests slightly more efficient at the expense of memory.

Both should be generally helpful

kettanaito · 2024-01-07T13:18:41Z

src/core/utils/request/urlFromRequestOrCache.ts

+ * parsing the same url multiple times.
+ */
+export function urlFromRequestOrCache(request: Request): URL {
+  const requestUrl = request.url


We seem to be adopting this "look-in-cache-or-create" pattern quite extensively over these two pull requests. I think it may be worth exploring a tiny abstraction to make these actions more readable.

function cached<V, K extends object>(producer: () => V, key: K, cache: WeakMap<K, V>): V { if (!cache.has(key)) { cache.set(key, producer()) } return cache.get(key)! } // Usage. const urlCache = new WeakMap<Request, URL> export function cachedUrlFromRequest(request: Request) { return cached(() => new URL(request.url), request, urlCache) }

kettanaito · 2024-01-12T15:15:22Z

@mattcosta7, I still appreciate the effort on this. Thank you so much for making MSW better!

mattcosta7 added 2 commits January 6, 2024 00:43

fix: cache additional request parsing

452a0df

fix: cache additional request parsing

0bd44ca

mattcosta7 commented Jan 6, 2024

View reviewed changes

fix: cache additional request parsing

ed110b0

mattcosta7 commented Jan 6, 2024

View reviewed changes

src/core/handlers/GraphQLHandler.ts Show resolved Hide resolved

mattcosta7 changed the title ~~More caching for performance~~ fix: additional caching for improved handler performance Jan 6, 2024

mattcosta7 mentioned this pull request Jan 6, 2024

[experiment] chore: performance updates #1905

Closed

mattcosta7 marked this pull request as ready for review January 6, 2024 06:14

mattcosta7 self-assigned this Jan 6, 2024

fix: stack error on large use calls

4d4ce65

mattcosta7 commented Jan 6, 2024

View reviewed changes

kettanaito reviewed Jan 7, 2024

View reviewed changes

mattcosta7 closed this Jan 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: additional caching for improved handler performance #1953

fix: additional caching for improved handler performance #1953

mattcosta7 commented Jan 6, 2024 •

edited

Loading

mattcosta7 Jan 6, 2024

mattcosta7 Jan 6, 2024

mattcosta7 Jan 6, 2024

kettanaito Jan 7, 2024 •

edited

Loading

mattcosta7 Jan 8, 2024 •

edited

Loading

kettanaito Jan 12, 2024

mattcosta7 Jan 12, 2024

mattcosta7 Jan 6, 2024 •

edited

Loading

kettanaito Jan 7, 2024

mattcosta7 Jan 8, 2024

kettanaito Jan 12, 2024

kettanaito Jan 7, 2024 •

edited

Loading

kettanaito Jan 7, 2024

kettanaito Jan 7, 2024

kettanaito Jan 7, 2024

kettanaito Jan 7, 2024

mattcosta7 Jan 7, 2024

kettanaito Jan 7, 2024

kettanaito Jan 7, 2024

kettanaito Jan 7, 2024

kettanaito Jan 7, 2024

kettanaito Jan 7, 2024

kettanaito Jan 7, 2024

kettanaito Jan 7, 2024 •

edited

Loading

mattcosta7 Jan 7, 2024

kettanaito Jan 7, 2024

kettanaito commented Jan 12, 2024

	const match = this.matchRequestURLOrGetMatchFromCache(
	const match = this.matchRequestOrGetFromCache(

	const cookies = this.parseAllRequestCookiesOrGetFromCache(args.request)
	const cookies = this.parseRequestCookiesOrGetFromCache(args.request)

	private static cookiesForRequestCache = new WeakMap<
	private static requestCookiesCache = new WeakMap<

	protected parseAllRequestCookiesOrGetFromCache(
	protected parseRequestCookiesOrGetFromCache(

	protected matchRequestURLOrGetMatchFromCache(
	protected matchRequestOrGetFromCache(

fix: additional caching for improved handler performance #1953

fix: additional caching for improved handler performance #1953

Conversation

mattcosta7 commented Jan 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kettanaito Jan 7, 2024 • edited Loading

Choose a reason for hiding this comment

mattcosta7 Jan 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattcosta7 Jan 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kettanaito Jan 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kettanaito Jan 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kettanaito commented Jan 12, 2024

mattcosta7 commented Jan 6, 2024 •

edited

Loading

kettanaito Jan 7, 2024 •

edited

Loading

mattcosta7 Jan 8, 2024 •

edited

Loading

mattcosta7 Jan 6, 2024 •

edited

Loading

kettanaito Jan 7, 2024 •

edited

Loading

kettanaito Jan 7, 2024 •

edited

Loading