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 max call stack error on bad input #105

Merged
merged 5 commits into from
Jun 15, 2018
Merged

Conversation

mourner
Copy link
Member

@mourner mourner commented Jun 12, 2018

Closes #104 by eliminating recursion in the simplify routine.

@mourner
Copy link
Member Author

mourner commented Jun 12, 2018

This seems to severely hit simplification performance, so looking now whether we could optimize it...

@mourner
Copy link
Member Author

mourner commented Jun 12, 2018

I couldn't make non-recursive version fast enough (it was 27% slower), so I pushed an alternative fix which should hypothetically work, and fixes the sample above, but I'm not entirely sure, so would appreciate a review.

Basically the issue here is on a certain degenerate input, recursive simplification can go too deep because the "pivot" (the most distance point in the Douglas-Peucker simplification step) will be close to the left point all the time, so on every recursive call, the amount of points remaining for simplification will only be reduced by a small constant number. So the alternative fix I come up with is, if there are many pivot candidates equally distant from the current segment, we choose the one with index closest to the middle. That way if the input is degenerate and there are many pivot candidates, we'll split the recursion equally so that the call stack size is closer to log(n) than n.

@anandthakker could you take a look? Let me know if the above doesn't make sense and I'll try to clarify, it's a tricky issue.

@mourner mourner requested a review from anandthakker June 12, 2018 16:08
Copy link
Contributor

@anandthakker anandthakker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a good solution to me!

I'd be in favor of inlining a bit more explanation of the workaround into the code comment, maybe: "a workaround to ensure we choose a pivot close to the middle of the list, reducing recursion depth, for certain degenerate inputs"

@mourner
Copy link
Member Author

mourner commented Jun 15, 2018

@anandthakker good point, updated the comment.

@mourner mourner merged commit 0134ff4 into master Jun 15, 2018
@mourner mourner deleted the fix-max-call-stack-error branch June 15, 2018 13:05
@mourner
Copy link
Member Author

mourner commented Jun 18, 2018

In theory, this fix doesn't need to be ported to C++ because it doesn't have such harsh max call stack limit as JS, which varies depending on platform but is pretty low everywhere, e.g. ~10k on Chrome. But I'm not sure — @kkaefer thoughts?

@kkaefer
Copy link
Member

kkaefer commented Jun 20, 2018

Let's port this to C++ too; 10k stack calls aren't something we'd really want on the C++ side either.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants