Skip to content
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

feat: Make the maximum recycleable parser cache size configurable #1156

Closed
jfroundjian opened this issue Sep 9, 2024 · 6 comments
Closed
Assignees
Labels
enhancement New feature or request internally-reviewed The issue has been reviewed internally.

Comments

@jfroundjian
Copy link
Contributor

jfroundjian commented Sep 9, 2024

Component(s)

router

Is your feature request related to a problem? Please describe.

Problem:

One of our federated queries is for loading a batch of data >1500 items all with >100 fields. This often results in larger JSON responses that the Resolver ends up parsing. After switching to ParserPools in fastjson (sometime after Router .88.0) we noticed unbounded memory growth when performing a relatively large number of these batch requests (> 150 min) often resulting in unwieldy heap sizes and a struggling GC. This was not a problem for smaller requests (same Query but with <200 items).

It turns out that the issue is for our given queries, the ParserPool ends up retaining Parser objects which have allocated arrays of 2^17 (131,072) astjson.Value objects. This makes the Router retain an unusually large Heap that can not be reduce by the GC because the Parsers are being actively used to resolve GraphQL responses.

I've attached a few pprof dumps from v 0.88.0 and v 0.107.0
v88.pb.gz
v107.pb.gz

Describe the solution you'd like

I'm proposing and have implemented a somewhat simple solution to the problem to discard Parser objects that have unusually large caches so that the GC can eventually release that memory back to the Program.

Additionally, I propose making this a configurable property of the Router and Graphql-go-tools so that the default behavior will continue as is and this value can be set on a per need basis.

I've already tested this locally and it works well for our use case. Here are the 3 PRs I'm proposing:

  1. astjson: Add a PutIfSizeLessThan method to ParserPool
    feat: Add configurable size limit to recycled Parsers in pool astjson#2

  2. graphql-go-tools: Add ResolverOption for MaxRecyclableParserSize and use on Resolvable.Reset()
    feat: Add configurable size limit for parsers in pool graphql-go-tools#881

  3. router: Add ResolverMaxRecyclableParserSize configuration and pass to Resolver on creation
    feat: make the maximum recycleable parser cache size configurable #1157

Describe alternatives you've considered

No response

Additional context

No response

@jfroundjian jfroundjian added the enhancement New feature or request label Sep 9, 2024
@jfroundjian jfroundjian changed the title Make the maximum recycleable parser cache size configurable feat: Make the maximum recycleable parser cache size configurable Sep 9, 2024
Copy link

github-actions bot commented Sep 9, 2024

WunderGraph commits fully to Open Source and we want to make sure that we can help you as fast as possible.
The roadmap is driven by our customers and we have to prioritize issues that are important to them.
You can influence the priority by becoming a customer. Please contact us here.

@jensneuse
Copy link
Member

@jfroundjian thanks for raising this, it sounds very reasonable.
I mostly like your proposal, although I'd suggest that we set a default max value to automatically cleanup the heap based on a "reasonable" number of cache objects.

Do you have any suggestion on what this number could be?
We could determine an approximation in terms of memory usage using reflect and limit the storage to some amount, e.g. 100MiB. I'm not sure what's a good way to configure this as a random number like "100k" will not have a lot of meaning to the users. What are your thoughts?

@jfroundjian
Copy link
Contributor Author

jfroundjian commented Sep 10, 2024

Thanks @jensneuse setting a default is definitely a good idea I just didn't want to suggest it immediately because it would have some performance implications for those really large requests (which is probably a rare use case).

I agree that the number won't have a lot of meaning to folks since it's just a cap on the number of json objects that are parsed in an individual response but it is also difficult to estimate the size since it depends a lot on the JSON being parsed.

From my example our subgraph responses can get pretty large so I'll try to do some basic maths here to work backwards:

  • 1500 items with 180 fields and some nested objects ~ 12MB uncompressed.
  • This will usually mean that the Parser object allocates a byte array for the string (12MB) and then an array of Values for every parsed field and subfield which in this case equate to about 270,000 fields. In practice a lot of these are null and I've seen the Value array end up at around 2^17 values
  • For our particular use case this translated to about 100MB allocated per parser as Value objects store references to nested Values and other nested Objects increasing the amount of heap space required for every individual Value.

I think 32,768 or 2^15 is a very reasonable limit that almost all graphql responses will never get to but if you want to be more accomodating going up one more also works. Wdyt? Btw I am going up in powers of 2 because that's how the Parser grows the underlying array to store these values.

@jensneuse
Copy link
Member

I think your suggestion of 2^15 sounds reasonable.

Another case to think about is that we could implement a more sophisticated parser pool.
If the parser pool would know the cap of each parser in the pool, we can determine the most appropriate parser in the pool.
E.g. we could estimate a parser of the right size so that we don't have to increase the cap again during parsing.
In addition, we could have more control over releasing a parser.
E.g. if we run a dedicated goroutine for cleanups of the pool, we wouldn't have to "push" parsers to the GC based on size, as the goroutine could track a few seconds if a parser is not needed anymore and automatically discard it.

That said, I'm fine going with a simple approach first and seeing where it takes us.

jensneuse pushed a commit to wundergraph/graphql-go-tools that referenced this issue Sep 11, 2024
@jfroundjian
Copy link
Contributor Author

That sounds pretty nice @jensneuse but going simple first is also a deliberate decision I made because afaict our use case is unique.
In the future, I'd be happy to contribute a more elaborate scheme for assigning parsers and cleaning up.

Btw I also came across this PR that was never merged into fastjson to add a reusability limit for a parser. That's more appropriate for the use case where we occasionally parse large jsons but it's also doing something similar
valyala/fastjson#45

@jfroundjian
Copy link
Contributor Author

Thanks for helping me merge all the PRs and update the documentation @jensneuse. I can confirm that the feature is working as expected in [email protected] and that our high memory usage has been curbed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request internally-reviewed The issue has been reviewed internally.
Projects
None yet
Development

No branches or pull requests

3 participants