Skip to content

Update non-ST FFI to use Fn #235

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

Merged
merged 5 commits into from
Jun 13, 2023
Merged

Update non-ST FFI to use Fn #235

merged 5 commits into from
Jun 13, 2023

Conversation

JordanMartinez
Copy link
Contributor

Description of the change

Updates non-ST FFI to use uncurried functions via Fn types


Checklist:

  • Added the change to the changelog's "Unreleased" section with a reference to this PR (e.g. "- Made a change (#0000)")
  • Linked any existing issues or proposals that this pull request should close
  • Updated or added relevant documentation
  • Added a test for the contribution (if applicable)

@thomashoneyman
Copy link
Member

What is the motivation for this? Just to write more idiomatic JS in the FFI? Is this a change we're going to apply across core?

Co-authored-by: Thomas Honeyman <[email protected]>
@JordanMartinez
Copy link
Contributor Author

Here's my rationale:

  1. it produces stylistically cleaner JS output (no test needed)
  2. Those doing dev work (i.e. just doing purs compile w/ no optimizations via purs-backend-es) should get code that runs faster (test needed to confirm)
  3. once these functions use Fn* functions, purs-backend-es with no other changes should optimize more things (test needed to confirm)
  4. once these functions use Fn* functions, purs-backend-es can be further updated to know how to evaluate some of these things during its optimization pass (pretty sure this is true).

@gbagan gbagan mentioned this pull request May 9, 2023
4 tasks
@@ -18,6 +18,7 @@
"purescript-bifunctors": "^6.0.0",
"purescript-control": "^6.0.0",
"purescript-foldable-traversable": "^6.0.0",
"purescript-functions": "^6.0.0",
Copy link
Member

Choose a reason for hiding this comment

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

Not considering this a relevant dependency change because this was already brought in transitively (but good we now have it listed)

@thomashoneyman thomashoneyman merged commit eb750d4 into master Jun 13, 2023
@thomashoneyman thomashoneyman deleted the use-fns branch June 13, 2023 15:40
@natefaubion
Copy link

once these functions use Fn* functions, purs-backend-es with no other changes should optimize more things (test needed to confirm)
once these functions use Fn* functions, purs-backend-es can be further updated to know how to evaluate some of these things during its optimization pass (pretty sure this is true).

Just as an FYI for changes like this, changing FFI calling conventions will potentially break optimizations. purs-backend-optimizer cases the calling convention against unsafeIndexImpl (curried vs uncurried matters), so currently the point release here very likely does not optimize at all with backend-es.

@natefaubion
Copy link

That is, if it's important for the core team (I'm not saying it is) to preserve optimizations in tools like backend-optimizer, it's important that we actually do follow up and verify the impact of these changes.

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.

3 participants