Warn if the benchmarking function is evaluating #358

BrooklinJazz · 2022-05-07T20:14:59Z

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

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

Resolves #355

…unction

…chmark jobs

PragTob · 2022-05-20T09:39:33Z

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.

PragTob

Heyo, thanks for your contribution!

Sorry it took me so long to get around here... busy times.

I left some comments in line. Besides that, I also wonder how to test this.

I was thinking if one could launch iex -S mix and then hand it the input to do a Benchee.run, capture the output and see the message. That might be overdoing it and I didn't manage to get it to go as I couldn't make -S and -e work together.

The other way would be to make the function that checks if the function is being evaluated injectable so that for test purposes we could turn it to true once. Might be overdone for a simple printing message, but I still think about it 😁

Cheers & thanks,
Tobi

PragTob · 2022-05-20T10:07:28Z

lib/benchee.ex

@@ -55,6 +55,14 @@ for {module, moduledoc} <- [{Benchee, elixir_doc}, {:benchee, erlang_doc}] do

    defp add_benchmarking_jobs(suite, jobs) do


👋

This function isn't the best one to add it to, this will only be triggered for Benchee.run/2 - which is not the best as calling Benchee.benchmark manually is also a valid interface.

So this should rather live around Benchee.benchmark or some of the functions it calls: https://github.com/bencheeorg/benchee/blob/main/lib/benchee/benchmark.ex#L27-L36

PragTob · 2022-05-20T10:10:41Z

lib/benchee.ex

+          IO.puts("""
+          Warning: the benchmark #{key} is using an evaluated function.
+            Evaluated functions perform slower than compiled functions.
+            Consider moving all of the Benchee caller to a function in a module and invoking the `Mod.fun()` instead.


In the Benchee.Benchmark module you'll see that there is a dedicated printer module to cover printing of text messages (for testability but also separation of concerns).

As for the warning itself, I think we'd also need to tell folks that they can put the benchmark into a benchmark.exs file and just mix run benchmark.exs - that also works to avoid this. Might bloat up the message a bit too much and we might wanna link to some wiki/README instead. Not sure. See what you think :)

PragTob · 2022-05-20T10:12:38Z

lib/benchee.ex

@@ -55,6 +55,14 @@ for {module, moduledoc} <- [{Benchee, elixir_doc}, {:benchee, erlang_doc}] do

    defp add_benchmarking_jobs(suite, jobs) do
      Enum.reduce(jobs, suite, fn {key, function}, suite_acc ->
+        if is_function(function) && :erlang.fun_info(function, :module) == {:module, :erl_eval} do


As you said you didn't deal with the tuple version here. I think we need to deal with it though, although it's not often used it should still work :)

Iirc it is {function, kwlist} so a pattern match to extract the function we want to check should be rather easy.

PragTob · 2022-05-20T10:36:21Z

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

BrooklinJazz · 2022-05-31T18:30:20Z

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 · 2022-06-01T16:09:48Z

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

Co-authored-by: aar2dee2 <aar2dee2@users.noreply.github.com> Co-authored-by: ReecesPeanutButterCodes <ReecesPeanutButterCodes@users.noreply.github.com> Co-authored-by: Justin Morgan <justin-m-morgan@users.noreply.github.com> Co-authored-by: Tom <BigTom@users.noreply.github.com> Co-authored-by: Jeremy Brayton <w0rd-driven@users.noreply.github.com> Co-authored-by: Russell Baker <3triangles@users.noreply.github.com>

BrooklinJazz · 2022-06-01T16:33:14Z

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 · 2022-06-11T10:41:54Z

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 😁

Let me see if I can get it to work 😁 (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 · 2022-06-11T11:02:32Z

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 😁

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()

PragTob

Code looking good!

I'll add the test and maybe do some minor adjustments hopefully today but maybe tomorrow/during the week.

Thanks a lot! 💚

BrooklinJazz · 2022-06-13T18:39:34Z

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

PragTob · 2022-06-18T12:30:21Z

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

PragTob · 2022-07-08T15:13:34Z

And it was merged, thanks y'all!

BrooklinJazz added 3 commits May 7, 2022 13:03

warn if benchmark uses an evaluated function rather than a compiled f…

0c2d91c

…unction

refactor one line pipe

4c3eff1

remove added whitespace

1de81d2

BrooklinJazz changed the title ~~Warn if the benchmarking function is evaluating~~ Warn if the benchmarking function is evaluating [WIP] May 7, 2022

test if function is a function for benchmark warnings when adding ben…

7fb829b

…chmark jobs

BrooklinJazz changed the title ~~Warn if the benchmarking function is evaluating [WIP]~~ Warn if the benchmarking function is evaluating May 7, 2022

PragTob reviewed May 20, 2022

View reviewed changes

PragTob approved these changes Jun 13, 2022

View reviewed changes

PragTob mentioned this pull request Jun 18, 2022

Eval function notice #365

Merged

PragTob closed this Jul 8, 2022

PragTob mentioned this pull request Jul 8, 2022

Warn if the benchmarking function is evaluating #355

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Warn if the benchmarking function is evaluating #358

Warn if the benchmarking function is evaluating #358

BrooklinJazz commented May 7, 2022 •

edited

Loading

PragTob commented May 20, 2022

PragTob left a comment

PragTob May 20, 2022

PragTob May 20, 2022

PragTob May 20, 2022

PragTob commented May 20, 2022

BrooklinJazz commented May 31, 2022

PragTob commented Jun 1, 2022

BrooklinJazz commented Jun 1, 2022 •

edited

Loading

PragTob commented Jun 11, 2022

PragTob commented Jun 11, 2022

PragTob left a comment

BrooklinJazz commented Jun 13, 2022

PragTob commented Jun 18, 2022

PragTob commented Jul 8, 2022

		@@ -55,6 +55,14 @@ for {module, moduledoc} <- [{Benchee, elixir_doc}, {:benchee, erlang_doc}] do

		defp add_benchmarking_jobs(suite, jobs) do

Warn if the benchmarking function is evaluating #358

Warn if the benchmarking function is evaluating #358

Conversation

BrooklinJazz commented May 7, 2022 • edited Loading

PragTob commented May 20, 2022

PragTob left a comment

Choose a reason for hiding this comment

PragTob May 20, 2022

Choose a reason for hiding this comment

PragTob May 20, 2022

Choose a reason for hiding this comment

PragTob May 20, 2022

Choose a reason for hiding this comment

PragTob commented May 20, 2022

BrooklinJazz commented May 31, 2022

PragTob commented Jun 1, 2022

BrooklinJazz commented Jun 1, 2022 • edited Loading

PragTob commented Jun 11, 2022

PragTob commented Jun 11, 2022

PragTob left a comment

Choose a reason for hiding this comment

BrooklinJazz commented Jun 13, 2022

PragTob commented Jun 18, 2022

PragTob commented Jul 8, 2022

BrooklinJazz commented May 7, 2022 •

edited

Loading

BrooklinJazz commented Jun 1, 2022 •

edited

Loading