Move evaluation of binary operations to step_bin_op

[DarkDrek]

Implement short circuiting for && and ||
Solves #4 and is part of #9

I added a helper function[1] to the test that stops the new test after 1 sec. So i someone breaks the short-circuiting the tests do not run endless.

Some big performance regression so please verify it locally before merge 😄 I don't trust my old laptops performance

arithmetic              time:   [1.2384 us 1.2401 us 1.2420 us]                        
                        change: [+27.376% +27.712% +28.030%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 12 outliers among 100 measurements (12.00%)
  1 (1.00%) low mild
  7 (7.00%) high mild
  4 (4.00%) high severe

fact_rec                time:   [279.13 us 280.23 us 281.51 us]                     
                        change: [+14.877% +15.253% +15.631%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 12 outliers among 100 measurements (12.00%)
  3 (3.00%) high mild
  9 (9.00%) high severe

fact_tail               time:   [117.14 us 117.29 us 117.44 us]                      
                        change: [+1.6303% +1.9006% +2.1893%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 12 outliers among 100 measurements (12.00%)
  1 (1.00%) low mild
  4 (4.00%) high mild
  7 (7.00%) high severe

fancy_max               time:   [4.0958 us 4.1659 us 4.2710 us]                       
                        change: [+0.8552% +1.9608% +3.1966%] (p = 0.00 < 0.05)
                        Change within noise threshold.
Found 10 outliers among 100 measurements (10.00%)
  5 (5.00%) high mild
  5 (5.00%) high severe

step                    time:   [1.5533 us 1.5620 us 1.5742 us]                  
                        change: [-2.7167% -1.0125% +1.1629%] (p = 0.31 > 0.05)
                        No change in performance detected.
Found 14 outliers among 100 measurements (14.00%)
  14 (14.00%) high severe

Edit: I now saw that the issue is assigned to @seanchen1991 I hope you don't mind me trying to do it 😅

[seanchen1991]

No worries! Good learning opportunity for me 🙂

[seanchen1991]

The panic_after function is cool; I like the idea of it for this use-case.

Never seen the @ symbol before. Looks like it binds a pattern to a name. So in step_bin_op you're binding box Lit(Bool(...)) to t so you can return the pattern. Cool stuff 🙂

[DarkDrek]

You are right it is rather arcane/uncommon https://doc.rust-lang.org/book/ch18-03-pattern-syntax.html#-bindings but nonetheless a cool feature.

[pvdrz]

Some big performance regression so please verify it locally before merge smile I don't trust my old laptops performance

I'll fetch your code and test it on mine (maybe benchmarks could be done by Travis?).

Edit: I now saw that the issue is assigned to @seanchen1991 I hope you don't mind me trying to do it sweat_smile

Yeah :/ next time try to check if the issue is not assigned beforehand. But no problem.

[pvdrz]

I ran the benchmarks and got the same regressions. I believe (take it with a grain of salt) that this has something to do with the match inside the new step function. I'm going to tinker a little bit with this and decide what to do.

Edit: After poking the code here and there I discovered something. If you remove the short-circuiting patterns from step_bin_op we still have a regression (it is smaller tho):

arithmetic              time:   [945.88 ns 948.92 ns 952.40 ns]
arithmetic              time:   [1.2528 us 1.2547 us 1.2569 us]

fact_rec                time:   [220.55 us 220.97 us 221.46 us]
fact_rec                time:   [271.30 us 271.49 us 271.68 us]

fact_tail               time:   [97.284 us 97.647 us 98.127 us]
fact_tail               time:   [103.22 us 103.92 us 104.79 us]

fancy_max               time:   [2.6738 us 2.6831 us 2.6947 us]
fancy_max               time:   [2.6679 us 2.6791 us 2.6940 us]

step                    time:   [1.2486 us 1.2505 us 1.2526 us]
step                    time:   [1.2769 us 1.2886 us 1.3125 us]

I think we can omit the short circuiting part for now and implement it in an earlier stage. Something that happens after the creation of the LIR of the code but before execution. However I'd like to have both your opinions before taking the next step.

[DarkDrek]

Than we would lose the ability to short circuit besides the simplest cases.

This would not be short circuited before execution.

fn foo() do
    true
end

foo() || loop()

[pvdrz]

Than we would lose the ability to short circuit besides the simplest cases

Ok that's true. I didn't think about that. I'm going to check which rules are executed when to see if we can win some performance somewhere.

[pvdrz]

This would not be short circuited before execution.

fn foo() do
    true
end

foo() || loop()

you made me realize that

fn foo() do
    true
end

loop() || foo()

doesn't short circuit right now, or does it?

[DarkDrek]

This short circuits

fn rec loop(): Bool do
    loop()
end

fn foo() do
    true
end

loop() || foo() || loop()

fn foo() do
    if true do
        true
    else
        true
    end
end

foo() || loop()

This does not

fn rec loop(): Bool do
    loop()
end

fn foo() do
    if true do
        true
    else
        true
    end
end

loop() || foo() || loop()

Edit: Is there some code that transforms function without conditions to constant?

[pvdrz]

Edit: Is there some code that transforms function without conditions to constant?

We don't really have the concept of constant yet. However functions without arguments are equivalent to its body. So these two snippets

foo = 0
foo + 1

and

fn foo() do 0 end
foo() + 1

are compiled to the same IR

[DarkDrek]

Ok good to know. I am fine with removing the short circuiting during evaluation and just doing it once before it. But maybe the performance hit is acceptable but our (selection of) benchmark algorithms aren't that good?

[pvdrz]

Ok good to know. I am fine with removing the short circuiting during evaluation and just doing it once before it. But maybe the performance hit is acceptable but our (selection of) benchmark algorithms aren't that good?

Yep benchmarks aren't great. I'll open an issue to discuss it. I think we might take an intermediate decision and just short-circuit using the left term of a binary operation. i.e true || loop() evals to true but loop() || true diverges.

I propose that behavior to be consistent when using longer functions and it should make the regression less pronounced.

What do you think?

[DarkDrek]

That would match the behavior of other languages that evaluate expression from left to right. I'm ok with it I just implemented both ways because it was possible 😆

[pvdrz]

That would match the behavior of other languages that evaluate expression from left to right. I'm ok with it I just implemented both ways because it was possible

Heh yeah I also thought it was cooler that way:

However, we can follow the behavior of other languages here to avoid confussion :P. I'll merge this after that change.

[DarkDrek]

Applied the changes and added one small fix to panic_after to allow panic messages from the inner thread to show in the test thread.

[pvdrz]

LGTM, thanks!