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

Stack Overflow Mapping over Array #54

Open
fros1y opened this issue Aug 29, 2020 · 5 comments
Open

Stack Overflow Mapping over Array #54

fros1y opened this issue Aug 29, 2020 · 5 comments
Labels

Comments

@fros1y
Copy link

fros1y commented Aug 29, 2020

Hello,

I have a reproducible stack overflow error triggered by just mapping a D.text element over a sufficiently large array (n > 9000). This is a synthetic example derived from a real example that was mapping over two dimensions with more reasonable values. While the N is higher, the error seems to be the same.

This reproduces for n > 9000, at least in my Chrome. Tested on TryPurescript to avoid any potential local issues with same results.

testCase :: forall a. Int -> Widget HTML a
testCase n = D.div' $ (\x -> D.text $ (show x) <> "\t") <$> (A.range 0 n) 

Full gist here:

https://gist.github.com/fros1y/09bf2efabe49d880f0a9c022f25d2706

To the extent it is relevant, I'm using a Linux build of Chrome, Version 84.0.4147.135 (Official Build) (64-bit)

Please let me know if there is more data I can provide!

@fros1y fros1y changed the title Stack Overflow in Mapping Over Large-ish nested Data.Maps Stack Overflow Mapping over Array Aug 29, 2020
@fros1y
Copy link
Author

fros1y commented Aug 29, 2020

Also worth pointing out, I just tried the official SlowButton implementation and it has the same failure mode for higher numbers. That example implies that the difference between SlowButton and FastButton is performance, but it appears to be correctness as well. Am I running into a fundamental limitation? (i.e., do I need to figure out how to drop down into mkLeafNode?)

Thanks!

@ajnsit
Copy link
Member

ajnsit commented Aug 29, 2020

Thanks for opening the issue. This is definitely a bug and not a fundamental limitation. I'll look into it.

@ajnsit ajnsit self-assigned this Aug 29, 2020
@ajnsit ajnsit added the bug label Aug 29, 2020
@fros1y
Copy link
Author

fros1y commented Aug 29, 2020

Thanks for looking into this. There's a thread on discourse also with some thoughts: https://discourse.purescript.org/t/advice-on-debugging-stack-size-problems-concur-react-related/1676/4

@fros1y
Copy link
Author

fros1y commented Aug 30, 2020

So it looks like there are multiple places that may not be stack safe. Based on the feedback on the Discourse thread above, above, I looked at the call to createElement.apply inside of purescript-react. I made a branch (https://github.com/fros1y/purescript-react/tree/remove-apply) of purescript-react that gets rid of the .apply, at the cost of a bunch of warnings about unkeyed components from the React framework.

That gets farther!

But I then get a a stack fault apparently based on a call to foldlWithIndex inside of the widgetMultiAlternative instance here: https://github.com/purescript-concur/purescript-concur-core/blob/7017365e11384024177615b579c0229b6905d274/src/Concur/Core/Types.purs#L146

That's a pretty gnarly function, so still trying to get a good mental model for it.

@ajnsit ajnsit added the hacktoberfest hacktoberfest label Sep 30, 2020
@ajnsit ajnsit removed their assignment Sep 30, 2020
@ajnsit ajnsit removed the hacktoberfest hacktoberfest label Oct 12, 2020
@ajbarber
Copy link
Contributor

@fros1y Can you can try locally overriding purescript-concur-core with this branch, and let me know if this resolves your issue in your application?

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 a pull request may close this issue.

3 participants