bencheeorg / benchee

Easy and extensible benchmarking in Elixir providing you with lots of statistics!
MIT License
1.42k stars 66 forks source link

Warn if the benchmarking function is evaluating #358

Closed BrooklinJazz closed 2 years ago

BrooklinJazz commented 2 years ago

First time contributing to benchee, please let me know how I can improve this PR! 🙏

This was done in a group with @czrpb, @aar2dee2, and @ReecesPeanutButterCodes.

Attempting to warn if the benchmarking function is evaluated instead of compiled based on #355 .

We were not sure how to best present the warning, and could not find previous examples to follow. Please let me know if there is a better way than a multiline IO.puts

image

We opted to only handle when the benchmarked function is a function, rather than when it's a tuple.

Resolves #355

PragTob commented 2 years ago

Hi there,

thanks for the contribution and sorry for the long wait - been quite busy and my arms are still so-so so OSS time is limited.

Still fighting with the CI a bit, reviewing in a few.

WhatsApp Image 2021-09-19 at 11 46 51

PragTob commented 2 years ago

(the test failure isn't your failt, just gotta rebase/merge with main)

BrooklinJazz commented 2 years ago

Hey @PragTob, just want to let you know what we got together last weekend to work on this PR. All comments have been handled, the only thing left to do is figure out how to test this. We plan to get together next weekend on Saturday to hopefully finish it up.

PragTob commented 2 years ago

@BrooklinJazz thanks for the update! Might as well be too hard to test, feel free to push the code and I might chime in.

Sometimes testing isn't worth the effort (sadly). Try is important, but don't bind it to that or break your backs trying to do that.

BrooklinJazz commented 2 years ago

Thank you @PragTob! I've pushed the changes so you can have a look.

I'm really curious to try your recommended solution of launching the IEx shell (not sure how to do that from a test, would appreciate it if you have any tips). If that doesn't work I think we could use a mock.

Maybe the test isn't worth it for the sake of getting this PR in, that's up to you! But, I'd still love to circle around to it and try it during our next Saturday session.

Wanted to say thank you again @PragTob! Your feedback really helped our learning group :)

PragTob commented 2 years ago

Hey there, live was busier than expected again :(

So... for launching iex with Benchee I couldn't quite get it done somehow... theoretically it should be possible by launching a process that launches iex and then sending the input to it and capturing its output. should be possible - but I never did it :grin:

Let me see if I can get it to work :grin: (might take some time, am at a conference and as I said it was full).

I usually like to have test and thing go in together, but as you said correctly (and I think me too) might not be worth it here though.

PragTob commented 2 years ago

I'm 100% sure that I was being stupid when it comes to receiving messages but this script should work as the basis for a test, might implement it :grin:

defmodule Receive do
  def recv do
    receive do
      message ->
        IO.inspect(message)
    end
    recv()
  end
end

port = Port.open({:spawn, "iex -S mix"}, [:binary])

send(port, {self(), {:command, "Benchee.run(%{\"test\" => fn -> 1 end}, time: 0.001, warmup: 0)\n"}})

Receive.recv()
BrooklinJazz commented 2 years ago

Thank you @PragTob! Appreciate your help with this one!

PragTob commented 2 years ago

Merging as #365 with the added changes, thanks y'all a lot!

IMG_20220418_085032_Bokeh

PragTob commented 2 years ago

And it was merged, thanks y'all!