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

Improve performance of toJS (#1453) #1581

Closed

Conversation

lukaswelinder
Copy link

This PR addresses #1453, significantly improving performance of toJS method; more details can be found in my comment on that issue.

It might be worth adding a performance regression test for the toJS method.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@facebook-github-bot
Copy link

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@facebook-github-bot
Copy link

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

leebyron added a commit that referenced this pull request Sep 18, 2018
Fixes #1453

commit b600bd7c56da9f71d19a035838523610d4c96c00
Author: Lee Byron <lee.byron@robinhood.com>
Date:   Tue Sep 18 12:54:40 2018 -0700

    Further improvements

commit 9c6395b4baba8a79b3b364d68a4909a26ce7b51c
Merge: ca85e75 26ecbab
Author: Lee Byron <lee.byron@robinhood.com>
Date:   Tue Sep 18 12:28:49 2018 -0700

    Merge branch 'toJS-performance' of https://github.com/lukaswelinder/immutable-js into lukaswelinder-toJS-performance

commit 26ecbab
Author: Lukas Welinder <lukas@welinder.co>
Date:   Fri Aug 24 20:28:00 2018 -0700

    Resolve issue w/ toJS handling structures that should become arrays

commit 1c56169
Author: Lukas Welinder <lukas@welinder.co>
Date:   Fri Aug 24 20:11:42 2018 -0700

    Resolve issue w/ toJS handling structures that should become arrays

commit 0b44fad
Author: Lukas Welinder <lukas@welinder.co>
Date:   Fri Aug 24 20:01:39 2018 -0700

    Revert "Adjust type check approach for toJS performance fix"

    This reverts commit 0ca7f5d

commit 0ca7f5d
Author: Lukas Welinder <lukas@welinder.co>
Date:   Fri Aug 24 19:57:19 2018 -0700

    Adjust type check approach for toJS performance fix

commit 727bb23
Author: Lukas Welinder <lukas@welinder.co>
Date:   Fri Aug 24 19:38:01 2018 -0700

    Improve performance of toJS (#1453)
@leebyron
Copy link
Collaborator

Nice! I was able to speed things up a bit further by avoiding the Seq cast for collections and using the internal __iterate method instead of forEach - squash merge in 0082ff7

@leebyron leebyron closed this Sep 18, 2018
leebyron added a commit that referenced this pull request Sep 18, 2018
Fixes #1453

commit b600bd7c56da9f71d19a035838523610d4c96c00
Author: Lee Byron <lee.byron@robinhood.com>
Date:   Tue Sep 18 12:54:40 2018 -0700

    Further improvements

commit 9c6395b4baba8a79b3b364d68a4909a26ce7b51c
Merge: ca85e75 26ecbab
Author: Lee Byron <lee.byron@robinhood.com>
Date:   Tue Sep 18 12:28:49 2018 -0700

    Merge branch 'toJS-performance' of https://github.com/lukaswelinder/immutable-js into lukaswelinder-toJS-performance

commit 26ecbab
Author: Lukas Welinder <lukas@welinder.co>
Date:   Fri Aug 24 20:28:00 2018 -0700

    Resolve issue w/ toJS handling structures that should become arrays

commit 1c56169
Author: Lukas Welinder <lukas@welinder.co>
Date:   Fri Aug 24 20:11:42 2018 -0700

    Resolve issue w/ toJS handling structures that should become arrays

commit 0b44fad
Author: Lukas Welinder <lukas@welinder.co>
Date:   Fri Aug 24 20:01:39 2018 -0700

    Revert "Adjust type check approach for toJS performance fix"

    This reverts commit 0ca7f5d

commit 0ca7f5d
Author: Lukas Welinder <lukas@welinder.co>
Date:   Fri Aug 24 19:57:19 2018 -0700

    Adjust type check approach for toJS performance fix

commit 727bb23
Author: Lukas Welinder <lukas@welinder.co>
Date:   Fri Aug 24 19:38:01 2018 -0700

    Improve performance of toJS (#1453)
@lukaswelinder
Copy link
Author

lukaswelinder commented Sep 21, 2018

@leebyron Awesome! Nice work; over 60% faster than it was even before the issue cropped up in 4.0.0-rc.8 (using the same test linked in #1453).

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

Successfully merging this pull request may close these issues.

None yet

3 participants