-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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): do not cause stack overflow over circular refs #19802
Conversation
Thanks @cameron-martin for the inspiration! |
}) => { | ||
const updateValueDescriptor = ( | ||
{ nodeId, key, value, operation = `add` /*: add | del*/, descriptor = {} }, | ||
path = [] |
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.
other places use Set
for keeping track if already visited things, was there special reason to use array here?
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 benchmarked it, this seemed more efficient (although it could be variation). Perhaps because this is an explicit push/pop stack, where the other is arbitrary push.
This allows nodes to have a circular dependency without causing stack overflow exceptions when traversing the graph. Fixes #11364 (hopefully). Will have to confirm this after publishing an update. Causes somewhere between 0 and 1% perf regression for our 25k page benchmark site.
Rebased |
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.
Nicely done! Thanks @pvdz 🥇
…#19802) * fix(gatsby): do not cause stack overflow over circular refs This allows nodes to have a circular dependency without causing stack overflow exceptions when traversing the graph. Fixes gatsbyjs#11364 (hopefully). Will have to confirm this after publishing an update. Causes somewhere between 0 and 1% perf regression for our 25k page benchmark site. * Use actual `if` for conditionals * Only add data to the path when actually an object
This allows nodes to have a circular dependency without causing stack overflow exceptions when traversing the graph.
Fixes #11364 (hopefully). Will have to confirm this after publishing an update.
Causes somewhere between 0 and 1% perf regression for our 25k page benchmark site. But it's within variance of benchmarking, not even sure if there's a noticable regression at all. See this table:
The first two columns are on master (at the time of writing), the third and fourth column are after this change. (The
(govbook)
lines are printed from within the benchmark site). Shrug.