Skip to content

Conversation

@PragTob
Copy link
Contributor

@PragTob PragTob commented Feb 20, 2022

As discussed in: https://groups.google.com/g/elixir-lang-core/c/f0gP0It-yuU

But basically:

  • right now no return value is documented/discussed so should not
    be relied upon
  • the original profiler themselves all return the return value,
    so it should be possible
  • I need it in Benchee 👼

Happy to make any kind of adjustments. I was thinking about introducing a behavior as all of these functions were very samey (and in fact in Benchee we rely on them being the same) but not sure it's needed here.

Thanks for all your work y'all! 💚

The bunny thanks thee!

IMG_20220109_104347

As discussed in: https://groups.google.com/g/elixir-lang-core/c/f0gP0It-yuU

But basically:
* right now no return value is documented/discussed so should not
be relied upon
* the original profiler themselves all return the return value,
so it should be possible
* I need it in Benchee 👼

:eprof.start()
:eprof.profile([], fun, Keyword.get(opts, :matching, {:_, :_, :_}))
{:ok, return_value} = :eprof.profile([], fun, Keyword.get(opts, :matching, {:_, :_, :_}))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This bit is potentially dangerous, as it might return {:error, Reason} https://www.erlang.org/doc/man/eprof.html#profile-1

If tracing could be enabled for P and all processes in Rootset, the function returns {ok,Value} when Fun()/apply returns with the value Value, or {error,Reason} if Fun()/apply fails with exit reason Reason. Otherwise it returns {error, Reason} immediately.

It would be a rather hard failure here, on the other hand I'm not sure a possible error was handled gracefully before. Also not sure how to best "gracefully" handle it here.

Feedback welcome!

@josevalim josevalim merged commit 36eb6eb into elixir-lang:main Feb 20, 2022
@josevalim
Copy link
Member

💚 💙 💜 💛 ❤️

@PragTob
Copy link
Contributor Author

PragTob commented Feb 21, 2022

@josevalim one small q if I may, I always kind of thought when providing default arguments it's basically seen as 2 different functions, hence I provided 2 type specs for them. Since you changed it I suspect I'm wrong, I just don't know why or/how 😅 If you had a minute to tell me or point me somewhere I'd really appreciate it! 💚

@PragTob PragTob deleted the profiler-return-function-return-value branch February 21, 2022 08:47
@josevalim
Copy link
Member

josevalim commented Feb 21, 2022

Yes, they are different functions! @spec-ing both is fine, it is just not a practice we do in the Elixir repo. :) Especially because for us specs are mainly for docs purpose and the other one is never showed in the docs.

@PragTob
Copy link
Contributor Author

PragTob commented Feb 21, 2022

Got ya, thanks a lot! 💚
IMG_20180201_140436-ANIMATION

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants