purescript-concur / purescript-concur-react

Concur UI Framework for Purescript
https://purescript-concur.github.io/purescript-concur-react
MIT License
271 stars 17 forks source link

Stack Overflow Mapping over Array #54

Open fros1y opened 4 years ago

fros1y commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

fros1y commented 4 years ago

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 commented 4 years ago

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.

ajbarber commented 3 years ago

@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?