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

Convert recursive quicksort into iterative to avoid stack overflow #9457

Closed
wants to merge 1 commit into from

Conversation

vallendm
Copy link
Contributor

@vallendm vallendm commented Mar 24, 2020

Launch Checklist

We've recently switched to using the feature state property in our mapbox implementation. We found during one test - which includes rendering 30000+ points - a stack-overflow eminating from feature_position_map.js. This was in the sort function which is implemented as a custom quick sort using recursion. The issue is that there are certain circumstances where quicksort can have a worst-case scenario. The pivot position chosen can end up being the next sorted item in the list - whick leads to a recusion to sort the remaining N-1 items. If this happens continuously we can infact recurse through all items to be sorted which causes stack overflow issues. This change simply rewrites the sort to use a queue and an iterative approach.

  • [ x ] briefly describe the changes in this PR
  • include before/after visuals or gifs if this PR includes visual changes
  • write tests for all new functionality
  • document any changes to public APIs
  • post benchmark scores
  • manually test the debug page
  • tagged @mapbox/map-design-team @mapbox/static-apis if this PR includes style spec API or visual changes
  • tagged @mapbox/gl-native if this PR includes shader changes or needs a native port
  • apply changelog label ('bug', 'feature', 'docs', etc) or use the label 'skip changelog'
  • add an entry inside this element for inclusion in the mapbox-gl-js changelog: <changelog>Fix "Maximum call stack size exceeded" error when using feature-state.</changelog>

@ansis ansis added the bug 🐞 label Mar 24, 2020
Copy link
Contributor

@ansis ansis left a comment

Choose a reason for hiding this comment

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

Thanks for opening this! It looks good to me

@mourner do you want to take a glance at this before we merge?

@mourner
Copy link
Member

mourner commented Mar 25, 2020

Thanks a lot for the contribution @vallendm! I wasn't sure switching to manual queue was the best option, so I found an article about fixing this more idiomatically (and perhaps also faster / simpler) by manual tail call optimization — can you try #9463 and see if it fixes your issue?

@mourner
Copy link
Member

mourner commented Mar 30, 2020

Let's close this in favor of #9463 for now (but keep it in mind as an alternative).

@mourner mourner closed this Mar 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants