Skip to content
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

Added initial Solaris support to Benchee.System #422

Merged
merged 3 commits into from
Dec 23, 2023
Merged

Added initial Solaris support to Benchee.System #422

merged 3 commits into from
Dec 23, 2023

Conversation

brianewell
Copy link
Contributor

@brianewell brianewell commented Dec 22, 2023

Associated PR for issue: #421
Added calls as appropriate to kstat and prtconf to produce intended results.

This was specifically tested on SmartOS base-64-trunk 20231113

Copy link
Member

@PragTob PragTob left a comment

Choose a reason for hiding this comment

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

Thank you very much for the PR! Looking good! 💚

Only 2 small comments, but one is a bit "tougher" as we can't rely on Jason.

IMG_20220901_174517

@@ -139,6 +144,11 @@ defmodule Benchee.System do

def parse_cpu_for(:FreeBSD, raw_output), do: String.trim(raw_output)

def parse_cpu_for(:Solaris, raw_output) do
Copy link
Member

Choose a reason for hiding this comment

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

Adding a test may be neat, that's the reason that function is public :) It's also nice for me to have an example of the output: https://github.com/bencheeorg/benchee/blob/main/test/benchee/system_test.exs#L49

Copy link
Contributor Author

@brianewell brianewell Dec 23, 2023

Choose a reason for hiding this comment

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

Illumos kstat can be flagged to output data in json format:

$ kstat -j cpu_info:0::brand
[{
        "module": "cpu_info",
        "instance": 0,
        "name": "cpu_info0",
        "class": "misc",
        "type": 1,
        "snaptime": 3916379.363662097,
        "data": {
                "brand": "Intel(r) Xeon(r) CPU E5-2678 v3 @ 2.50GHz"
        }
}]

More universally, the -p (parsable) flag can be used to reduce the output to key value pairs:

$ kstat -p cpu_info:0:cpu_info0:brand
cpu_info:0:cpu_info0:brand      Intel(r) Xeon(r) CPU E5-2678 v3 @ 2.50GHz

This second command is effective across pretty much every variant of Solaris.

@@ -139,6 +144,11 @@ defmodule Benchee.System do

def parse_cpu_for(:FreeBSD, raw_output), do: String.trim(raw_output)

def parse_cpu_for(:Solaris, raw_output) do
{:ok, [decoded]} = Jason.decode(raw_output)
Copy link
Member

Choose a reason for hiding this comment

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

This works by accident. Benchee does not depend on Jason and should not depend on it. It's available in the dev and test environments due to dev/test dependencies:

tobi@qiqi:~/github/benchee(main)$ mix deps.tree
benchee
├── credo ~> 1.7.2-rc.0 (Hex package)
│   ├── bunt ~> 0.2.1 or ~> 1.0 (Hex package)
│   ├── file_system ~> 0.2 or ~> 1.0 (Hex package)
│   └── jason ~> 1.0 (Hex package)
├── deep_merge ~> 1.0 (Hex package)
├── dialyxir ~> 1.0 (Hex package)
│   └── erlex >= 0.2.6 (Hex package)
├── ex_doc >= 0.0.0 (Hex package)
│   ├── earmark_parser ~> 1.4.39 (Hex package)
│   ├── makeup_elixir ~> 0.14 (Hex package)
│   │   ├── makeup ~> 1.0 (Hex package)
│   │   │   └── nimble_parsec ~> 1.2.2 or ~> 1.3 (Hex package)
│   │   └── nimble_parsec ~> 1.2.3 or ~> 1.3 (Hex package)
│   └── makeup_erlang ~> 0.1 (Hex package)
│       └── makeup ~> 1.0 (Hex package)
├── ex_guard ~> 1.3 (Hex package)
│   └── fs ~> 8.6.1 (Hex package)
├── excoveralls ~> 0.13 (Hex package)
│   └── jason ~> 1.0 (Hex package)
├── statistex ~> 1.0 (Hex package)
└── table ~> 0.1.0 (Hex package)

so, we need make do here without Jason :)

@PragTob
Copy link
Member

PragTob commented Dec 23, 2023

Ah one more @brianewell - you don't seem to have run the elixir formatter. Run mix format - also double check if mix credo passes :) The CI runs a whole bunch of things 😁

@PragTob
Copy link
Member

PragTob commented Dec 23, 2023

Looking great, thanks a lot! 💚

@PragTob PragTob merged commit 46f27c5 into bencheeorg:main Dec 23, 2023
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants