Skip to content

Conversation

natefaubion
Copy link
Contributor

Description of the change

This updates the timing FFI to not bind to hrtime directly, which is an odd API in general, instead taking the closure directly and returning the expected nanoseconds.

My primary motivation for doing this is that the existing framework does not work well with purescript-backend-optimizer, which aggressively eliminates unused code. The current pattern after inlining results in an unused call to f, which means nothing ever gets executed. By using an opaque FFI call, the demand on the closure is kept, and everything works as expected. Normally I wouldn't suggest doing this arbitrarily for this tool, but I think this is a better binding anyway, and is likely easier for other backends to implement since it doesn't rely on the specifics of a Node API.


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)

@natefaubion
Copy link
Contributor Author

I'm not sure what core's policy is on module private FFI implementations. Do we consider this a breaking change?

Copy link
Contributor

@JordanMartinez JordanMartinez left a comment

Choose a reason for hiding this comment

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

I don't think we have a clear policy on whether or not changing FFI counts as a breaking change. I think this is fine though.

@JordanMartinez
Copy link
Contributor

@thomashoneyman / @garyb Thoughts on this?

Copy link
Member

@garyb garyb left a comment

Choose a reason for hiding this comment

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

LGTM 👍 I'd be happy with this even if we were just doing it to make the interface better.

Seems I wasn't watching the library, so just got the notification with the ping.

@JordanMartinez
Copy link
Contributor

@natefaubion Mind adding a changelog entry?

@thomashoneyman
Copy link
Member

This is internal so I don’t consider it breaking.

@natefaubion
Copy link
Contributor Author

I've updated the changelog.

@thomashoneyman thomashoneyman merged commit f3fd0d7 into purescript:master Sep 23, 2022
@JordanMartinez
Copy link
Contributor

Can this get a release?

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.

4 participants