Gather mem information for MacOS and Linux #20
Conversation
lib/beamchmark/suite/system_info.ex
Outdated
| @spec mem(atom()) :: integer() | ||
| defp mem(os) do | ||
| {memory_str, 0} = | ||
| case os do | ||
| :macOS -> System.cmd("sysctl", ["-n", "hw.memsize"]) | ||
| :Linux -> System.cmd("awk", ["/MemTotal/ {print $2}", "/proc/meminfo"]) | ||
| _os -> 0 | ||
| end | ||
|
|
||
| memory_str |> String.trim() |> String.to_integer() | ||
| end |
There was a problem hiding this comment.
Some comments here
- I think that we shouldn't assume that there is
awkinstalled in OS. Instead let's parse this file on our own. - The same for
sysctl. I am not sure about using it. Wouldn't it be possible to read some system file manually? Could we check how benchee does it? - We should be prepared for situation where we couldn't get MEM info and in such a case return something like
{:error, :couldnt_get_mem_info}or-1. We should then change typespec for mem in stuct SystemInfo topos_integer() | :unknownorinteger()depending on what we want to return frommem().
There was a problem hiding this comment.
As per https://unix.stackexchange.com/questions/82409/are-there-versions-of-unix-that-dont-have-awk-in-default-install I will assume, that all linux distributions have awk installed. I would assume as well, that all MacOS distibutions come with sysctl.
There was a problem hiding this comment.
Hmm 🤔 Okay so let's keep this as it is
lib/beamchmark/formatters/utils.ex
Outdated
| end | ||
| end | ||
|
|
||
| def format_memory(_mem) do |
There was a problem hiding this comment.
I think we should change typespec for format_memory. It requires non_neg_integer but here we are handling all non_pos_integers. Let's run dialyzer to check other typespecs too 😉
roznawsk
force-pushed
the
gather_mem_information
branch
from
b848d0d
to
2557547
Compare
|
As I wrote @shuntrho, it looks like I won't be able to continue supervising this project. Someone else has to be responsible for leading it. |
mix.exs
Outdated
| @@ -46,7 +46,8 @@ defmodule Beamchmark.MixProject do | |||
| {:bunch, "~> 1.3.0"}, | |||
| {:dialyxir, "~> 1.1.0", only: :dev, runtime: false}, | |||
| {:credo, "~> 1.6.1", only: :dev, runtime: false}, | |||
| {:ex_doc, "~> 0.27.0", only: :dev, runtime: false} | |||
| {:ex_doc, "~> 0.27.0", only: :dev, runtime: false}, | |||
| {:math, "~> 0.6.0"} | |||
There was a problem hiding this comment.
Why not use math 0.7.0?
There was a problem hiding this comment.
The version wasn't updated in Math's hexdocs, I just copy-pasted it :P.
| def format_memory(mem) when is_integer(mem) do | ||
| log_mem = Math.log(mem, 1024) | ||
|
|
||
| cond do |
There was a problem hiding this comment.
NIT: please also add the case for TB.
There was a problem hiding this comment.
This makes sense if we decide to use this function to parse disk space as well.
| otp_version: String.t(), | ||
| nif_version: String.t(), | ||
| os: atom(), | ||
| mem: pos_integer() | :unknown, | ||
| arch: String.t(), | ||
| num_cores: pos_integer() | ||
| } | ||
|
|
||
| @enforce_keys [:elixir_version, :otp_version, :nif_version, :os, :arch, :num_cores] | ||
| @enforce_keys [:elixir_version, :otp_version, :nif_version, :os, :mem, :arch, :num_cores] |
There was a problem hiding this comment.
NIT: please sort the struct fields alphabetically.
There was a problem hiding this comment.
I would rather see the fields ordered logically rather than alphabetically, so I will leave them as they are.
|
|
No description provided.