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

Dev: Fixes runtime crash when slicing a large array so it becomes small (< 32 elements) #865

Merged
merged 2 commits into from May 12, 2017

Conversation

Projects
None yet
3 participants
@Skinney
Contributor

Skinney commented May 12, 2017

Includes test to reproduce issue.

The first problem was that depth calculation when slicing down to a one element array (slice 0 1 array) would return NaN, which is a setup for failure.

The second problem was the assumption that hoistTree always would be called with a tree with a size of > 1.

@process-bot

This comment has been minimized.

Show comment
Hide comment
@process-bot

process-bot May 12, 2017

Thanks for the pull request! Make sure it satisfies this checklist. My human colleagues will appreciate it!

Here is what to expect next, and if anyone wants to comment, keep these things in mind.

process-bot commented May 12, 2017

Thanks for the pull request! Make sure it satisfies this checklist. My human colleagues will appreciate it!

Here is what to expect next, and if anyone wants to comment, keep these things in mind.

@Skinney Skinney changed the title from Fixes runtime crash when slicing a large array so it becomes small (< 32 elements) to Dev: Fixes runtime crash when slicing a large array so it becomes small (< 32 elements) May 12, 2017

@Skinney

This comment has been minimized.

Show comment
Hide comment
@Skinney

Skinney May 12, 2017

Contributor

Hmm. That second commit was supposed to be a different PR. It's a harmless change though.

It doubles performance of _JsArray_push in Chrome and Safari (no change in Firefox). A similar change of the _JsArray_set function also had some performance increase in Chrome and Safari, but had the opposite effect (way worse performance) in Firefox.

Contributor

Skinney commented May 12, 2017

Hmm. That second commit was supposed to be a different PR. It's a harmless change though.

It doubles performance of _JsArray_push in Chrome and Safari (no change in Firefox). A similar change of the _JsArray_set function also had some performance increase in Chrome and Safari, but had the opposite effect (way worse performance) in Firefox.

@evancz evancz merged commit 779b5f5 into elm:dev May 12, 2017

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
@evancz

This comment has been minimized.

Show comment
Hide comment
@evancz

evancz May 12, 2017

Member

Thanks! :)

Member

evancz commented May 12, 2017

Thanks! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment