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

fix(gatsby): granular redux cache heuristics for finding large nodes #23643

Closed
wants to merge 2 commits into from

Conversation

pvdz
Copy link
Contributor

@pvdz pvdz commented Apr 30, 2020

When persisting the redux store, there is a heuristic that will try to roughly determine how big nodes are because there are intrinsic limits to the amount of data we can store in one pass (2gb). This heuristic is currently taking 11 random nodes from the entire pool and using the biggest one to determine the chunk size.

This PR improves the granularity of this check to do this per node type, because certain nodes are intrinsically smaller than others so it hopefully yields more relevant information. I suspect that, ultimately, we could skip all of them and just check the Page type, but we can determine that later.

One downside is that this means sites with many types, like Contentful sites, will probe 11*x nodes now. I don't expect this to be a huge problem, but something noteworthy regardless.

Fixes #23627

This fix will not work with Loki, since that doesn't create the by-type map. Since I'm planning to remove that in the next two weeks I'm probably going to wait with actually getting this PR merged until that happens.

@pvdz pvdz marked this pull request as ready for review May 11, 2020 10:38
@pvdz pvdz requested a review from a team as a code owner May 11, 2020 10:38
@freiksenet
Copy link
Contributor

we could skip all of them and just check the Page type

I suspect that too, tbh. Generally the plan is to start warning about big Page types because it usually means big context and that is bad for runtime performance.

freiksenet
freiksenet previously approved these changes May 11, 2020
@pvdz
Copy link
Contributor Author

pvdz commented May 11, 2020

I fixed a bug, added a test. I think we can't merge this because of the perf implication :'(

The problem is that the map by type does not have arrays but it has another map. This means we can't have direct access to the nodes and we either need to create a temporary array for this, or loop through all nodes manually, either way a performance penalty.

I think we'll need to find another way.

@pvdz pvdz marked this pull request as draft May 11, 2020 14:03
@pvdz pvdz added scaling topic: performance Related to runtime & build performance labels May 11, 2020
@pvdz
Copy link
Contributor Author

pvdz commented Nov 12, 2020

Just not worth it currently so I'm going to close this.

@pvdz pvdz closed this Nov 12, 2020
@LekoArts LekoArts deleted the redux-cache-type branch February 15, 2021 10:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: performance Related to runtime & build performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Node buffer allocation crash
2 participants