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

Add simple builder #162

Closed
wants to merge 2 commits into from
Closed

Add simple builder #162

wants to merge 2 commits into from

Conversation

paluh
Copy link
Contributor

@paluh paluh commented Dec 22, 2019

I'm providing a simple builder implementation which was shortly discussed here: #115

@JordanMartinez
Copy link
Contributor

Any updates on this? Or would it be best to turn this into a library so others can use it sooner and then merge it into this project after its API gets fleshed out a bit more?

@garyb
Copy link
Member

garyb commented Jan 26, 2020

Maybe we should add a warning that this isn't suitable for building large arrays, since it's not stack safe? (The same could be said for the Record case, but is perhaps less likely there since records won't tend to have thousands of fields).

@JordanMartinez
Copy link
Contributor

Oh, good point.

@JordanMartinez
Copy link
Contributor

Couldn't this be made stack-safe if one used https://github.com/safareli/purescript-stacksafe-function ?

@paluh
Copy link
Contributor Author

paluh commented Oct 24, 2020

@JordanMartinez I think it should be stack safe then. Should I reimplement it? Isn't dependency on stacksafe-function a problem?

Copy link
Contributor

@hdgarrood hdgarrood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think stack safety is a requirement, but a dependency on stacksafe-function is not possible since it’s not in core. I’m not particularly keen on adding stacksafe-function to core either. We could potentially use a similar approach here.

I think this needs benchmarks too, because if it turns out this doesn’t perform well in realistic examples, then I don’t think we should add it.


import Prelude

newtype Builder a = Builder (Array a → Array a)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you use the ASCII versions of syntax like ->, ::, and forall please?

@JordanMartinez
Copy link
Contributor

I think stack safety is a requirement, but a dependency on stacksafe-function is not possible since it’s not in core. I’m not particularly keen on adding stacksafe-function to core either. We could potentially use a similar approach here.

Right, because it's not in core. If we do decide to support this here, how would we make the function stack-safe? If we don't support it here, then couldn't this code exist as a separate library that does depend on stacksafe-function? I would prefer it doesn't do that, but something worth discussing.

@natefaubion
Copy link

If you make it stack safe, it's not totally clear to me what the advantage of this is then. You are already allocating a closure for every cell, and if you make it stack safe you will have to suspend those closures in an additional data structure, which is either going to just be another Array, or a tree of some kind. If it's a tree, you'll have to consume it in a loop and make additional allocations for stack. That's a lot of overhead just to build an Array.

@hdgarrood
Copy link
Contributor

That’s the conclusion I was coming to as well. Maybe we just shouldn’t add this then? Can anyone provide a convincing example where this API is preferable to both the normal array API and the STArray API? If not I don’t think this approach earns a spot here.

@hdgarrood
Copy link
Contributor

Or do you think a non stack safe version still has something going for it despite the lack of stack safety? I just think that it could be quite surprising for this API to not be stack safe.

@natefaubion
Copy link

I don’t think we should add it without evidence that it’s a preferable alternative along some metric.

@natefaubion
Copy link

Or do you think a non stack safe version still has something going for it despite the lack of stack safety?

I just mean that the primary advantage appears to be something lightweight and pure. I could see adding an API on those merits. Accounting for stack-safety makes it anything but lightweight, however, both in implementation and overhead. If stack safety is critical (and I agree, for Array it probably is), then we shouldn't add it without evidence that it still satisfies a desirable space over ST, pure Array operations, or List/CatList conversions.

@JordanMartinez
Copy link
Contributor

I'd imagine that the desire for a Builder API for Array is partly because it requires less learning to use.

Regardless, should we close this issue?

@hdgarrood
Copy link
Contributor

I'm not sure I agree that it requires less learning, but yeah, I think we can close this PR (and the associated issue). Thank you anyway @paluh!

@hdgarrood hdgarrood closed this Dec 23, 2020
@hdgarrood hdgarrood mentioned this pull request Dec 23, 2020
@paluh
Copy link
Contributor Author

paluh commented Dec 23, 2020

Thanks for all the reviews and discussion.

P.S. Maybe I release something small like overflowing-array-builder instead ;-)

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.

None yet

5 participants