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

Inline Foreign Func calls in interpreter (i.e. Haskell builtins) #5339

Closed
wants to merge 11 commits into from

Conversation

ChrisPenner
Copy link
Contributor

@ChrisPenner ChrisPenner commented Sep 10, 2024

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

-- inlined foreign

Decode Nat
718ns

Generate 100 random numbers
458.95µs

Count to 1 million
435.8618ms

Json parsing (per document)
359.359µs

Count to N (per element)
543ns

Count to 1000
524.014µs

Mutate a Ref 1000 times
984.583µs

CAS an IO.ref 1000 times
1.231184ms

List.range (per element)
643ns

List.range 0 1000
652.162µs

Set.fromList (range 0 1000)
3.000585ms

Map.fromList (range 0 1000)
2.318429ms

Map.lookup (1k element map)
5.789µs

Map.insert (1k element map)
14.877µs

List.at (1k element list)
563ns

Text.split /
42.094µs

--------------------------------------------------------------------------------
trunk (with func inlining)

Decode Nat
740ns

Generate 100 random numbers
464.28µs

Count to 1 million
451.7112ms

Json parsing (per document)
364.226µs

Count to N (per element)
523ns

Count to 1000
521.775µs

Mutate a Ref 1000 times
970.351µs

CAS an IO.ref 1000 times
1.256695ms

List.range (per element)
639ns

List.range 0 1000
652.142µs

Set.fromList (range 0 1000)
3.060119ms

Map.fromList (range 0 1000)
2.388843ms

Map.lookup (1k element map)
5.755µs

Map.insert (1k element map)
14.54µs

List.at (1k element list)
571ns

Text.split /
40.268µs

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.

Comment on lines +531 to +533
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)
Copy link
Contributor Author

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

@ceedubs
Copy link
Contributor

ceedubs commented Sep 11, 2024

Am I reading it right that these timings are largely the same and maybe even a bit slower?

@ChrisPenner
Copy link
Contributor Author

@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

@stew
Copy link
Member

stew commented Sep 12, 2024

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.

@ChrisPenner
Copy link
Contributor Author

@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.

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