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 stack overflow when diffing large collections on background threads #66

Merged
merged 2 commits into from
Jan 5, 2020

Conversation

jenox
Copy link

@jenox jenox commented Dec 29, 2019

Fixes #63.
Fixes #44.

As I mentioned over there, the issue is a stack overflow in large data sets due to recursive implementations of various methods operating on linked lists. I am unsure as to why this wasn't an issue on the main thread, but this fix should be an improvement nonetheless.

The recursive methods could easily be converted to be iterative or removed altogether by conforming the linked lists to Sequence and using for in loops to iterate over them.

I'm not sure if we should include the large, previously-failing test case as it takes quite a few seconds to run. It needs to be large enough to cause the stack overflow though.

@jenox
Copy link
Author

jenox commented Dec 29, 2019

Hey @tonyarnold, most of the checks seem to fail with xcrun: error: missing DEVELOPER_DIR path: /Applications/Xcode_10.3.app/Contents/Developer — any idea why this is happening?

@tonyarnold
Copy link
Owner

It’s probably hardcoded into the CI workflows. Let me check that.

@tonyarnold
Copy link
Owner

Ok, rebase or merge the changes from master onto your branch, and you should be right to test.

@jenox
Copy link
Author

jenox commented Dec 30, 2019

Thanks for the quick fix Tony. Looking good now.

@tonyarnold
Copy link
Owner

Awesome - I’m away for a few days for New Years, and I won’t have a chance to look at this until I’m back. If you get time, could you run the performance tests linked in the read me against your changes? Otherwise, I’ll do it when I’m back.

I’m aiming to hook those up to CI early next year.

@jenox
Copy link
Author

jenox commented Jan 4, 2020

I ran the tests on my local machine and noticed a slight performance improvement for the diff case which went from averaging 0.13s to 0.125s on my machine. same, created, and deleted were too small to notice any difference at all.

I also had to make some minor adjustments to the performance suit because it didn't build out of the box with Swift 5, but I'll leave those to you if you want to hook it up to the CI anyway.

@tonyarnold
Copy link
Owner

Perfect - that's what I wanted to hear (that there was no large difference in performance on similar hardware).

I'm going to merge this now - the CI changes can wait until my kids aren't underfoot and I have time to actually focus on doing it correctly.

Copy link
Owner

@tonyarnold tonyarnold 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 this contribution - you've improved the ergonomics of the DoublyLinkedList type immensely 👍

@tonyarnold tonyarnold merged commit a858f9f into tonyarnold:master Jan 5, 2020
@jenox jenox deleted the bug/diff-on-background-thread branch June 5, 2020 23:21
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.

Does it work asynchronously? Stack Overflow with an array of 2000 rows
2 participants