-
Notifications
You must be signed in to change notification settings - Fork 270
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
Inline Foreign Func calls in interpreter (i.e. Haskell builtins) #5339
Conversation
exec !_env !denv !_activeThreads !ustk !bstk !k _ (ForeignCall _ (FF _ref arg res ev) args) = | ||
uncurry (denv,,,k) | ||
<$> (arg ustk bstk args >>= ev >>= res ustk bstk) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's the actual foreign embedding
Am I reading it right that these timings are largely the same and maybe even a bit slower? |
@ceedubs Yup pretty much, it's not much of a noticeable improvement. The ones that are slower by a couple microseconds may be within variance, things like counting to a million are like 16ms faster which is at least noticeable. Whether it's actually worth merging this is up for debate |
FWIW, we have been running this in staging all day. I feel like my data on the improvement is all kinda within the margin or error but it's certainly not slower. We've been running all the nimbus integration tests against this and its all been good. |
@stew Given that this kinda makes the code uglier Dan and I figure if it's not a noticeable improvement we'll probably just skip this one. |
Code's a bit ugly, still needs refactoring
Choose your PR title well: Your pull request title is what's used to create release notes, so please make it descriptive of the change itself, which may be different from the initial motivation to make the change.
Overview
What does this change accomplish and why?
i.e. How does it change the user experience?
i.e. What was the old behavior/API and what is the new behavior/API?
Feel free to include "before and after" examples if appropriate. (You can copy/paste screenshots directly into this editor.)
If relevant, which Github issues does it close? (See closing-issues-using-keywords.)
Implementation notes
How does it accomplish it, in broad strokes? i.e. How does it change the Haskell codebase?
Interesting/controversial decisions
Include anything that you thought twice about, debated, chose arbitrarily, etc.
What could have been done differently, but wasn't? And why?
Test coverage
Loose ends
Link to related issues that address things you didn't get to. Stuff you encountered on the way and decided not to include in this PR.