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

ChrisPenner commented 2 months ago

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.

ceedubs commented 2 months ago

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

ChrisPenner commented 2 months ago

@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 commented 2 months ago

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 commented 2 months ago

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

unisonweb / unison