From 42e01b777598aa1c5db58f0e175d49b97a9f616c Mon Sep 17 00:00:00 2001 From: Alex Koshelev Date: Mon, 2 Sep 2024 15:25:48 -0700 Subject: [PATCH 1/2] Remove step-trace and trace features Those were used to generate steps graph at runtime, but we've migrated to the new step engine that generates it at compile time, so we no longer need a script and this set of features --- ipa-core/Cargo.toml | 1 - ipa-core/benches/oneshot/ipa.rs | 24 +--- ipa-core/src/telemetry/stats.rs | 2 - ipa-core/src/telemetry/step_stats.rs | 10 +- ipa-core/src/test_fixture/logging.rs | 11 +- ipa-core/src/test_fixture/world.rs | 3 +- ipa-step/Cargo.toml | 2 - ipa-step/src/descriptive.rs | 11 +- scripts/collect_steps.py | 178 --------------------------- 9 files changed, 16 insertions(+), 226 deletions(-) delete mode 100755 scripts/collect_steps.py diff --git a/ipa-core/Cargo.toml b/ipa-core/Cargo.toml index 45abc4f58..835ebc28c 100644 --- a/ipa-core/Cargo.toml +++ b/ipa-core/Cargo.toml @@ -60,7 +60,6 @@ real-world-infra = [] dhat-heap = ["cli", "test-fixture"] # Enable this feature to enable our colossally weak Fp31. weak-field = [] -step-trace = ["ipa-step/trace"] # Enable using more than one thread for protocol execution. Most of the parallelism occurs at parallel/seq_join operations multi-threading = ["async-scoped"] # Enable tokio task profiling. Requires tokio_unstable flag to be passed to the compiler. diff --git a/ipa-core/benches/oneshot/ipa.rs b/ipa-core/benches/oneshot/ipa.rs index fa0a47452..6ae4aff44 100644 --- a/ipa-core/benches/oneshot/ipa.rs +++ b/ipa-core/benches/oneshot/ipa.rs @@ -122,25 +122,11 @@ async fn run(args: Args) -> Result<(), Error> { q = args.query_size ); let rng = StdRng::seed_from_u64(seed); - let event_gen_config = if cfg!(feature = "step-trace") { - // For the steps collection, compact gate requires: - // * At least one user with the same number of dynamic steps as defined for `UserNthRowStep::Row`. - // * Enough records to exercise the saturating addition case in aggregation. - EventGeneratorConfig { - user_count: NonZeroU64::new(5).unwrap(), - max_trigger_value: NonZeroU32::try_from(args.max_trigger_value).unwrap(), - max_breakdown_key: NonZeroU32::try_from(args.breakdown_keys).unwrap(), - min_events_per_user: NonZeroU32::new(64).unwrap(), - max_events_per_user: NonZeroU32::new(64).unwrap(), - ..Default::default() - } - } else { - EventGeneratorConfig { - max_trigger_value: NonZeroU32::try_from(args.max_trigger_value).unwrap(), - max_breakdown_key: NonZeroU32::try_from(args.breakdown_keys).unwrap(), - max_events_per_user: NonZeroU32::try_from(args.records_per_user).unwrap(), - ..Default::default() - } + let event_gen_config = EventGeneratorConfig { + max_trigger_value: NonZeroU32::try_from(args.max_trigger_value).unwrap(), + max_breakdown_key: NonZeroU32::try_from(args.breakdown_keys).unwrap(), + max_events_per_user: NonZeroU32::try_from(args.records_per_user).unwrap(), + ..Default::default() }; let raw_data = EventGenerator::with_config(rng, event_gen_config) .take(args.query_size) diff --git a/ipa-core/src/telemetry/stats.rs b/ipa-core/src/telemetry/stats.rs index 8191c699c..e6febddb3 100644 --- a/ipa-core/src/telemetry/stats.rs +++ b/ipa-core/src/telemetry/stats.rs @@ -35,7 +35,6 @@ pub struct CounterDetails { pub struct Metrics { pub counters: HashMap, pub metric_description: HashMap, - pub print_header: bool, } impl CounterDetails { @@ -81,7 +80,6 @@ impl Metrics { let mut this = Metrics { counters: HashMap::new(), metric_description: HashMap::new(), - print_header: !cfg!(feature = "step-trace"), }; let snapshot = snapshot.into_vec(); diff --git a/ipa-core/src/telemetry/step_stats.rs b/ipa-core/src/telemetry/step_stats.rs index 2f0e03ac0..661952cfe 100644 --- a/ipa-core/src/telemetry/step_stats.rs +++ b/ipa-core/src/telemetry/step_stats.rs @@ -38,12 +38,10 @@ impl CsvExporter for Metrics { // then dump them to the provided Write interface // TODO: include role dimension. That requires rethinking `Metrics` implementation // because it does not allow such breakdown atm. - if self.print_header { - writeln!( - w, - "Step,Records sent,Bytes sent,Indexed PRSS,Sequential PRSS,Step narrowed" - )?; - } + writeln!( + w, + "Step,Records sent,Bytes sent,Indexed PRSS,Sequential PRSS,Step narrowed" + )?; for (step, stats) in steps_stats.all_steps() { writeln!( w, diff --git a/ipa-core/src/test_fixture/logging.rs b/ipa-core/src/test_fixture/logging.rs index 1346f63c7..c69686fc1 100644 --- a/ipa-core/src/test_fixture/logging.rs +++ b/ipa-core/src/test_fixture/logging.rs @@ -1,5 +1,7 @@ use std::sync::Once; +use tracing_subscriber::fmt::format::FmtSpan; + /// Set up logging for IPA /// /// ## Panics @@ -30,12 +32,9 @@ pub fn setup() { Level::INFO.into() }; - let fmt_layer = fmt::layer().with_test_writer(); - #[cfg(not(feature = "step-trace"))] - let fmt_layer = { - use tracing_subscriber::fmt::format::FmtSpan; - fmt_layer.with_span_events(FmtSpan::NEW | FmtSpan::CLOSE) - }; + let fmt_layer = fmt::layer() + .with_test_writer() + .with_span_events(FmtSpan::NEW | FmtSpan::CLOSE); tracing_subscriber::registry() .with( diff --git a/ipa-core/src/test_fixture/world.rs b/ipa-core/src/test_fixture/world.rs index 5d2585af8..6fba504cf 100644 --- a/ipa-core/src/test_fixture/world.rs +++ b/ipa-core/src/test_fixture/world.rs @@ -250,7 +250,7 @@ impl TestWorld { impl Drop for TestWorld { fn drop(&mut self) { - if tracing::span_enabled!(Level::DEBUG) || cfg!(feature = "step-trace") { + if tracing::span_enabled!(Level::DEBUG) { let metrics = self.metrics_handle.snapshot(); metrics.export(&mut stdout()).unwrap(); } @@ -266,7 +266,6 @@ impl TestWorld { pub fn with_config(config: &TestWorldConfig) -> Self { logging::setup(); // Print to stdout so that it appears in test runs only on failure. - // scripts/collect_steps.py must be updated if the message text changes. println!("TestWorld random seed {seed}", seed = config.seed); let shard_count = ShardIndex::try_from(S::SHARDS).unwrap(); diff --git a/ipa-step/Cargo.toml b/ipa-step/Cargo.toml index 4f4a4c0b5..05b06abbf 100644 --- a/ipa-step/Cargo.toml +++ b/ipa-step/Cargo.toml @@ -8,10 +8,8 @@ edition = "2021" build = ["name", "prettyplease", "proc-macro2", "quote", "syn"] name = [] string-step = [] -trace = ["metrics"] [dependencies] -metrics = { version = "0.21.0", optional = true } prettyplease = { version = "0.2", optional = true } proc-macro2 = { version = "1", optional = true } quote = { version = "1.0.36", optional = true } diff --git a/ipa-step/src/descriptive.rs b/ipa-step/src/descriptive.rs index d1eb4a08b..3cc8f0482 100644 --- a/ipa-step/src/descriptive.rs +++ b/ipa-step/src/descriptive.rs @@ -87,16 +87,7 @@ impl StepNarrow for Descriptive { assert!(!s.contains('/'), "The string for a step cannot contain '/'"); } - let mut id = self.id.clone() + "/"; - #[cfg(feature = "trace")] - { - id += [std::any::type_name::(), "::"].concat().as_ref(); - } - id += step.as_ref(); - #[cfg(feature = "trace")] - { - metrics::increment_counter!(STEP_NARROWED, STEP => id.clone()); - } + let id = format!("{}/{}", self.id, step.as_ref()); Self { id } } diff --git a/scripts/collect_steps.py b/scripts/collect_steps.py deleted file mode 100755 index 30ef933eb..000000000 --- a/scripts/collect_steps.py +++ /dev/null @@ -1,178 +0,0 @@ -#!/usr/bin/env python3 -import argparse -import os -import re -import subprocess -import sys - -# This script collects all the steps that are executed in the oneshot_ipa with -# all possible configurations. - -IPA_ENV = [["RUST_LOG", "WARN,ipa_core::test_fixture::metrics=DEBUG"]] -ROOT_STEP_PREFIX = "protocol/ipa_core::test_fixture::world::TestExecutionStep::iter0" -SECURITY_MODEL = "semi-honest" - -# TODO(taikiy): #771 allows us to remove this synthetic step generation code - -# There are protocols in IPA that that will generate log(N) steps where N is the number -# of input rows to IPA. In this script, we execute IPA with 10 input rows, hence it -# only generates log2(10) (maybe a few more/less because of the optimizations) dynamic -# steps. Our goal is to generate 32 sets of these steps. (32 > log2(1B)) -# We do this by replacing all "depth\d+" in the steps with "depthX", store them in the -# set, and later replace X with the depth of the iteration [0..32). We do that because -# there are more optimizations done at row index level (i.e., skip the multiplication -# for the last row), so the number of sub-steps branching off at each depth may differ. -# To workaround this issue, we do the "depthX" replacement and collect all possible -# steps and sub-steps (log2(10) steps should be enough to generate all possible -# combinations). That means the generated `Compact` gate code will contain state -# transitions that are not actually executed. This is not optimal, but not a big deal. -# It's impossible to generate the exact set of steps that are executed in the actual -# protocol without executing the protocol or analyzing the code statically. -DEPTH_DYNAMIC_STEPS = [ - "ipa_core::protocol::attribution::InteractionPatternStep", -] -MAXIMUM_DEPTH = 32 - - -def set_env(): - env = os.environ.copy() - for k, v in IPA_ENV: - env[k] = v - return env - - -def remove_root_step_name_from_line(l): - return l.split(",")[0][len(ROOT_STEP_PREFIX) + 1 :] - - -def collect_steps(args): - output = set() - depth_dynamic_steps = set() - - proc = subprocess.Popen( - args=args, - env=set_env(), - stdout=subprocess.PIPE, - bufsize=1, - universal_newlines=True, - ) - - count = 0 - while True: - line = proc.stdout.readline() - - if not line or line == "": - break - - if line.startswith("TestWorld random seed "): - continue - - if not line.startswith(ROOT_STEP_PREFIX): - print("Unexpected line: " + line, flush=True) - exit(1) - - count += 1 - - if any(s in line for s in DEPTH_DYNAMIC_STEPS): - line = re.sub(r"depth\d+", "depthX", line) - depth_dynamic_steps.add(remove_root_step_name_from_line(line)) - # continue without adding to the `output`. we'll generate the dynamic steps later - continue - - output.update([remove_root_step_name_from_line(line)]) - - # safeguard against empty output - if count == 0: - print("No steps in the output", flush=True) - exit(1) - - # generate dynamic steps - for i in range(MAXIMUM_DEPTH): - for s in depth_dynamic_steps: - line = re.sub(r"depthX", "depth" + str(i), s) - output.add(line) - - return output - - -# Splits a line by "/" and create a vector consisting each splitted string -# concatenated by all the preceding strings. -# -# # Example -# input = "mod_conv_match_key/mc0/mc0/xor1" -# output = ["mod_conv_match_key", -# "mod_conv_match_key/mc0", -# "mod_conv_match_key/mc0/mc0", -# "mod_conv_match_key/mc0/mc0/xor1"] -# -# We do this because not all substeps invoke communications between helpers. -# -# For example, a leaf substep "mod_conv_match_key/mc0/mc0/xor1" invokes -# multiplications, but "mod_conv_match_key/mc0/mc0" and "mod_conv_match_key/mc0" -# do not. However, we want to include these intermediate substeps in the output -# since each `narrow`, even if it doesn't actually do anything, is a state -# transition. -# -# This function generates a lot of duplicates. It is inefficient but we don't -# really care because we execute this script only once when we add a new -# protocol which is a pretty rare case. The duplicates will be removed in the -# `remove_duplicates_and_sort`. -def extract_intermediate_steps(steps): - output = set() - for step in steps: - substeps = step.split("/") - for i in range(1, len(substeps)): - output.add("/".join(substeps[:i])) - steps.update(output) - - # remove empty string if present - try: - steps.remove("") - except Exception: - pass - - return steps - - -def ipa_steps(base_args): - output = set() - args = base_args + ["-n", "100", "-c", "8", "-w", "0", "-b", "256", "-m", SECURITY_MODEL, "-t", "7"] - print(" ".join(args), file=sys.stderr) - output.update(collect_steps(args)) - args = base_args + ["-n", "320", "-c", "256", "-w", "86400", "-b", "32", "-m", SECURITY_MODEL, "-t", "255"] - print(" ".join(args), file=sys.stderr) - output.update(collect_steps(args)) - return output - - -if __name__ == "__main__": - parser = argparse.ArgumentParser(description="Generate steps file") - parser.add_argument( - "-m", - "--multi-threading", - action="store_true", - ) - args = parser.parse_args() - - features = ["enable-benches", "debug-trace", "step-trace"] - if args.multi_threading: - features.append("multi-threading") - - ARGS = [ - "cargo", - "bench", - "--bench", - "oneshot_ipa", - "--no-default-features", - f'--features={" ".join(features)}', - '--', - ] - - steps = set() - steps.update(ipa_steps(ARGS)) - - full_steps = extract_intermediate_steps(steps) - sorted_steps = sorted(full_steps) - - for step in sorted_steps: - print(step) From 411c7717ed6b6e594a1d072642170853c963c20c Mon Sep 17 00:00:00 2001 From: Alex Koshelev Date: Wed, 4 Sep 2024 09:14:10 -0700 Subject: [PATCH 2/2] Fix benchmark build --- ipa-core/benches/oneshot/ipa.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ipa-core/benches/oneshot/ipa.rs b/ipa-core/benches/oneshot/ipa.rs index 6ae4aff44..02f6e0304 100644 --- a/ipa-core/benches/oneshot/ipa.rs +++ b/ipa-core/benches/oneshot/ipa.rs @@ -1,6 +1,6 @@ use std::{ env, - num::{NonZeroU32, NonZeroU64, NonZeroUsize}, + num::{NonZeroU32, NonZeroUsize}, time::Instant, };