Rewrite Control.Effect.Interpret in terms of reflection #193

ocharles · 2019-07-17T08:53:22Z

ocharles · 2019-07-17T09:32:46Z

Something seems up with the Cabal configuration, but if I build the benchmarks with ghc --make -O2 -isrc benchmarks/Bench.hs, here are the results:

After

benchmarking InterpretC vs InterpretStateC vs StateC/InterpretC/100
time                 71.27 ns   (71.13 ns .. 71.45 ns)
                     1.000 R²   (1.000 R² .. 1.000 R²)
mean                 71.53 ns   (71.33 ns .. 71.99 ns)
std dev              959.5 ps   (450.8 ps .. 1.699 ns)
variance introduced by outliers: 15% (moderately inflated)
benchmarking InterpretC vs InterpretStateC vs StateC/InterpretC/1000

time                 577.8 ns   (576.3 ns .. 580.1 ns)

1.000 R²   (1.000 R² .. 1.000 R²)

mean                 579.7 ns   (578.1 ns .. 582.2 ns)

std dev              6.607 ns   (4.240 ns .. 10.98 ns)
benchmarking InterpretC vs InterpretStateC vs StateC/InterpretC/10000

time                 5.746 μs   (5.682 μs .. 5.835 μs)

0.999 R²   (0.998 R² .. 1.000 R²)

mean                 5.701 μs   (5.675 μs .. 5.738 μs)

std dev              109.2 ns   (79.61 ns .. 170.5 ns)

variance introduced by outliers: 19% (moderately inflated)
benchmarking InterpretC vs InterpretStateC vs StateC/InterpretStateC/100

time                 74.61 ns   (74.35 ns .. 74.92 ns)

1.000 R²   (1.000 R² .. 1.000 R²)

mean                 74.66 ns   (74.47 ns .. 74.88 ns)

std dev              691.4 ps   (571.7 ps .. 851.1 ps)
benchmarking InterpretC vs InterpretStateC vs StateC/InterpretStateC/1000

time                 608.7 ns   (607.6 ns .. 609.9 ns)

1.000 R²   (1.000 R² .. 1.000 R²)

mean                 608.8 ns   (607.6 ns .. 610.1 ns)

std dev              4.215 ns   (3.207 ns .. 6.545 ns)
benchmarking InterpretC vs InterpretStateC vs StateC/InterpretStateC/10000

time                 6.036 μs   (6.021 μs .. 6.052 μs)

1.000 R²   (1.000 R² .. 1.000 R²)

mean                 6.039 μs   (6.022 μs .. 6.056 μs)

std dev              52.99 ns   (42.80 ns .. 66.86 ns)
benchmarking InterpretC vs InterpretStateC vs StateC/StateC/100

time                 71.33 ns   (71.18 ns .. 71.49 ns)

1.000 R²   (1.000 R² .. 1.000 R²)

mean                 71.41 ns   (71.22 ns .. 71.61 ns)

std dev              649.5 ps   (523.3 ps .. 847.2 ps)
benchmarking InterpretC vs InterpretStateC vs StateC/StateC/1000

time                 577.2 ns   (576.2 ns .. 578.3 ns)

1.000 R²   (1.000 R² .. 1.000 R²)

mean                 577.5 ns   (575.9 ns .. 579.7 ns)

std dev              6.372 ns   (3.857 ns .. 9.426 ns)
benchmarking InterpretC vs InterpretStateC vs StateC/StateC/10000

time                 5.668 μs   (5.646 μs .. 5.695 μs)

1.000 R²   (1.000 R² .. 1.000 R²)

mean                 5.673 μs   (5.655 μs .. 5.690 μs)

std dev              60.56 ns   (51.11 ns .. 73.21 ns)

Before

benchmarking InterpretC vs InterpretStateC vs StateC/InterpretC/100
time                 6.719 μs   (6.569 μs .. 6.860 μs)
                     0.997 R²   (0.995 R² .. 0.999 R²)
mean                 6.630 μs   (6.566 μs .. 6.727 μs)
std dev              265.0 ns   (192.1 ns .. 401.7 ns)
variance introduced by outliers: 50% (severely inflated)
benchmarking InterpretC vs InterpretStateC vs StateC/InterpretC/1000

time                 66.46 μs   (65.92 μs .. 67.23 μs)

0.999 R²   (0.999 R² .. 1.000 R²)

mean                 67.59 μs   (67.15 μs .. 68.07 μs)

std dev              1.582 μs   (1.312 μs .. 2.130 μs)

variance introduced by outliers: 20% (moderately inflated)
benchmarking InterpretC vs InterpretStateC vs StateC/InterpretC/10000

time                 672.0 μs   (659.5 μs .. 682.1 μs)

0.998 R²   (0.997 R² .. 0.999 R²)

mean                 655.6 μs   (650.3 μs .. 662.6 μs)

std dev              20.91 μs   (17.36 μs .. 26.26 μs)

variance introduced by outliers: 23% (moderately inflated)
benchmarking InterpretC vs InterpretStateC vs StateC/InterpretStateC/100

time                 5.727 μs   (5.648 μs .. 5.816 μs)

0.998 R²   (0.998 R² .. 0.999 R²)

mean                 5.679 μs   (5.637 μs .. 5.738 μs)

std dev              168.3 ns   (133.5 ns .. 212.0 ns)

variance introduced by outliers: 36% (moderately inflated)
benchmarking InterpretC vs InterpretStateC vs StateC/InterpretStateC/1000

time                 56.31 μs   (55.91 μs .. 56.74 μs)

0.999 R²   (0.999 R² .. 1.000 R²)

mean                 57.03 μs   (56.40 μs .. 58.14 μs)

std dev              2.697 μs   (1.627 μs .. 4.228 μs)

variance introduced by outliers: 52% (severely inflated)
benchmarking InterpretC vs InterpretStateC vs StateC/InterpretStateC/10000

time                 567.6 μs   (553.0 μs .. 587.0 μs)

0.994 R²   (0.985 R² .. 1.000 R²)

mean                 562.1 μs   (557.4 μs .. 573.7 μs)

std dev              24.28 μs   (10.58 μs .. 46.72 μs)

variance introduced by outliers: 36% (moderately inflated)
benchmarking InterpretC vs InterpretStateC vs StateC/StateC/100

time                 66.37 ns   (66.24 ns .. 66.53 ns)

1.000 R²   (0.999 R² .. 1.000 R²)

mean                 66.31 ns   (66.09 ns .. 66.59 ns)

std dev              872.5 ps   (621.5 ps .. 1.312 ns)

variance introduced by outliers: 14% (moderately inflated)
benchmarking InterpretC vs InterpretStateC vs StateC/StateC/1000

time                 581.8 ns   (579.0 ns .. 585.7 ns)

1.000 R²   (1.000 R² .. 1.000 R²)

mean                 586.0 ns   (583.5 ns .. 588.7 ns)

std dev              8.648 ns   (7.394 ns .. 10.33 ns)

variance introduced by outliers: 15% (moderately inflated)
benchmarking InterpretC vs InterpretStateC vs StateC/StateC/10000

time                 5.701 μs   (5.682 μs .. 5.730 μs)

1.000 R²   (1.000 R² .. 1.000 R²)

mean                 5.726 μs   (5.703 μs .. 5.762 μs)

std dev              95.20 ns   (62.63 ns .. 146.8 ns)

variance introduced by outliers: 15% (moderately inflated)

The important result is here:

With this PR:

benchmarking InterpretC vs InterpretStateC vs StateC/InterpretC/10000
time                 5.746 μs

benchmarking InterpretC vs InterpretStateC vs StateC/InterpretStateC/10000
time                 6.036 μs

benchmarking InterpretC vs InterpretStateC vs StateC/StateC/10000
time                 5.668 μs

Before this PR:

benchmarking InterpretC vs InterpretStateC vs StateC/InterpretC/10000
time                 672.0 μs

benchmarking InterpretC vs InterpretStateC vs StateC/InterpretStateC/10000
time                 567.6 μs

benchmarking InterpretC vs InterpretStateC vs StateC/StateC/10000
time                 5.701 μs

I can't reproduce this using cabal new-bench though, something must be going on with inlining somehow when building separate components.

robrix

@ocharles: Thank you so much for submitting this PR! This is superb work, and has held up very nicely in my explorations this morning. I’m excited to see what more we can do with it!

Regarding the benchmarks, I’ll take a look at it today/tomorrow and see if I can help figure that out.

I’ve left a bunch of comments, mostly stylistic nitpicks but also a couple of things about what we’re going to need to export.

Note to self: this is going to be a major version bump due to the removal of InterpretStateC and the changed signatures.

benchmark/Bench.hs

src/Control/Effect/Interpret.hs

robrix · 2019-07-17T16:28:39Z

src/Control/Effect/Interpret.hs

+  eff ( L eff ) =
+    runHandler ( unTag ( reflect @s ) ) eff
+  eff ( R other ) =
+    InterpretC ( eff ( handleCoercible other ) )


I think we’re going to want an INLINE pragma on this (might be relevant to the benchmarking stuff, I’m not sure).

I haven't seen a need for it yet. Should I add it or leave it until benchmarking shows it necessary? Note the benchmarks in my above comment didn't have it, unless you can't reproduce them.

Cool, let’s not then!

ocharles · 2019-07-17T18:52:24Z

Thanks for the extensive feedback! I admit I just dumped it into a file and forgot about it, I wasn't quite intending it to be in a mergeable state, but more as a conversation point. But I guess there's not much more to do! Let me blast through all the stylistic stuff, and we'll see what we're left with.

ocharles · 2019-07-17T20:31:03Z

I found the Cabal oddity - your cabal.project specifies -O0, which is overriding anything I put in the Cabal file. Maybe we drop this?

ocharles · 2019-07-18T07:20:30Z

Ok, I think I've addressed all your review points. Bring on round two!

robrix

Fantastic work @ocharles!

ocharles added 2 commits July 17, 2019 09:52

Rewrite Control.Effect.Interpret in terms of reflection

5de23e6

See fused-effects#191.

Fix benchmarks

b22f606

robrix suggested changes Jul 17, 2019

View reviewed changes

ocharles added 12 commits July 17, 2019 19:57

Style

22fe9d9

More style

358a52d

unInterpretC -> runInterpretC

b24ec40

Remove kind signature

756167e

Tidy deriving

ecffb53

Single line

ba99a1a

Spaces in foralls

1f0172b

Drop a kind signature

3c21d7b

Move InterpretC declaration down

64d9ccf

Export Reifies

9b9c4df

Export Handler

b60b9bc

Remove outdated comments

9faa0dc

Finally review tweaks

b1893ee

robrix approved these changes Jul 21, 2019

View reviewed changes

robrix added this to the 0.6 milestone Jul 21, 2019

robrix merged commit feeb354 into fused-effects:master Jul 21, 2019

robrix mentioned this pull request Jul 21, 2019

Brainstorming an approach using reflection #191

Closed

robrix mentioned this pull request Oct 7, 2019

Document the effects and their carriers. #251

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite Control.Effect.Interpret in terms of reflection #193

Rewrite Control.Effect.Interpret in terms of reflection #193

ocharles commented Jul 17, 2019

ocharles commented Jul 17, 2019 •

edited

Loading

robrix left a comment

robrix Jul 17, 2019

ocharles Jul 17, 2019

robrix Jul 18, 2019

ocharles commented Jul 17, 2019

ocharles commented Jul 17, 2019

ocharles commented Jul 18, 2019

robrix left a comment

Rewrite Control.Effect.Interpret in terms of reflection #193

Rewrite Control.Effect.Interpret in terms of reflection #193

Conversation

ocharles commented Jul 17, 2019

ocharles commented Jul 17, 2019 • edited Loading

robrix left a comment

Choose a reason for hiding this comment

robrix Jul 17, 2019

Choose a reason for hiding this comment

ocharles Jul 17, 2019

Choose a reason for hiding this comment

robrix Jul 18, 2019

Choose a reason for hiding this comment

ocharles commented Jul 17, 2019

ocharles commented Jul 17, 2019

ocharles commented Jul 18, 2019

robrix left a comment

Choose a reason for hiding this comment

ocharles commented Jul 17, 2019 •

edited

Loading