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

Windows support #42

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 15 additions & 5 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,16 @@ on: [push]

jobs:
build:
strategy:
matrix:
os: [ubuntu-latest, windows-latest]

runs-on: ubuntu-latest
runs-on: ${{ matrix.os }}

steps:
- name: Disable autocrlf (Windows)
if: runner.os == 'Windows'
run: git config --global core.autocrlf false
- uses: actions/checkout@v4
with:
submodules: true
Expand All @@ -19,14 +25,18 @@ jobs:
- uses: Swatinem/rust-cache@v2
- name: Run cargo build
run: cargo build --release --features default-networks
- name: Check if code is formatted
- name: Check if code is formatted (Linux)
if: runner.os == 'Linux'
run: cargo fmt --check
- name: Run Clippy
- name: Run Clippy (Linux)
if: runner.os == 'Linux'
run: scripts/ci/clippy.bash --deny warnings
- name: Run tests
run: cargo test --release --no-fail-fast
- name: Check consensus-spec-tests coverage
- name: Check consensus-spec-tests coverage (Linux)
if: runner.os == 'Linux'
run: scripts/ci/consensus-spec-tests-coverage.rb
# Ignore RUSTSEC-2023-0071 because we don't use RSA in `jwt-simple`
- name: Run cargo audit
- name: Run cargo audit (Linux)
if: runner.os == 'Linux'
run: cargo audit --ignore RUSTSEC-2024-0370 --ignore RUSTSEC-2023-0071
1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -504,6 +504,7 @@ try_from_iterator = { path = 'try_from_iterator' }
types = { path = 'types' }
validator = { path = 'validator' }
validator_key_cache = { path = 'validator_key_cache' }
winsafe = { git = 'https://github.com/rodrigocfd/winsafe', features = ['kernel', 'psapi'] }

# Banned crates
#
Expand Down
4 changes: 3 additions & 1 deletion ad_hoc_bench/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,9 @@ eth2_cache_utils = { workspace = true }
fork_choice_control = { workspace = true }
fork_choice_store = { workspace = true }
futures = { workspace = true }
jemalloc-ctl = { workspace = true }
log = { workspace = true }
rand = { workspace = true }
types = { workspace = true }

[target.'cfg(not(windows))'.dependencies]
jemalloc-ctl = { workspace = true }
25 changes: 14 additions & 11 deletions ad_hoc_bench/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,11 @@ use core::ops::RangeInclusive;
use std::{collections::BTreeMap, sync::Arc, time::Instant};

use allocator as _;
use anyhow::{Error, Result};
use bytesize::ByteSize;
use anyhow::Result;
use clap::{Parser, ValueEnum};
use eth2_cache_utils::{goerli, holesky, holesky_devnet, mainnet, medalla, withdrawal_devnet_4};
use fork_choice_control::AdHocBenchController;
use fork_choice_store::StoreConfig;
use jemalloc_ctl::Result as JemallocResult;
use log::info;
use rand::seq::SliceRandom as _;
use types::{
Expand Down Expand Up @@ -247,7 +245,7 @@ impl From<Blocks> for BlockParameters {
fn main() -> Result<()> {
binary_utils::initialize_logger(module_path!(), false)?;
binary_utils::initialize_rayon()?;

#[cfg(not(target_os = "windows"))]
print_jemalloc_stats()?;

let options = Options::parse();
Expand Down Expand Up @@ -296,7 +294,7 @@ fn main() -> Result<()> {
|_, _| BTreeMap::new(),
),
}?;

#[cfg(not(target_os = "windows"))]
print_jemalloc_stats()?;

Ok(())
Expand All @@ -312,6 +310,7 @@ fn run<P: Preset>(
beacon_blocks: impl FnOnce(RangeInclusive<Slot>, usize) -> Vec<Arc<SignedBeaconBlock<P>>>,
blob_sidecars: impl FnOnce(RangeInclusive<Slot>, usize) -> BTreeMap<Slot, Vec<Arc<BlobSidecar<P>>>>,
) -> Result<()> {
#[cfg(not(target_os = "windows"))]
print_jemalloc_stats()?;

let Options {
Expand Down Expand Up @@ -422,11 +421,15 @@ fn run<P: Preset>(
info!("average block throughput: {block_throughput:.3} blocks/s");
info!("average slot throughput: {slot_throughput:.3} slots/s");

print_jemalloc_stats()
#[cfg(not(target_os = "windows"))]
print_jemalloc_stats()?;

Ok(())
}

#[cfg(not(target_os = "windows"))]
fn print_jemalloc_stats() -> Result<()> {
jemalloc_ctl::epoch::advance().map_err(Error::msg)?;
jemalloc_ctl::epoch::advance().map_err(anyhow::Error::msg)?;
Comment on lines -429 to +432
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: Import types at the top of the file.

#[cfg(target_os = "windows")]
use ::{anyhow::Error, bytesize::ByteSize, jemalloc_ctl::Result as JemallocResult};

Copy link
Author

Choose a reason for hiding this comment

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

This import is just used for not(target_os = "windows"). To prompt it to the top in import will cause additional verbose imports.

But, I can also accept that if you, as a committer, think you want to use this more verbose imports.


info!(
"allocated: {}, \
Expand All @@ -445,9 +448,9 @@ fn print_jemalloc_stats() -> Result<()> {

Ok(())
}

fn human_readable_size(result: JemallocResult<usize>) -> Result<String> {
let size = result.map_err(Error::msg)?;
#[cfg(not(target_os = "windows"))]
fn human_readable_size(result: jemalloc_ctl::Result<usize>) -> Result<String> {
let size = result.map_err(anyhow::Error::msg)?;
let size = size.try_into()?;
Ok(ByteSize(size).to_string_as(true))
Ok(bytesize::ByteSize(size).to_string_as(true))
}
9 changes: 7 additions & 2 deletions metrics/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -20,15 +20,13 @@ futures = { workspace = true }
grandine_version = { workspace = true }
helper_functions = { workspace = true }
http_api_utils = { workspace = true }
jemalloc-ctl = { workspace = true }
log = { workspace = true }
num_threads = { workspace = true }
p2p = { workspace = true }
prometheus = { workspace = true }
# `prometheus-client` is only needed for libp2p metrics.
prometheus-client = { workspace = true }
prometheus_metrics = { workspace = true }
psutil = { workspace = true }
reqwest = { workspace = true }
serde = { workspace = true }
std_ext = { workspace = true }
Expand All @@ -40,5 +38,12 @@ tower-http = { workspace = true }
transition_functions = { workspace = true }
types = { workspace = true }

[target.'cfg(not(windows))'.dependencies]
jemalloc-ctl = { workspace = true }
psutil = { workspace = true }

[target.'cfg(windows)'.dependencies]
winsafe = { workspace = true }

[dev-dependencies]
serde_json = { workspace = true }
55 changes: 20 additions & 35 deletions metrics/src/beaconchain.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ use helper_functions::{accessors, predicates};
use log::warn;
use p2p::metrics::PEERS_CONNECTED;
use prometheus::IntGauge;
use psutil::{cpu::CpuTimes, process::Process};
use serde::Serialize;
use sysinfo::{Disks, System};
use types::{preset::Preset, traits::BeaconState};
Expand Down Expand Up @@ -88,29 +87,13 @@ pub struct ProcessMetrics {

impl ProcessMetrics {
pub fn get() -> Self {
let mut cpu_process_seconds_total = 0;
let mut memory_process_bytes = 0;

match Process::current() {
Ok(process) => {
match process.cpu_times() {
Ok(cpu_times) => {
cpu_process_seconds_total = cpu_times.busy().as_secs()
+ cpu_times.children_system().as_secs()
+ cpu_times.children_system().as_secs();
}
Err(error) => warn!("unable to get current process CPU usage: {error:?}"),
}

match process.memory_info() {
Ok(mem_info) => {
memory_process_bytes = mem_info.rss();
}
Err(error) => warn!("unable to get process memory usage: {error:?}"),
}
}
Err(error) => warn!("unable to get current process: {error:?}"),
}
let Ok(crate::metric_sys::ProcessCpuMetric {
cpu_process_seconds_total,
memory_process_bytes,
}) = crate::metric_sys::get_process_cpu_metric()
else {
panic!("unable to get current process's cpu metric");
};

let client_build = DateTime::parse_from_rfc3339(build_time_utc!())
.expect("unable to parse build time")
Expand Down Expand Up @@ -286,12 +269,16 @@ struct PlatformSpecificSystemMetrics {

impl PlatformSpecificSystemMetrics {
#[cfg(not(target_os = "linux"))]
fn new(_cpu: Option<&CpuTimes>) -> Self {
fn new() -> Self {
Self::default()
}

#[cfg(target_os = "linux")]
fn new(cpu: Option<&CpuTimes>) -> Self {
fn new() -> Self {
let cpu_times = psutil::cpu::cpu_times()
.map_err(|error| warn!("unable to get CPU times information: {error:?}"))
.ok();
let cpu = cpu_times.as_ref();
let mem = psutil::memory::virtual_memory()
.map_err(|error| warn!("unable to get virtual memory information: {error:?}"))
.ok();
Expand Down Expand Up @@ -325,19 +312,17 @@ impl SystemMetrics {
let (network_node_bytes_total_receive, network_node_bytes_total_transmit) =
helpers::get_network_bytes();

let cpu_times = psutil::cpu::cpu_times()
.map_err(|error| warn!("unable to get CPU times information: {error:?}"))
.ok();

let cpu = cpu_times.as_ref();
let Ok(cpu_metric) = crate::metric_sys::get_cpu_metric() else {
panic!("unable to get current system's cpu metric");
};

Self {
// CPU
cpu_cores: system.physical_core_count().unwrap_or_default(),
cpu_threads: system.cpus().len(),
cpu_node_system_seconds_total: cpu.map(CpuTimes::total).unwrap_or_default().as_secs(),
cpu_node_user_seconds_total: cpu.map(CpuTimes::user).unwrap_or_default().as_secs(),
cpu_node_idle_seconds_total: cpu.map(CpuTimes::idle).unwrap_or_default().as_secs(),
cpu_node_system_seconds_total: cpu_metric.system_seconds,
cpu_node_user_seconds_total: cpu_metric.user_seconds,
cpu_node_idle_seconds_total: cpu_metric.idle_seconds,

// memory
memory_node_bytes_total: system.total_memory(),
Expand All @@ -359,7 +344,7 @@ impl SystemMetrics {
misc_os: metrics_os(),

// platform specific metrics
platform_specific_metrics: PlatformSpecificSystemMetrics::new(cpu),
platform_specific_metrics: PlatformSpecificSystemMetrics::new(),
}
}
}
Expand Down
1 change: 1 addition & 0 deletions metrics/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,5 +8,6 @@ mod beaconchain;
mod gui;
mod helpers;
mod messages;
mod metric_sys;
mod server;
mod service;
123 changes: 123 additions & 0 deletions metrics/src/metric_sys.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,123 @@
use anyhow::Result;
#[cfg(target_os = "linux")]
use {
anyhow::bail,
psutil::{cpu, process::Process},
};
#[cfg(target_os = "windows")]
use {
log::debug,
winsafe::{self as w, prelude::*},
};

pub struct ProcessCpuMetric {
pub cpu_process_seconds_total: u64,
pub memory_process_bytes: u64,
}

#[cfg(target_os = "linux")]
pub fn get_process_cpu_metric() -> Result<ProcessCpuMetric> {
#[allow(unused_assignments)]
let mut cpu_process_seconds_total = 0;
#[allow(unused_assignments)]
let mut memory_process_bytes = 0;
Comment on lines +20 to +23
Copy link
Contributor

Choose a reason for hiding this comment

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

polish: Leave the variables uninitialized to pass the unused_assignments lint.

Suggested change
#[allow(unused_assignments)]
let mut cpu_process_seconds_total = 0;
#[allow(unused_assignments)]
let mut memory_process_bytes = 0;
let cpu_process_seconds_total;
let memory_process_bytes;

Copy link
Author

Choose a reason for hiding this comment

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

gone with new impl based on winsafe

match Process::current() {
Ok(process) => {
match process.cpu_times() {
Ok(cpu_times) => {
cpu_process_seconds_total = cpu_times.busy().as_secs()
+ cpu_times.children_system().as_secs()
+ cpu_times.children_system().as_secs();
}
Err(error) => bail!("unable to get current process CPU usage: {error:?}"),
}

match process.memory_info() {
Ok(mem_info) => {
memory_process_bytes = mem_info.rss();
}
Err(error) => bail!("unable to get process memory usage: {error:?}"),
}
}
Err(error) => bail!("unable to get current process: {error:?}"),
}
Ok(ProcessCpuMetric {
cpu_process_seconds_total,
memory_process_bytes,
})
}

#[allow(unused_assignments)]
#[cfg(target_os = "windows")]
pub fn get_process_cpu_metric() -> Result<ProcessCpuMetric> {
let proc = w::HPROCESS::GetCurrentProcess();

// Get CPU times
let (_, _, kernel, user) = proc.GetProcessTimes()?;
let kernel_seconds = filetime_to_seconds(kernel);
let user_seconds = filetime_to_seconds(user);
let cpu_process_seconds_total = kernel_seconds + user_seconds;
debug!("CPU time: {:.2} seconds", cpu_process_seconds_total);

// Get memory info
let mem_info = proc.GetProcessMemoryInfo()?;
let memory_process_bytes = mem_info.WorkingSetSize.try_into()?;
debug!("memory usage: {} bytes", memory_process_bytes);

Ok(ProcessCpuMetric {
cpu_process_seconds_total,
memory_process_bytes,
})
}

#[derive(Debug)]
pub struct CpuMetric {
pub idle_seconds: u64,
pub system_seconds: u64,
pub user_seconds: u64,
}

// TODO maybe work for MacOS or wider Unix?
#[cfg(target_os = "linux")]
pub fn get_cpu_metric() -> Result<CpuMetric> {
let cpu = cpu::cpu_times()?;
let system_seconds = cpu.total().as_secs();
let user_seconds = cpu.user().as_secs();
let idle_seconds = cpu.idle().as_secs();
Ok(CpuMetric {
idle_seconds,
system_seconds,
user_seconds,
})
}

#[cfg(target_os = "windows")]
pub fn get_cpu_metric() -> Result<CpuMetric> {
let (idle_time, kernel_time, user_time) = w::GetSystemTimes()?;

// Convert FILETIME to u64 (100-nanosecond intervals)
let idle = filetime_to_seconds(idle_time);
let kernel = filetime_to_seconds(kernel_time);
let user = filetime_to_seconds(user_time);

// Calculate system time (kernel time includes idle time)
let system = kernel - idle;

Ok(CpuMetric {
idle_seconds: idle,
system_seconds: system,
user_seconds: user,
})
}

// NOTE:
// FILETIME: Contains a 64-bit value representing the number of 100-nanosecond intervals since January 1, 1601 (UTC). (Ref: https://learn.microsoft.com/en-us/windows/win32/api/minwinbase/ns-minwinbase-filetime)
// But some Windows APIs just use it to represent a relative time interval, a.k.a., duration.
// For example, GetSystemTimes like APIs are used in this mod.
// The helper function just converts the number of 100-nanosecond into the number of seconds
#[inline(always)]
#[cfg(target_os = "windows")]
fn filetime_to_seconds(ft: w::FILETIME) -> u64 {
let total_seconds = ((ft.dwHighDateTime as u64) << 32 | ft.dwLowDateTime as u64) / 10_000_000;
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: Document the magic number somehow.

Consider extracting a constant with a self-documenting name.
Consider linking to the documentation of FILETIME.

Copy link
Author

Choose a reason for hiding this comment

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

accepted.

Copy link
Contributor

Choose a reason for hiding this comment

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

note: FILETIME is a kind of timestamp (with an unusual epoch), but GetSystemTimes uses it to represent a duration.

Copy link
Author

Choose a reason for hiding this comment

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

ok.

total_seconds
}
Loading