Skip to content

Conversation

@pmarsh-scottlogic
Copy link
Contributor

@pmarsh-scottlogic pmarsh-scottlogic commented May 3, 2023

Resolves #6101

Changes:
The previous implementation of the max() and min() methods used Function.prototype.apply(), which would have a stack error on arrays of length more than ~120000. On the back of a discussion under the issue, I benchmarked a few alternative implementations and we settled with a simple for loop, which is one of the fastest if not the fastest implementation across all array sizes.

PR Checklist

  • npm run lint passes
  • [Inline documentation] is included / updated
  • [Unit tests] are included / updated

no doc or unit test changes given that the functionality has not really been changed

…ses stack error, replaced with simple for loop
@welcome
Copy link

welcome bot commented May 3, 2023

🎉 Thanks for opening this pull request! Please check out our contributing guidelines if you haven't already. And be sure to add yourself to the list of contributors on the readme page!

@pmarsh-scottlogic pmarsh-scottlogic changed the title change implementation of min and max. No longer using apply which cau… fix min() and max() crashing on large arrays May 3, 2023
@pmarsh-scottlogic
Copy link
Contributor Author

@all-contributors please add @pmarsh-scottlogic for THING(S)

@allcontributors
Copy link
Contributor

@pmarsh-scottlogic

I couldn't determine any contributions to add, did you specify any contributions?
Please make sure to use valid contribution names.

@pmarsh-scottlogic
Copy link
Contributor Author

@Qianqianye Qianqianye requested a review from limzykenneth May 5, 2023 02:38
@Qianqianye
Copy link
Contributor

@all-contributors please add @pmarsh-scottlogic for code

@allcontributors
Copy link
Contributor

@Qianqianye

I've put up a pull request to add @pmarsh-scottlogic! 🎉

@pmarsh-scottlogic pmarsh-scottlogic mentioned this pull request May 5, 2023
17 tasks
@limzykenneth limzykenneth merged commit 7e3d771 into processing:main May 5, 2023
@limzykenneth
Copy link
Member

Looks good. Thanks!

@davepagurek
Copy link
Contributor

Nice work @pmarsh-scottlogic!

@Qianqianye
Copy link
Contributor

Thanks @pmarsh-scottlogic!

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.

max() crashes on long arrays

4 participants