-
Notifications
You must be signed in to change notification settings - Fork 64
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add warning when protocols are not consolidated #231
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a bunch! 👍
Like to have it in System
:)
lib/benchee/system.ex
Outdated
path = :code.lib_dir(:elixir, :ebin) | ||
|
||
[path] | ||
|> Protocol.extract_protocols() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no idea how this works :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It takes a list of paths to directories full of BEAM files and tells you which protocols have been defined in those directories. It nicely returns the list of atoms corresponding to the protocol name. Very helpful when combined with :code.lib_dir/2
to get the top level directory for the Elixir BEAM files. I used this as a proxy since if those aren't consolidated, then there's no way any of your other protocols are consolidated. In theory it's still possible for a protocol to have been defined and not consolidated, but that would emit a warning (like if you're defining a protocol in a test body or something).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah yeah that's what I wondered about - I thought this just caught it from elixir itself and not deps/libraries - but you're right, if I understand correctly it should usually be all or nothing in terms of consolidation if I understand it.
Not all of your protocols have been consolidated. In order to achieve the | ||
best possible accuracy for benchmarks, please ensure protocol | ||
consolidation is enabled in your benchmarking environment. | ||
""") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'd extract this into its own method, maybe just the puts
maybe the whole thing. Takes up so much space that makes it seem like a more important piece then it actually is right now.
Maybe a method like warn_about_performance_degrading_settings
or something? not sure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
works for me!
This adds a warning when folks try to run benchmarks in environments without protocol consolidation enabled. I have no idea how to test this since you can't turn protocol consolidation off at runtime, though.
This adds a warning when folks try to run benchmarks in environments
without protocol consolidation enabled.
I have no idea how to test this since you can't turn protocol
consolidation off at runtime, though. I did test this locally, and this
is the output I got:
Resolves #68