-
-
Notifications
You must be signed in to change notification settings - Fork 729
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 parsing & compacting very deep objects #224
Conversation
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.
Overall this looks great - adding a test, no changes to existing tests, no personal reactions besides unimportant style nits.
Just the one comment, otherwise LGTM!
lib/utils.js
Outdated
return exports.compactQueue(queue); | ||
}; | ||
|
||
exports.compactQueue = function (queue) { |
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.
does this need to be publicly exported? It'd be nice not to increase the public API.
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.
It doesn't, but didn't see the utils
object being exported out through the index
entry point. I wasn't sure if adding to exports
was just the style or not (most of it was changed to the Hapi project style when it lived there for a bit and I just wasn't super familiar).
TL;DR thought that it would fit in that way, but I'll in-export the function 👍
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.
It's certainly consistent :-) but I'm in a slow yet determined march to move away from the hapi style, and minimize the publicly reachable API of qs
, so the change is appreciated!
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.
haha, making the change is no problem at all :) I try to copy the existing code style as much as possible to blend it in, so wasn't sure, hehe
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.
LGTM, thanks
Looks like your new test is only failing in Turbofan-enabled nodes - 8.3+. |
Yep, just saw that and taking a look :) |
Ok, for whatever reason Turbofan was just failing to collapse in the same function Crankshaft was doing in the actual parsing. So now parsing itself is iterative instead of recursive. Probably even a better win overall now for everyone. |
Released in v6.5.1 |
This is a change of the internal
compact
utility to use an iterative approach instead of recursion in order to better handle very, very deep objects without a stack overflow. It was noticed on CharkaCore because it has a much smaller maximum stack than v8 does, though it can still happen on v8 (which the test does)./cc @kunalspathak