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

ffi array type interop with opaque types #64

Open
joprice opened this issue Oct 4, 2020 · 7 comments
Open

ffi array type interop with opaque types #64

joprice opened this issue Oct 4, 2020 · 7 comments
Labels
Golang Applies to the Go target question

Comments

@joprice
Copy link

joprice commented Oct 4, 2020

I was trying to return an array from a function and use it with common Foldable and Traversable functions, and hit a wall of silent failures. Luckily, since the build doesn't detect changes in the output directory, I was able to throw some printlns in there and rebuild without it being overwritten. I found that casts in functions like Data.Array.length like

 xs, _ := xs_.([]Any)

return an empty array when the cast to interface fails.

To fix this, the type cannot simply be assigned to interface:

cannot use files (variable of type []os.FileInfo) as []interface{} value in variable declaration

so it needs to be copied, as this go wiki page points out:

https://github.com/golang/go/wiki/InterfaceSlice#what-can-i-do-instead

I'm wondering whether there's any way to get around this. Maybe define Traversable instances for an opaque type? I haven't tried this and not sure how much boilerplate it would require, but if there is some boilerplate on the go ffi side, maybe it could be provided in the small runtime library where Fn1-10 reside.

Below is the ffi function I ended up with that copies the array:

exports["readDir'"] = func(left Any) Any {
      return func(right Any) Any {
        return func(file_ Any) Any {
          file := file_.(string)
          return func() Any {
            files, err := ioutil.ReadDir(file)
            if err == nil {
              var f []interface{} = make([]interface{}, len(files))
              for i, d := range files {
                f[i] = d
              }
              return Apply(right, f)
            } else {
              return Apply(left, err.Error())
            }
          }
        }
      }
    }
@andyarvanitis
Copy link
Owner

When doing a type assertion (cast) in go, if you don't provide a (discarded) failure bool variable it will panic (with a description) if it fails. In other words, you would do this:

 xs := xs_.([]Any)

There's no current way to do it from spago, but if you do the go build using the debug tag you will more information:

$ spago build
$ go build -tags debug output/Main/Main.go
$ ./Main

(you could also just do go run -tags...)

Let me know if that helps with your debugging.

@andyarvanitis andyarvanitis added Golang Applies to the Go target question labels Oct 4, 2020
@joprice
Copy link
Author

joprice commented Oct 4, 2020

I see a lot of casts in the ffi definitions using the two-result value variant. There's a note in the readme on removing this once you're confident of the types. Is this something that should be cleaned up, and is there some overhead?

@andyarvanitis
Copy link
Owner

The idea was to end up with the (allegedly faster) two-result (ignored failure) assertions in ffi implementations. In my readme I intended to convey that an author should use the debug-friendly (one-result) version while testing. I say "allegedly faster" because TBH I don't know if in recent versions of golang the non-panic assertions are still faster.

Providing debug and release versions of the files, like I do in the runtime, would have been nice, but I thought it would be too onerous for contributors.

@joprice
Copy link
Author

joprice commented Oct 5, 2020

Ah I misunderstood. I probably read it too fast while frantically debugging.

Writing and maintaining two files is definitely error prone. I guess you could provide a few casting helper functions that are generated in debug or release mode based on a flag and the ffi could target those. Or there could be a preprocessor step. Also, couldn't the runtime be generated, since everything is built from source anyway? Is there a benefit to have it as a separate library?

@andyarvanitis
Copy link
Owner

Well, if I end up getting a definitive answer that the panics don't cause any overhead I would just get rid of the two-result versions everywhere.

I'd like to avoid any kind of preprocessor and keep things simple and fast-building.

Benefit to separate runtime library is simply cleaner and easier maintenance. It's not set in stone, though.

@joprice
Copy link
Author

joprice commented Dec 12, 2020

It looks like you've gone with using the checked versions andyarvanitis/purescript-native-go-ffi@8512b65, so that answers the silent failure part of my question.

The other part was whether boxing arrays into interface{} is avoidable, and if not whether the helper library should have a helper function to make that easier for writers of ffi code.

Having to re-allocate arrays to box them is not great in my opinion, even if it is only a constant factor difference, since it immediately adds overhead to using purescript-native over writing the code in go. This can lead people to push more logic into the ffi for performance reasons, getting less use our of purescript's abstractions.

@andyarvanitis
Copy link
Owner

I don't think there's a way around arrays needing to be of type []Any, short of using opaque types in places, as you mention. A helper function for boxing arrays would indeed be nice, but go doesn't have generics.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Golang Applies to the Go target question
Projects
None yet
Development

No branches or pull requests

2 participants