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..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, }; @@ -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)