From f48420b2c67f7672f22d2359c352f38038635675 Mon Sep 17 00:00:00 2001 From: Emanuele Stoppa Date: Mon, 31 Jan 2022 14:35:06 +0000 Subject: [PATCH 1/8] wip --- xtask/bench/src/features/formatter.rs | 71 +++++++++++++ xtask/bench/src/features/mod.rs | 2 + xtask/bench/src/features/parser.rs | 102 +++++++++++++++++++ xtask/bench/src/lib.rs | 138 +------------------------- xtask/bench/src/utils.rs | 31 ++++++ 5 files changed, 211 insertions(+), 133 deletions(-) create mode 100644 xtask/bench/src/features/formatter.rs create mode 100644 xtask/bench/src/features/mod.rs create mode 100644 xtask/bench/src/features/parser.rs create mode 100644 xtask/bench/src/utils.rs diff --git a/xtask/bench/src/features/formatter.rs b/xtask/bench/src/features/formatter.rs new file mode 100644 index 00000000000..690fec4bf96 --- /dev/null +++ b/xtask/bench/src/features/formatter.rs @@ -0,0 +1,71 @@ +use crate::utils::print_stats_diff; +use std::time::Duration; + +#[derive(Debug, Clone)] +pub struct BenchmarkParseResult { + id: String, + formatting: Duration, +} +pub fn benchmar_format_lib(id: &str, code: &str) -> BenchmarkParseResult { + #[cfg(feature = "dhat-on")] + println!("Start"); + #[cfg(feature = "dhat-on")] + let stats = dhat::get_stats().unwrap(); + + let tokenizer_timer = timing::start(); + let (tokens, mut diagnostics) = rslint_parser::tokenize(code, 0); + let tok_source = rslint_parser::TokenSource::new(code, &tokens); + let tokenization_duration = tokenizer_timer.stop(); + + #[cfg(feature = "dhat-on")] + println!("Tokenizer"); + #[cfg(feature = "dhat-on")] + let stats = print_stats_diff(stats, dhat::get_stats().unwrap()); + + let parser_timer = timing::start(); + let (events, parsing_diags, tokens) = { + let mut parser = + rslint_parser::Parser::new(tok_source, 0, rslint_parser::Syntax::default().script()); + rslint_parser::syntax::program::parse(&mut parser); + let (events, parsing_diags) = parser.finish(); + (events, parsing_diags, tokens) + }; + let parse_duration = parser_timer.stop(); + + #[cfg(feature = "dhat-on")] + println!("Parsed"); + #[cfg(feature = "dhat-on")] + let stats = print_stats_diff(stats, dhat::get_stats().unwrap()); + + let tree_sink_timer = timing::start(); + let mut tree_sink = rslint_parser::LosslessTreeSink::new(code, &tokens); + rslint_parser::process(&mut tree_sink, events, parsing_diags); + let (_green, sink_diags) = tree_sink.finish(); + let tree_sink_duration = tree_sink_timer.stop(); + + #[cfg(feature = "dhat-on")] + println!("Tree-Sink"); + #[cfg(feature = "dhat-on")] + print_stats_diff(stats, dhat::get_stats().unwrap()); + + diagnostics.extend(sink_diags); + BenchmarkParseResult { id: id.to_string() } +} + +impl Display for BenchmarkParseResult { + fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + // let _ = writeln!(f, "\tTokenization: {:>10?}", self.tokenization); + // let _ = writeln!(f, "\tParsing: {:>10?}", self.parsing); + // let _ = writeln!(f, "\tTree_sink: {:>10?}", self.tree_sink); + // let _ = writeln!(f, "\t ----------"); + // let _ = writeln!(f, "\tTotal: {:>10?}", self.total()); + // + // let _ = writeln!(f, "\tDiagnostics"); + // let diagnostics = &self.diagnostics.iter().group_by(|x| x.severity); + // for (severity, items) in diagnostics { + // let _ = writeln!(f, "\t\t{:?}: {}", severity, items.count()); + // } + + Ok(()) + } +} diff --git a/xtask/bench/src/features/mod.rs b/xtask/bench/src/features/mod.rs new file mode 100644 index 00000000000..6d694c3a516 --- /dev/null +++ b/xtask/bench/src/features/mod.rs @@ -0,0 +1,2 @@ +mod formatter; +mod parser; diff --git a/xtask/bench/src/features/parser.rs b/xtask/bench/src/features/parser.rs new file mode 100644 index 00000000000..d83369cc5e4 --- /dev/null +++ b/xtask/bench/src/features/parser.rs @@ -0,0 +1,102 @@ +use crate::utils::print_stats_diff; +use itertools::Itertools; +use rslint_errors::Diagnostic; +use std::fmt::{Display, Formatter}; +use std::ops::Add; +use std::time::Duration; + +#[derive(Debug, Clone)] +pub struct BenchmarkParseResult { + id: String, + tokenization: Duration, + parsing: Duration, + tree_sink: Duration, + diagnostics: Vec, +} + +pub fn benchmark_parse_lib(id: &str, code: &str) -> BenchmarkParseResult { + #[cfg(feature = "dhat-on")] + println!("Start"); + #[cfg(feature = "dhat-on")] + let stats = dhat::get_stats().unwrap(); + + let tokenizer_timer = timing::start(); + let (tokens, mut diagnostics) = rslint_parser::tokenize(code, 0); + let tok_source = rslint_parser::TokenSource::new(code, &tokens); + let tokenization_duration = tokenizer_timer.stop(); + + #[cfg(feature = "dhat-on")] + println!("Tokenizer"); + #[cfg(feature = "dhat-on")] + let stats = print_stats_diff(stats, dhat::get_stats().unwrap()); + + let parser_timer = timing::start(); + let (events, parsing_diags, tokens) = { + let mut parser = + rslint_parser::Parser::new(tok_source, 0, rslint_parser::Syntax::default().script()); + rslint_parser::syntax::program::parse(&mut parser); + let (events, parsing_diags) = parser.finish(); + (events, parsing_diags, tokens) + }; + let parse_duration = parser_timer.stop(); + + #[cfg(feature = "dhat-on")] + println!("Parsed"); + #[cfg(feature = "dhat-on")] + let stats = print_stats_diff(stats, dhat::get_stats().unwrap()); + + let tree_sink_timer = timing::start(); + let mut tree_sink = rslint_parser::LosslessTreeSink::new(code, &tokens); + rslint_parser::process(&mut tree_sink, events, parsing_diags); + let (_green, sink_diags) = tree_sink.finish(); + let tree_sink_duration = tree_sink_timer.stop(); + + #[cfg(feature = "dhat-on")] + println!("Tree-Sink"); + #[cfg(feature = "dhat-on")] + print_stats_diff(stats, dhat::get_stats().unwrap()); + + diagnostics.extend(sink_diags); + BenchmarkParseResult { + id: id.to_string(), + tokenization: tokenization_duration, + parsing: parse_duration, + tree_sink: tree_sink_duration, + diagnostics, + } +} + +impl BenchmarkParseResult { + fn total(&self) -> Duration { + self.tokenization.add(self.parsing).add(self.tree_sink) + } + + pub(crate) fn summary(&self) -> String { + format!( + "{},Total Time,{:?},tokenization,{:?},parsing,{:?},tree_sink,{:?}", + self.id, + self.total(), + self.tokenization, + self.parsing, + self.tree_sink, + ) + } +} + +impl Display for BenchmarkParseResult { + fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + let _ = writeln!(f, "\tTokenization: {:>10?}", self.tokenization); + let _ = writeln!(f, "\tParsing: {:>10?}", self.parsing); + let _ = writeln!(f, "\tTree_sink: {:>10?}", self.tree_sink); + let _ = writeln!(f, "\t ----------"); + let _ = writeln!(f, "\tTotal: {:>10?}", self.total()); + + let _ = writeln!(f, "\tDiagnostics"); + let diagnostics = &self.diagnostics.iter().group_by(|x| x.severity); + for (severity, items) in diagnostics { + let _ = writeln!(f, "\t\t{:?}: {}", severity, items.count()); + } + + Ok(()) + } +} diff --git a/xtask/bench/src/lib.rs b/xtask/bench/src/lib.rs index 094a0ecf4df..825bd116ce5 100644 --- a/xtask/bench/src/lib.rs +++ b/xtask/bench/src/lib.rs @@ -1,8 +1,8 @@ +pub(crate) mod features; +pub(crate) mod utils; + +use crate::features::parser::benchmark_parse_lib; use ansi_rgb::{red, Foreground}; -use itertools::Itertools; -use rslint_errors::Diagnostic; -use std::fmt::{Display, Formatter}; -use std::ops::Add; use std::time::Duration; use std::{path::PathBuf, str::FromStr}; @@ -10,38 +10,6 @@ fn err_to_string(e: E) -> String { format!("{:?}", e) } -#[cfg(feature = "dhat-on")] -fn print_diff(before: dhat::Stats, current: dhat::Stats) -> dhat::Stats { - use humansize::{file_size_opts as options, FileSize}; - - println!("\tMemory"); - if let Some(heap) = ¤t.heap { - println!("\t\tCurrent Blocks: {}", heap.curr_blocks); - println!( - "\t\tCurrent Bytes: {}", - heap.curr_bytes.file_size(options::CONVENTIONAL).unwrap() - ); - println!("\t\tMax Blocks: {}", heap.max_blocks); - println!( - "\t\tMax Bytes: {}", - heap.max_bytes.file_size(options::CONVENTIONAL).unwrap() - ); - } - - println!( - "\t\tTotal Blocks: {}", - current.total_blocks - before.total_blocks - ); - println!( - "\t\tTotal Bytes: {}", - (current.total_bytes - before.total_bytes) - .file_size(options::CONVENTIONAL) - .unwrap() - ); - - current -} - pub fn get_code(lib: &str) -> Result<(String, String), String> { let url = url::Url::from_str(lib).map_err(err_to_string)?; let segments = url @@ -122,7 +90,7 @@ pub fn run(filter: String, criterion: bool, baseline: Option) { rslint_parser::parse_module(code, 0); } - let result = benchmark_lib(&id, code); + let result = benchmark_parse_lib(&id, code); summary.push(result.summary()); println!("Benchmark: {}", lib); @@ -138,99 +106,3 @@ pub fn run(filter: String, criterion: bool, baseline: Option) { println!("{}", l); } } - -fn benchmark_lib(id: &str, code: &str) -> BenchmarkResult { - #[cfg(feature = "dhat-on")] - println!("Start"); - #[cfg(feature = "dhat-on")] - let stats = dhat::get_stats().unwrap(); - - let tokenizer_timer = timing::start(); - let (tokens, mut diagnostics) = rslint_parser::tokenize(code, 0); - let tok_source = rslint_parser::TokenSource::new(code, &tokens); - let tokenization_duration = tokenizer_timer.stop(); - - #[cfg(feature = "dhat-on")] - println!("Tokenizer"); - #[cfg(feature = "dhat-on")] - let stats = print_diff(stats, dhat::get_stats().unwrap()); - - let parser_timer = timing::start(); - let (events, parsing_diags, tokens) = { - let mut parser = - rslint_parser::Parser::new(tok_source, 0, rslint_parser::Syntax::default().script()); - rslint_parser::syntax::program::parse(&mut parser); - let (events, parsing_diags) = parser.finish(); - (events, parsing_diags, tokens) - }; - let parse_duration = parser_timer.stop(); - - #[cfg(feature = "dhat-on")] - println!("Parsed"); - #[cfg(feature = "dhat-on")] - let stats = print_diff(stats, dhat::get_stats().unwrap()); - - let tree_sink_timer = timing::start(); - let mut tree_sink = rslint_parser::LosslessTreeSink::new(code, &tokens); - rslint_parser::process(&mut tree_sink, events, parsing_diags); - let (_green, sink_diags) = tree_sink.finish(); - let tree_sink_duration = tree_sink_timer.stop(); - - #[cfg(feature = "dhat-on")] - println!("Tree-Sink"); - #[cfg(feature = "dhat-on")] - print_diff(stats, dhat::get_stats().unwrap()); - - diagnostics.extend(sink_diags); - BenchmarkResult { - id: id.to_string(), - tokenization: tokenization_duration, - parsing: parse_duration, - tree_sink: tree_sink_duration, - diagnostics, - } -} - -#[derive(Debug, Clone)] -struct BenchmarkResult { - id: String, - tokenization: Duration, - parsing: Duration, - tree_sink: Duration, - diagnostics: Vec, -} - -impl BenchmarkResult { - fn total(&self) -> Duration { - self.tokenization.add(self.parsing).add(self.tree_sink) - } - - fn summary(&self) -> String { - format!( - "{},Total Time,{:?},tokenization,{:?},parsing,{:?},tree_sink,{:?}", - self.id, - self.total(), - self.tokenization, - self.parsing, - self.tree_sink, - ) - } -} - -impl Display for BenchmarkResult { - fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { - let _ = writeln!(f, "\tTokenization: {:>10?}", self.tokenization); - let _ = writeln!(f, "\tParsing: {:>10?}", self.parsing); - let _ = writeln!(f, "\tTree_sink: {:>10?}", self.tree_sink); - let _ = writeln!(f, "\t ----------"); - let _ = writeln!(f, "\tTotal: {:>10?}", self.total()); - - let _ = writeln!(f, "\tDiagnostics"); - let diagnostics = &self.diagnostics.iter().group_by(|x| x.severity); - for (severity, items) in diagnostics { - let _ = writeln!(f, "\t\t{:?}: {}", severity, items.count()); - } - - Ok(()) - } -} diff --git a/xtask/bench/src/utils.rs b/xtask/bench/src/utils.rs new file mode 100644 index 00000000000..13f8a2fb4ff --- /dev/null +++ b/xtask/bench/src/utils.rs @@ -0,0 +1,31 @@ +#[cfg(feature = "dhat-on")] +pub fn print_stats_diff(before: dhat::Stats, current: dhat::Stats) -> dhat::Stats { + use humansize::{file_size_opts as options, FileSize}; + + println!("\tMemory"); + if let Some(heap) = ¤t.heap { + println!("\t\tCurrent Blocks: {}", heap.curr_blocks); + println!( + "\t\tCurrent Bytes: {}", + heap.curr_bytes.file_size(options::CONVENTIONAL).unwrap() + ); + println!("\t\tMax Blocks: {}", heap.max_blocks); + println!( + "\t\tMax Bytes: {}", + heap.max_bytes.file_size(options::CONVENTIONAL).unwrap() + ); + } + + println!( + "\t\tTotal Blocks: {}", + current.total_blocks - before.total_blocks + ); + println!( + "\t\tTotal Bytes: {}", + (current.total_bytes - before.total_bytes) + .file_size(options::CONVENTIONAL) + .unwrap() + ); + + current +} From 25fff3463e887012002f55c25efbdd1a8ce4902d Mon Sep 17 00:00:00 2001 From: Emanuele Stoppa Date: Mon, 31 Jan 2022 17:39:18 +0000 Subject: [PATCH 2/8] ci: benchmarking formatter --- .../{bench.yml => bench_formatter.yml} | 8 +- .github/workflows/bench_parser.yml | 85 +++++++++++++ CONTRIBUTING.md | 3 +- Cargo.lock | 1 + .../src/ts/expressions/literal_expression.rs | 4 +- .../rome_formatter/src/ts/expressions/mod.rs | 1 + .../src/ts/expressions/regex_literal.rs | 8 ++ xtask/bench/Cargo.toml | 1 + xtask/bench/README.md | 18 ++- xtask/bench/src/features/formatter.rs | 86 +++++-------- xtask/bench/src/features/mod.rs | 4 +- xtask/bench/src/features/parser.rs | 47 ++++++-- xtask/bench/src/lib.rs | 114 +++++++++++------- xtask/bench/src/main.rs | 15 ++- xtask/bench/src/utils.rs | 69 ++++++----- 15 files changed, 313 insertions(+), 151 deletions(-) rename .github/workflows/{bench.yml => bench_formatter.yml} (96%) create mode 100644 .github/workflows/bench_parser.yml create mode 100644 crates/rome_formatter/src/ts/expressions/regex_literal.rs diff --git a/.github/workflows/bench.yml b/.github/workflows/bench_formatter.yml similarity index 96% rename from .github/workflows/bench.yml rename to .github/workflows/bench_formatter.yml index 27afa454948..844180424f6 100644 --- a/.github/workflows/bench.yml +++ b/.github/workflows/bench_formatter.yml @@ -1,7 +1,7 @@ # Parser benchmark, compares main and PR branch with Criterion. # Comment with text containing `!bench`, a new result will be commented at the bottom of this PR. -name: Parser Benchmark +name: Formatter Benchmark on: issue_comment: @@ -14,13 +14,13 @@ env: jobs: bench: name: Bench - if: github.event.issue.pull_request && contains(github.event.comment.body, '!bench') + if: github.event.issue.pull_request && contains(github.event.comment.body, '!bench_formatter') runs-on: ${{ matrix.os }} strategy: fail-fast: false matrix: - os: [ubuntu-latest] + os: [ubuntu-latest] steps: - name: Checkout PR Branch @@ -30,7 +30,7 @@ jobs: - name: Support longpaths run: git config core.longpaths true - + - name: Checkout PR Branch uses: actions/checkout@v2 diff --git a/.github/workflows/bench_parser.yml b/.github/workflows/bench_parser.yml new file mode 100644 index 00000000000..43cb4f53ff1 --- /dev/null +++ b/.github/workflows/bench_parser.yml @@ -0,0 +1,85 @@ +# Parser benchmark, compares main and PR branch with Criterion. +# Comment with text containing `!bench`, a new result will be commented at the bottom of this PR. + +name: Parser Benchmark + +on: + issue_comment: + types: [created] + +env: + RUST_LOG: info + RUST_BACKTRACE: 1 + +jobs: + bench: + name: Bench + if: github.event.issue.pull_request && contains(github.event.comment.body, '!bench_parser') + runs-on: ${{ matrix.os }} + + strategy: + fail-fast: false + matrix: + os: [ubuntu-latest] + + steps: + - name: Checkout PR Branch + uses: actions/checkout@v2 + with: + submodules: false + + - name: Support longpaths + run: git config core.longpaths true + + - name: Checkout PR Branch + uses: actions/checkout@v2 + + - name: Fetch Main Branch + run: git fetch --no-tags --prune --no-recurse-submodules --depth=1 origin main + + - name: Install toolchain + run: rustup show + + - name: Cache + uses: Swatinem/rust-cache@v1 + + - name: Install critcmp + run: cargo install critcmp + + - name: Compile + run: cargo build --release --locked -p xtask_bench + + - name: Run Bench on PR Branch + run: cargo benchmark --save-baseline pr + + - name: Checkout Main Branch + run: git checkout main + + - name: Run Bench on Main Branch + run: cargo benchmark --save-baseline main + + - name: Compare Bench Results on ${{ matrix.os }} + id: bench_comparison + shell: bash + run: | + echo "### Bench results on ${{ matrix.os }} of the parser" > output + echo "\`\`\`" >> output + critcmp main pr >> output + echo "\`\`\`" >> output + cat output + comment="$(cat output)" + comment="${comment//'%'/'%25'}" + comment="${comment//$'\n'/'%0A'}" + comment="${comment//$'\r'/'%0D'}" + echo "::set-output name=comment::$comment" + + - name: Write a new comment + uses: peter-evans/create-or-update-comment@v1.4.5 + continue-on-error: true + with: + issue-number: ${{ github.event.issue.number }} + body: | + ${{ steps.bench_comparison.outputs.comment }} + + - name: Remove Criterion Artifact + run: rm -rf ./target/criterion diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index cfa5ca60bf0..c6e36d11fee 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -99,7 +99,8 @@ Here are some other scripts that you might find useful. If you are a core contributor, and you have access to create new branches from the main repository (not a fork), use these comments to run specific workflows: -- `!bench` benchmarks the parser's runtime performance and writes a comment with the results +- `!bench_parser` benchmarks the parser's runtime performance and writes a comment with the results; +- `!bench_formatter` benchmarks the formatter runtime performance and writes a comment with the results; #### Naming patterns diff --git a/Cargo.lock b/Cargo.lock index 78d14d9d400..71aedd8f927 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2025,6 +2025,7 @@ dependencies = [ "itertools", "pico-args", "regex", + "rome_formatter", "rslint_errors", "rslint_parser", "timing", diff --git a/crates/rome_formatter/src/ts/expressions/literal_expression.rs b/crates/rome_formatter/src/ts/expressions/literal_expression.rs index 884aaa24613..40d78ce71fd 100644 --- a/crates/rome_formatter/src/ts/expressions/literal_expression.rs +++ b/crates/rome_formatter/src/ts/expressions/literal_expression.rs @@ -64,7 +64,9 @@ impl ToFormatElement for JsAnyLiteralExpression { JsAnyLiteralExpression::JsNullLiteralExpression(null_literal) => { null_literal.to_format_element(formatter) } - JsAnyLiteralExpression::JsRegexLiteralExpression(_) => todo!(), + JsAnyLiteralExpression::JsRegexLiteralExpression(node) => { + node.to_format_element(formatter) + } } } } diff --git a/crates/rome_formatter/src/ts/expressions/mod.rs b/crates/rome_formatter/src/ts/expressions/mod.rs index 3a4b83f64f7..142b745529e 100644 --- a/crates/rome_formatter/src/ts/expressions/mod.rs +++ b/crates/rome_formatter/src/ts/expressions/mod.rs @@ -7,6 +7,7 @@ mod function_expression; mod identifier_expression; mod literal_expression; mod object_expression; +mod regex_literal; mod sequence_expression; mod static_member_expression; mod super_expression; diff --git a/crates/rome_formatter/src/ts/expressions/regex_literal.rs b/crates/rome_formatter/src/ts/expressions/regex_literal.rs new file mode 100644 index 00000000000..6253e8672ee --- /dev/null +++ b/crates/rome_formatter/src/ts/expressions/regex_literal.rs @@ -0,0 +1,8 @@ +use crate::{FormatElement, FormatResult, Formatter, ToFormatElement}; +use rslint_parser::ast::JsRegexLiteralExpression; + +impl ToFormatElement for JsRegexLiteralExpression { + fn to_format_element(&self, formatter: &Formatter) -> FormatResult { + formatter.format_token(&self.value_token()?) + } +} diff --git a/xtask/bench/Cargo.toml b/xtask/bench/Cargo.toml index 24ea161ceaf..d09f7f8cd79 100644 --- a/xtask/bench/Cargo.toml +++ b/xtask/bench/Cargo.toml @@ -8,6 +8,7 @@ publish = false xtask = { path = '../', version = "0.0" } rslint_parser = { path = "../../crates/rslint_parser", version = "0.3" } rslint_errors = { path = "../../crates/rslint_errors", version = "0.2.0" } +rome_formatter = { path = "../../crates/rome_formatter", version = "0.0.0" } pico-args = "0.3.4" timing = "0.2.3" diff --git a/xtask/bench/README.md b/xtask/bench/README.md index cba5712aaeb..6fba19e4544 100644 --- a/xtask/bench/README.md +++ b/xtask/bench/README.md @@ -2,10 +2,11 @@ This crate contains benchmark suites for the project. +Criterion is used to generate benchmark results. + ## Parser Benchmark -Criterion is used to generate benchmark results, -you can use the following instruction to get nice benchmark comparison. +To get a benchmark comparison, you need to run the benchmark for `main` branch and your PR: ```bash # (commit your code on pr branch, run) @@ -36,6 +37,19 @@ parser/vue.global.prod.js 1.09 28.7±6.39ms 4.2 MB/sec 1 The 1.xx column is the percentage difference, larger means worse. For example jquery is 16% slower on main. And the pr branch performs better overall. +## Formatter benchmark + +To get a benchmark comparison, you need to run the benchmark for `main` branch and your PR: + +```bash +# (commit your code on pr branch, run) +git checkout main +cargo benchmark --save-baseline main --feature formatter +git checkout - +cargo benchmark --save-baseline pr --feature formatter +critcmp main pr # (cargo install critcmp) +``` + ## Heap Profiling using `dhat` ```bash diff --git a/xtask/bench/src/features/formatter.rs b/xtask/bench/src/features/formatter.rs index 690fec4bf96..66d68cf45d2 100644 --- a/xtask/bench/src/features/formatter.rs +++ b/xtask/bench/src/features/formatter.rs @@ -1,71 +1,45 @@ -use crate::utils::print_stats_diff; +use crate::BenchSummary; +use rome_formatter::{format, FormatOptions}; +use rslint_parser::{parse, Syntax}; +use std::fmt::{Display, Formatter}; use std::time::Duration; #[derive(Debug, Clone)] -pub struct BenchmarkParseResult { +pub struct BenchmarkFormatterResult { id: String, formatting: Duration, } -pub fn benchmar_format_lib(id: &str, code: &str) -> BenchmarkParseResult { - #[cfg(feature = "dhat-on")] - println!("Start"); - #[cfg(feature = "dhat-on")] - let stats = dhat::get_stats().unwrap(); - - let tokenizer_timer = timing::start(); - let (tokens, mut diagnostics) = rslint_parser::tokenize(code, 0); - let tok_source = rslint_parser::TokenSource::new(code, &tokens); - let tokenization_duration = tokenizer_timer.stop(); - - #[cfg(feature = "dhat-on")] - println!("Tokenizer"); - #[cfg(feature = "dhat-on")] - let stats = print_stats_diff(stats, dhat::get_stats().unwrap()); - - let parser_timer = timing::start(); - let (events, parsing_diags, tokens) = { - let mut parser = - rslint_parser::Parser::new(tok_source, 0, rslint_parser::Syntax::default().script()); - rslint_parser::syntax::program::parse(&mut parser); - let (events, parsing_diags) = parser.finish(); - (events, parsing_diags, tokens) - }; - let parse_duration = parser_timer.stop(); - - #[cfg(feature = "dhat-on")] - println!("Parsed"); - #[cfg(feature = "dhat-on")] - let stats = print_stats_diff(stats, dhat::get_stats().unwrap()); - - let tree_sink_timer = timing::start(); - let mut tree_sink = rslint_parser::LosslessTreeSink::new(code, &tokens); - rslint_parser::process(&mut tree_sink, events, parsing_diags); - let (_green, sink_diags) = tree_sink.finish(); - let tree_sink_duration = tree_sink_timer.stop(); +pub fn benchmark_format_lib(id: &str, code: &str) -> BenchSummary { + let syntax = Syntax::default().module(); + let root = parse(code, 0, syntax).syntax(); + + let formatter_timer = timing::start(); + let _ = format(FormatOptions::default(), &root); + let formatter_duration = formatter_timer.stop(); + + BenchSummary::Formatter(BenchmarkFormatterResult { + id: id.to_string(), + formatting: formatter_duration, + }) +} - #[cfg(feature = "dhat-on")] - println!("Tree-Sink"); - #[cfg(feature = "dhat-on")] - print_stats_diff(stats, dhat::get_stats().unwrap()); +impl BenchmarkFormatterResult { + fn total(&self) -> Duration { + self.formatting + } - diagnostics.extend(sink_diags); - BenchmarkParseResult { id: id.to_string() } + pub(crate) fn summary(&self) -> String { + format!("{}, Formatting: {:?}", self.id, self.total(),) + } } -impl Display for BenchmarkParseResult { +impl Display for BenchmarkFormatterResult { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { - // let _ = writeln!(f, "\tTokenization: {:>10?}", self.tokenization); - // let _ = writeln!(f, "\tParsing: {:>10?}", self.parsing); - // let _ = writeln!(f, "\tTree_sink: {:>10?}", self.tree_sink); - // let _ = writeln!(f, "\t ----------"); - // let _ = writeln!(f, "\tTotal: {:>10?}", self.total()); - // - // let _ = writeln!(f, "\tDiagnostics"); - // let diagnostics = &self.diagnostics.iter().group_by(|x| x.severity); - // for (severity, items) in diagnostics { - // let _ = writeln!(f, "\t\t{:?}: {}", severity, items.count()); - // } + let _ = writeln!(f, "\tFormatting: {:>10?}", self.formatting); + let _ = writeln!(f, "\t ----------"); + let _ = writeln!(f, "\tTotal: {:>10?}", self.total()); + // TODO: add diagnostics once the formatter API is able to return them from a formatter error Ok(()) } } diff --git a/xtask/bench/src/features/mod.rs b/xtask/bench/src/features/mod.rs index 6d694c3a516..f19d4aa918e 100644 --- a/xtask/bench/src/features/mod.rs +++ b/xtask/bench/src/features/mod.rs @@ -1,2 +1,2 @@ -mod formatter; -mod parser; +pub mod formatter; +pub mod parser; diff --git a/xtask/bench/src/features/parser.rs b/xtask/bench/src/features/parser.rs index d83369cc5e4..20a12928316 100644 --- a/xtask/bench/src/features/parser.rs +++ b/xtask/bench/src/features/parser.rs @@ -1,4 +1,4 @@ -use crate::utils::print_stats_diff; +use crate::BenchSummary; use itertools::Itertools; use rslint_errors::Diagnostic; use std::fmt::{Display, Formatter}; @@ -14,7 +14,38 @@ pub struct BenchmarkParseResult { diagnostics: Vec, } -pub fn benchmark_parse_lib(id: &str, code: &str) -> BenchmarkParseResult { +#[cfg(feature = "dhat-on")] +fn print_diff(before: dhat::Stats, current: dhat::Stats) -> dhat::Stats { + use humansize::{file_size_opts as options, FileSize}; + + println!("\tMemory"); + if let Some(heap) = ¤t.heap { + println!("\t\tCurrent Blocks: {}", heap.curr_blocks); + println!( + "\t\tCurrent Bytes: {}", + heap.curr_bytes.file_size(options::CONVENTIONAL).unwrap() + ); + println!("\t\tMax Blocks: {}", heap.max_blocks); + println!( + "\t\tMax Bytes: {}", + heap.max_bytes.file_size(options::CONVENTIONAL).unwrap() + ); + } + + println!( + "\t\tTotal Blocks: {}", + current.total_blocks - before.total_blocks + ); + println!( + "\t\tTotal Bytes: {}", + (current.total_bytes - before.total_bytes) + .file_size(options::CONVENTIONAL) + .unwrap() + ); + + current +} +pub fn benchmark_parse_lib(id: &str, code: &str) -> BenchSummary { #[cfg(feature = "dhat-on")] println!("Start"); #[cfg(feature = "dhat-on")] @@ -28,7 +59,7 @@ pub fn benchmark_parse_lib(id: &str, code: &str) -> BenchmarkParseResult { #[cfg(feature = "dhat-on")] println!("Tokenizer"); #[cfg(feature = "dhat-on")] - let stats = print_stats_diff(stats, dhat::get_stats().unwrap()); + let stats = print_diff(stats, dhat::get_stats().unwrap()); let parser_timer = timing::start(); let (events, parsing_diags, tokens) = { @@ -43,7 +74,7 @@ pub fn benchmark_parse_lib(id: &str, code: &str) -> BenchmarkParseResult { #[cfg(feature = "dhat-on")] println!("Parsed"); #[cfg(feature = "dhat-on")] - let stats = print_stats_diff(stats, dhat::get_stats().unwrap()); + let stats = print_diff(stats, dhat::get_stats().unwrap()); let tree_sink_timer = timing::start(); let mut tree_sink = rslint_parser::LosslessTreeSink::new(code, &tokens); @@ -54,16 +85,16 @@ pub fn benchmark_parse_lib(id: &str, code: &str) -> BenchmarkParseResult { #[cfg(feature = "dhat-on")] println!("Tree-Sink"); #[cfg(feature = "dhat-on")] - print_stats_diff(stats, dhat::get_stats().unwrap()); + print_diff(stats, dhat::get_stats().unwrap()); diagnostics.extend(sink_diags); - BenchmarkParseResult { + BenchSummary::Parser(BenchmarkParseResult { id: id.to_string(), tokenization: tokenization_duration, parsing: parse_duration, tree_sink: tree_sink_duration, diagnostics, - } + }) } impl BenchmarkParseResult { @@ -73,7 +104,7 @@ impl BenchmarkParseResult { pub(crate) fn summary(&self) -> String { format!( - "{},Total Time,{:?},tokenization,{:?},parsing,{:?},tree_sink,{:?}", + "{}, Total Time: {:?}, tokenization: {:?}, parsing: {:?}, tree_sink: {:?}", self.id, self.total(), self.tokenization, diff --git a/xtask/bench/src/lib.rs b/xtask/bench/src/lib.rs index 825bd116ce5..3cc1d429451 100644 --- a/xtask/bench/src/lib.rs +++ b/xtask/bench/src/lib.rs @@ -1,58 +1,76 @@ -pub(crate) mod features; -pub(crate) mod utils; +mod features; +mod utils; -use crate::features::parser::benchmark_parse_lib; -use ansi_rgb::{red, Foreground}; +use std::fmt::{Display, Formatter}; +use std::str::FromStr; use std::time::Duration; -use std::{path::PathBuf, str::FromStr}; -fn err_to_string(e: E) -> String { - format!("{:?}", e) +pub use crate::features::formatter::benchmark_format_lib; +use crate::features::formatter::BenchmarkFormatterResult; +pub use crate::features::parser::benchmark_parse_lib; +use crate::features::parser::BenchmarkParseResult; +pub use utils::get_code; + +/// What feature to benchmark +pub enum FeatureToBenchmark { + /// benchmark of the parser + Parser, + /// benchmark of the formatter + Formatter, } -pub fn get_code(lib: &str) -> Result<(String, String), String> { - let url = url::Url::from_str(lib).map_err(err_to_string)?; - let segments = url - .path_segments() - .ok_or_else(|| "lib url has no segments".to_string())?; - let filename = segments - .last() - .ok_or_else(|| "lib url has no segments".to_string())?; - - let mut file = PathBuf::from_str("target").map_err(err_to_string)?; - file.push(filename); - - match std::fs::read_to_string(&file) { - Ok(code) => { - println!("[{}] - using [{}]", filename.fg(red()), file.display()); - Ok((filename.to_string(), code)) +impl FromStr for FeatureToBenchmark { + type Err = (); + + fn from_str(s: &str) -> Result { + match s { + "parser" => Ok(Self::Parser), + "formatter" => Ok(Self::Formatter), + _ => Ok(Self::Parser), } - Err(_) => { - println!( - "[{}] - Downloading [{}] to [{}]", - filename, - lib, - file.display() - ); - match ureq::get(lib).call() { - Ok(response) => { - let mut reader = response.into_reader(); - - let _ = std::fs::remove_file(&file); - let mut writer = std::fs::File::create(&file).map_err(err_to_string)?; - let _ = std::io::copy(&mut reader, &mut writer); - - std::fs::read_to_string(&file) - .map_err(err_to_string) - .map(|code| (filename.to_string(), code)) - } - Err(e) => Err(format!("{:?}", e)), - } + } +} + +impl Display for FeatureToBenchmark { + fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + let _ = match self { + FeatureToBenchmark::Parser => writeln!(f, "parser"), + FeatureToBenchmark::Formatter => writeln!(f, "formatter"), + }; + Ok(()) + } +} + +/// If groups the summary by their category and creates a small interface +/// where each bench result can create their summary +pub enum BenchSummary { + Parser(BenchmarkParseResult), + Formatter(BenchmarkFormatterResult), +} + +impl BenchSummary { + pub fn summary(&self) -> String { + match self { + BenchSummary::Parser(result) => result.summary(), + BenchSummary::Formatter(result) => result.summary(), + } + } +} + +impl Display for BenchSummary { + fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + match self { + BenchSummary::Parser(result) => std::fmt::Display::fmt(&result, f), + BenchSummary::Formatter(result) => std::fmt::Display::fmt(&result, f), } } } -pub fn run(filter: String, criterion: bool, baseline: Option) { +fn err_to_string(e: E) -> String { + format!("{:?}", e) +} + +pub fn run(filter: String, criterion: bool, baseline: Option, feature: FeatureToBenchmark) { let regex = regex::Regex::new(filter.as_str()).unwrap(); let libs = include_str!("libs.txt").lines(); @@ -90,7 +108,11 @@ pub fn run(filter: String, criterion: bool, baseline: Option) { rslint_parser::parse_module(code, 0); } - let result = benchmark_parse_lib(&id, code); + let result = match feature { + FeatureToBenchmark::Parser => benchmark_parse_lib(&id, code), + FeatureToBenchmark::Formatter => benchmark_format_lib(&id, code), + }; + summary.push(result.summary()); println!("Benchmark: {}", lib); diff --git a/xtask/bench/src/main.rs b/xtask/bench/src/main.rs index c6e258f3eae..5c565c46161 100644 --- a/xtask/bench/src/main.rs +++ b/xtask/bench/src/main.rs @@ -1,7 +1,7 @@ use pico_args::Arguments; +use std::str::FromStr; use xtask::{project_root, pushd, Result}; - -use xtask_bench::run; +use xtask_bench::{run, FeatureToBenchmark}; #[cfg(feature = "dhat-on")] use dhat::DhatAlloc; @@ -32,8 +32,15 @@ fn main() -> Result<()> { .unwrap() .unwrap_or(true); let baseline: Option = args.opt_value_from_str("--save-baseline").unwrap(); - - run(filter, criterion, baseline); + let feature: Option = args.opt_value_from_str("--feature").unwrap(); + + run( + filter, + criterion, + baseline, + FeatureToBenchmark::from_str(feature.unwrap_or_else(|| "parser".to_string()).as_str()) + .unwrap(), + ); Ok(()) } diff --git a/xtask/bench/src/utils.rs b/xtask/bench/src/utils.rs index 13f8a2fb4ff..7b70eef857c 100644 --- a/xtask/bench/src/utils.rs +++ b/xtask/bench/src/utils.rs @@ -1,31 +1,46 @@ -#[cfg(feature = "dhat-on")] -pub fn print_stats_diff(before: dhat::Stats, current: dhat::Stats) -> dhat::Stats { - use humansize::{file_size_opts as options, FileSize}; +use crate::err_to_string; +use ansi_rgb::{red, Foreground}; +use std::path::PathBuf; +use std::str::FromStr; - println!("\tMemory"); - if let Some(heap) = ¤t.heap { - println!("\t\tCurrent Blocks: {}", heap.curr_blocks); - println!( - "\t\tCurrent Bytes: {}", - heap.curr_bytes.file_size(options::CONVENTIONAL).unwrap() - ); - println!("\t\tMax Blocks: {}", heap.max_blocks); - println!( - "\t\tMax Bytes: {}", - heap.max_bytes.file_size(options::CONVENTIONAL).unwrap() - ); - } +pub fn get_code(lib: &str) -> Result<(String, String), String> { + let url = url::Url::from_str(lib).map_err(err_to_string)?; + let segments = url + .path_segments() + .ok_or_else(|| "lib url has no segments".to_string())?; + let filename = segments + .last() + .ok_or_else(|| "lib url has no segments".to_string())?; + + let mut file = PathBuf::from_str("target").map_err(err_to_string)?; + file.push(filename); - println!( - "\t\tTotal Blocks: {}", - current.total_blocks - before.total_blocks - ); - println!( - "\t\tTotal Bytes: {}", - (current.total_bytes - before.total_bytes) - .file_size(options::CONVENTIONAL) - .unwrap() - ); + match std::fs::read_to_string(&file) { + Ok(code) => { + println!("[{}] - using [{}]", filename.fg(red()), file.display()); + Ok((filename.to_string(), code)) + } + Err(_) => { + println!( + "[{}] - Downloading [{}] to [{}]", + filename, + lib, + file.display() + ); + match ureq::get(lib).call() { + Ok(response) => { + let mut reader = response.into_reader(); - current + let _ = std::fs::remove_file(&file); + let mut writer = std::fs::File::create(&file).map_err(err_to_string)?; + let _ = std::io::copy(&mut reader, &mut writer); + + std::fs::read_to_string(&file) + .map_err(err_to_string) + .map(|code| (filename.to_string(), code)) + } + Err(e) => Err(format!("{:?}", e)), + } + } + } } From 866eae91643ff85b53b7a8ee0d22935b9dc90970 Mon Sep 17 00:00:00 2001 From: Emanuele Stoppa Date: Tue, 1 Feb 2022 11:03:08 +0000 Subject: [PATCH 3/8] code review improvements --- .cargo/config.toml | 3 ++- .github/workflows/bench_formatter.yml | 4 ++-- .github/workflows/bench_parser.yml | 4 ++-- xtask/bench/README.md | 8 +++---- xtask/bench/src/features/formatter.rs | 13 ++++++----- xtask/bench/src/features/parser.rs | 12 +++++------ xtask/bench/src/lib.rs | 31 +++++++++++++-------------- xtask/bench/src/main.rs | 15 ++++--------- xtask/bench/src/utils.rs | 1 - 9 files changed, 41 insertions(+), 50 deletions(-) diff --git a/.cargo/config.toml b/.cargo/config.toml index 91be03414e9..d29df3626dc 100644 --- a/.cargo/config.toml +++ b/.cargo/config.toml @@ -2,5 +2,6 @@ lint = "clippy --workspace --all-targets --verbose -- --deny warnings" format = "fmt --all --verbose" codegen = "run -p xtask_codegen --" -benchmark = "run -p xtask_bench --release --" +bench_parser = "run -p xtask_bench --release -- --feature parser" +bench_formatter = "run -p xtask_bench --release -- --feature formatter" coverage = "run -p xtask_coverage --release --" diff --git a/.github/workflows/bench_formatter.yml b/.github/workflows/bench_formatter.yml index 844180424f6..0ffcbd9c49e 100644 --- a/.github/workflows/bench_formatter.yml +++ b/.github/workflows/bench_formatter.yml @@ -50,13 +50,13 @@ jobs: run: cargo build --release --locked -p xtask_bench - name: Run Bench on PR Branch - run: cargo benchmark --save-baseline pr + run: cargo bench_formatter --save-baseline pr - name: Checkout Main Branch run: git checkout main - name: Run Bench on Main Branch - run: cargo benchmark --save-baseline main + run: cargo bench_formatter --save-baseline main - name: Compare Bench Results on ${{ matrix.os }} id: bench_comparison diff --git a/.github/workflows/bench_parser.yml b/.github/workflows/bench_parser.yml index 43cb4f53ff1..7e566bd6433 100644 --- a/.github/workflows/bench_parser.yml +++ b/.github/workflows/bench_parser.yml @@ -50,13 +50,13 @@ jobs: run: cargo build --release --locked -p xtask_bench - name: Run Bench on PR Branch - run: cargo benchmark --save-baseline pr + run: cargo bench_parser --save-baseline pr - name: Checkout Main Branch run: git checkout main - name: Run Bench on Main Branch - run: cargo benchmark --save-baseline main + run: cargo bench_parser --save-baseline main - name: Compare Bench Results on ${{ matrix.os }} id: bench_comparison diff --git a/xtask/bench/README.md b/xtask/bench/README.md index 6fba19e4544..d282525152a 100644 --- a/xtask/bench/README.md +++ b/xtask/bench/README.md @@ -11,9 +11,9 @@ To get a benchmark comparison, you need to run the benchmark for `main` branch a ```bash # (commit your code on pr branch, run) git checkout main -cargo benchmark --save-baseline main +cargo bench_parser --save-baseline main git checkout - -cargo benchmark --save-baseline pr +cargo bench_parser --save-baseline pr critcmp main pr # (cargo install critcmp) ``` @@ -44,9 +44,9 @@ To get a benchmark comparison, you need to run the benchmark for `main` branch a ```bash # (commit your code on pr branch, run) git checkout main -cargo benchmark --save-baseline main --feature formatter +cargo bench_formatter --save-baseline main git checkout - -cargo benchmark --save-baseline pr --feature formatter +cargo bench_formatter --save-baseline pr critcmp main pr # (cargo install critcmp) ``` diff --git a/xtask/bench/src/features/formatter.rs b/xtask/bench/src/features/formatter.rs index 66d68cf45d2..e6c23f3855c 100644 --- a/xtask/bench/src/features/formatter.rs +++ b/xtask/bench/src/features/formatter.rs @@ -1,15 +1,15 @@ -use crate::BenchSummary; +use crate::BenchmarkSummary; use rome_formatter::{format, FormatOptions}; use rslint_parser::{parse, Syntax}; use std::fmt::{Display, Formatter}; use std::time::Duration; #[derive(Debug, Clone)] -pub struct BenchmarkFormatterResult { +pub struct FormatterMeasurement { id: String, formatting: Duration, } -pub fn benchmark_format_lib(id: &str, code: &str) -> BenchSummary { +pub fn benchmark_format_lib(id: &str, code: &str) -> BenchmarkSummary { let syntax = Syntax::default().module(); let root = parse(code, 0, syntax).syntax(); @@ -17,13 +17,13 @@ pub fn benchmark_format_lib(id: &str, code: &str) -> BenchSummary { let _ = format(FormatOptions::default(), &root); let formatter_duration = formatter_timer.stop(); - BenchSummary::Formatter(BenchmarkFormatterResult { + BenchmarkSummary::Formatter(FormatterMeasurement { id: id.to_string(), formatting: formatter_duration, }) } -impl BenchmarkFormatterResult { +impl FormatterMeasurement { fn total(&self) -> Duration { self.formatting } @@ -33,13 +33,12 @@ impl BenchmarkFormatterResult { } } -impl Display for BenchmarkFormatterResult { +impl Display for FormatterMeasurement { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { let _ = writeln!(f, "\tFormatting: {:>10?}", self.formatting); let _ = writeln!(f, "\t ----------"); let _ = writeln!(f, "\tTotal: {:>10?}", self.total()); - // TODO: add diagnostics once the formatter API is able to return them from a formatter error Ok(()) } } diff --git a/xtask/bench/src/features/parser.rs b/xtask/bench/src/features/parser.rs index 20a12928316..626a80a1e88 100644 --- a/xtask/bench/src/features/parser.rs +++ b/xtask/bench/src/features/parser.rs @@ -1,4 +1,4 @@ -use crate::BenchSummary; +use crate::BenchmarkSummary; use itertools::Itertools; use rslint_errors::Diagnostic; use std::fmt::{Display, Formatter}; @@ -6,7 +6,7 @@ use std::ops::Add; use std::time::Duration; #[derive(Debug, Clone)] -pub struct BenchmarkParseResult { +pub struct ParseMeasurement { id: String, tokenization: Duration, parsing: Duration, @@ -45,7 +45,7 @@ fn print_diff(before: dhat::Stats, current: dhat::Stats) -> dhat::Stats { current } -pub fn benchmark_parse_lib(id: &str, code: &str) -> BenchSummary { +pub fn benchmark_parse_lib(id: &str, code: &str) -> BenchmarkSummary { #[cfg(feature = "dhat-on")] println!("Start"); #[cfg(feature = "dhat-on")] @@ -88,7 +88,7 @@ pub fn benchmark_parse_lib(id: &str, code: &str) -> BenchSummary { print_diff(stats, dhat::get_stats().unwrap()); diagnostics.extend(sink_diags); - BenchSummary::Parser(BenchmarkParseResult { + BenchmarkSummary::Parser(ParseMeasurement { id: id.to_string(), tokenization: tokenization_duration, parsing: parse_duration, @@ -97,7 +97,7 @@ pub fn benchmark_parse_lib(id: &str, code: &str) -> BenchSummary { }) } -impl BenchmarkParseResult { +impl ParseMeasurement { fn total(&self) -> Duration { self.tokenization.add(self.parsing).add(self.tree_sink) } @@ -114,7 +114,7 @@ impl BenchmarkParseResult { } } -impl Display for BenchmarkParseResult { +impl Display for ParseMeasurement { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { let _ = writeln!(f, "\tTokenization: {:>10?}", self.tokenization); let _ = writeln!(f, "\tParsing: {:>10?}", self.parsing); diff --git a/xtask/bench/src/lib.rs b/xtask/bench/src/lib.rs index 3cc1d429451..ad630653cce 100644 --- a/xtask/bench/src/lib.rs +++ b/xtask/bench/src/lib.rs @@ -6,9 +6,9 @@ use std::str::FromStr; use std::time::Duration; pub use crate::features::formatter::benchmark_format_lib; -use crate::features::formatter::BenchmarkFormatterResult; +use crate::features::formatter::FormatterMeasurement; pub use crate::features::parser::benchmark_parse_lib; -use crate::features::parser::BenchmarkParseResult; +use crate::features::parser::ParseMeasurement; pub use utils::get_code; /// What feature to benchmark @@ -20,48 +20,47 @@ pub enum FeatureToBenchmark { } impl FromStr for FeatureToBenchmark { - type Err = (); + type Err = pico_args::Error; fn from_str(s: &str) -> Result { match s { "parser" => Ok(Self::Parser), "formatter" => Ok(Self::Formatter), - _ => Ok(Self::Parser), + _ => Err(pico_args::Error::OptionWithoutAValue("feature")), } } } impl Display for FeatureToBenchmark { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { - let _ = match self { + match self { FeatureToBenchmark::Parser => writeln!(f, "parser"), FeatureToBenchmark::Formatter => writeln!(f, "formatter"), - }; - Ok(()) + } } } /// If groups the summary by their category and creates a small interface /// where each bench result can create their summary -pub enum BenchSummary { - Parser(BenchmarkParseResult), - Formatter(BenchmarkFormatterResult), +pub enum BenchmarkSummary { + Parser(ParseMeasurement), + Formatter(FormatterMeasurement), } -impl BenchSummary { +impl BenchmarkSummary { pub fn summary(&self) -> String { match self { - BenchSummary::Parser(result) => result.summary(), - BenchSummary::Formatter(result) => result.summary(), + BenchmarkSummary::Parser(result) => result.summary(), + BenchmarkSummary::Formatter(result) => result.summary(), } } } -impl Display for BenchSummary { +impl Display for BenchmarkSummary { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { match self { - BenchSummary::Parser(result) => std::fmt::Display::fmt(&result, f), - BenchSummary::Formatter(result) => std::fmt::Display::fmt(&result, f), + BenchmarkSummary::Parser(result) => std::fmt::Display::fmt(&result, f), + BenchmarkSummary::Formatter(result) => std::fmt::Display::fmt(&result, f), } } } diff --git a/xtask/bench/src/main.rs b/xtask/bench/src/main.rs index 5c565c46161..d01b6627b0a 100644 --- a/xtask/bench/src/main.rs +++ b/xtask/bench/src/main.rs @@ -1,5 +1,4 @@ use pico_args::Arguments; -use std::str::FromStr; use xtask::{project_root, pushd, Result}; use xtask_bench::{run, FeatureToBenchmark}; @@ -10,7 +9,7 @@ use dhat::DhatAlloc; #[global_allocator] static ALLOCATOR: DhatAlloc = DhatAlloc; -fn main() -> Result<()> { +fn main() -> Result<(), pico_args::Error> { #[cfg(feature = "dhat-on")] let dhat = dhat::Dhat::start_heap_profiling(); @@ -32,15 +31,9 @@ fn main() -> Result<()> { .unwrap() .unwrap_or(true); let baseline: Option = args.opt_value_from_str("--save-baseline").unwrap(); - let feature: Option = args.opt_value_from_str("--feature").unwrap(); - - run( - filter, - criterion, - baseline, - FeatureToBenchmark::from_str(feature.unwrap_or_else(|| "parser".to_string()).as_str()) - .unwrap(), - ); + // "feature" is a mandatory option and will throw an error if it's missing or incorrect + let feature: FeatureToBenchmark = args.value_from_str("--feature")?; + run(filter, criterion, baseline, feature); Ok(()) } diff --git a/xtask/bench/src/utils.rs b/xtask/bench/src/utils.rs index 7b70eef857c..a5d8b531408 100644 --- a/xtask/bench/src/utils.rs +++ b/xtask/bench/src/utils.rs @@ -31,7 +31,6 @@ pub fn get_code(lib: &str) -> Result<(String, String), String> { Ok(response) => { let mut reader = response.into_reader(); - let _ = std::fs::remove_file(&file); let mut writer = std::fs::File::create(&file).map_err(err_to_string)?; let _ = std::io::copy(&mut reader, &mut writer); From c65040817ea127d7b87cb52852c85becc246c0b6 Mon Sep 17 00:00:00 2001 From: Emanuele Stoppa Date: Tue, 1 Feb 2022 13:30:20 +0000 Subject: [PATCH 4/8] run proper function on criterion --- xtask/bench/src/features/formatter.rs | 12 ++++++++---- xtask/bench/src/features/parser.rs | 7 +++++++ xtask/bench/src/lib.rs | 24 +++++++++++++++++++----- 3 files changed, 34 insertions(+), 9 deletions(-) diff --git a/xtask/bench/src/features/formatter.rs b/xtask/bench/src/features/formatter.rs index e6c23f3855c..c0255273c41 100644 --- a/xtask/bench/src/features/formatter.rs +++ b/xtask/bench/src/features/formatter.rs @@ -10,11 +10,8 @@ pub struct FormatterMeasurement { formatting: Duration, } pub fn benchmark_format_lib(id: &str, code: &str) -> BenchmarkSummary { - let syntax = Syntax::default().module(); - let root = parse(code, 0, syntax).syntax(); - let formatter_timer = timing::start(); - let _ = format(FormatOptions::default(), &root); + run_format(code); let formatter_duration = formatter_timer.stop(); BenchmarkSummary::Formatter(FormatterMeasurement { @@ -23,6 +20,13 @@ pub fn benchmark_format_lib(id: &str, code: &str) -> BenchmarkSummary { }) } +pub fn run_format(code: &str) { + let syntax = Syntax::default().module(); + let root = parse(code, 0, syntax).syntax(); + + let _ = format(FormatOptions::default(), &root); +} + impl FormatterMeasurement { fn total(&self) -> Duration { self.formatting diff --git a/xtask/bench/src/features/parser.rs b/xtask/bench/src/features/parser.rs index 626a80a1e88..949551497c5 100644 --- a/xtask/bench/src/features/parser.rs +++ b/xtask/bench/src/features/parser.rs @@ -1,6 +1,8 @@ use crate::BenchmarkSummary; use itertools::Itertools; use rslint_errors::Diagnostic; +use rslint_parser::ast::JsAnyRoot; +use rslint_parser::{Parse, Syntax}; use std::fmt::{Display, Formatter}; use std::ops::Add; use std::time::Duration; @@ -97,6 +99,11 @@ pub fn benchmark_parse_lib(id: &str, code: &str) -> BenchmarkSummary { }) } +pub fn run_parse(code: &str) -> Parse { + let syntax = Syntax::default().module(); + rslint_parser::parse(code, 0, syntax) +} + impl ParseMeasurement { fn total(&self) -> Duration { self.tokenization.add(self.parsing).add(self.tree_sink) diff --git a/xtask/bench/src/lib.rs b/xtask/bench/src/lib.rs index ad630653cce..82c262e0284 100644 --- a/xtask/bench/src/lib.rs +++ b/xtask/bench/src/lib.rs @@ -6,9 +6,9 @@ use std::str::FromStr; use std::time::Duration; pub use crate::features::formatter::benchmark_format_lib; -use crate::features::formatter::FormatterMeasurement; +use crate::features::formatter::{run_format, FormatterMeasurement}; pub use crate::features::parser::benchmark_parse_lib; -use crate::features::parser::ParseMeasurement; +use crate::features::parser::{run_parse, ParseMeasurement}; pub use utils::get_code; /// What feature to benchmark @@ -94,17 +94,31 @@ pub fn run(filter: String, criterion: bool, baseline: Option, feature: F if let Some(ref baseline) = baseline { criterion = criterion.save_baseline(baseline.to_string()); } - let mut group = criterion.benchmark_group("parser"); + let mut group = criterion.benchmark_group(feature.to_string()); group.throughput(criterion::Throughput::Bytes(code.len() as u64)); group.bench_function(&id, |b| { b.iter(|| { - let _ = criterion::black_box(rslint_parser::parse_module(code, 0)); + let _ = criterion::black_box(match feature { + FeatureToBenchmark::Parser => { + run_parse(code); + } + FeatureToBenchmark::Formatter => { + run_format(code); + } + }); }) }); group.finish(); } else { //warmup - rslint_parser::parse_module(code, 0); + match feature { + FeatureToBenchmark::Parser => { + run_parse(code); + } + FeatureToBenchmark::Formatter => { + run_format(code); + } + } } let result = match feature { From 58c435e37fc75af6b95ea13d130c41b1a190e20b Mon Sep 17 00:00:00 2001 From: Emanuele Stoppa Date: Tue, 1 Feb 2022 13:31:55 +0000 Subject: [PATCH 5/8] handle error when copying file --- xtask/bench/src/utils.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/xtask/bench/src/utils.rs b/xtask/bench/src/utils.rs index a5d8b531408..ce4d4aff0a9 100644 --- a/xtask/bench/src/utils.rs +++ b/xtask/bench/src/utils.rs @@ -32,8 +32,11 @@ pub fn get_code(lib: &str) -> Result<(String, String), String> { let mut reader = response.into_reader(); let mut writer = std::fs::File::create(&file).map_err(err_to_string)?; - let _ = std::io::copy(&mut reader, &mut writer); - + if let Err(err) = std::io::copy(&mut reader, &mut writer) { + drop(writer); + std::fs::remove_file(&file).ok(); + return Err(err_to_string(err)); + } std::fs::read_to_string(&file) .map_err(err_to_string) .map(|code| (filename.to_string(), code)) From 75bcd647e517402dcdfd40d5028bcfb6cbfdb5da Mon Sep 17 00:00:00 2001 From: Emanuele Stoppa Date: Tue, 1 Feb 2022 13:36:20 +0000 Subject: [PATCH 6/8] fix: clippy warnings --- xtask/bench/src/features/formatter.rs | 8 ++++---- xtask/bench/src/lib.rs | 10 +++++----- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/xtask/bench/src/features/formatter.rs b/xtask/bench/src/features/formatter.rs index c0255273c41..3d0ecf354a8 100644 --- a/xtask/bench/src/features/formatter.rs +++ b/xtask/bench/src/features/formatter.rs @@ -1,5 +1,5 @@ use crate::BenchmarkSummary; -use rome_formatter::{format, FormatOptions}; +use rome_formatter::{format, FormatOptions, FormatResult, Formatted}; use rslint_parser::{parse, Syntax}; use std::fmt::{Display, Formatter}; use std::time::Duration; @@ -11,7 +11,7 @@ pub struct FormatterMeasurement { } pub fn benchmark_format_lib(id: &str, code: &str) -> BenchmarkSummary { let formatter_timer = timing::start(); - run_format(code); + run_format(code).unwrap(); let formatter_duration = formatter_timer.stop(); BenchmarkSummary::Formatter(FormatterMeasurement { @@ -20,11 +20,11 @@ pub fn benchmark_format_lib(id: &str, code: &str) -> BenchmarkSummary { }) } -pub fn run_format(code: &str) { +pub fn run_format(code: &str) -> FormatResult { let syntax = Syntax::default().module(); let root = parse(code, 0, syntax).syntax(); - let _ = format(FormatOptions::default(), &root); + format(FormatOptions::default(), &root) } impl FormatterMeasurement { diff --git a/xtask/bench/src/lib.rs b/xtask/bench/src/lib.rs index 82c262e0284..59c26f51ef2 100644 --- a/xtask/bench/src/lib.rs +++ b/xtask/bench/src/lib.rs @@ -98,14 +98,14 @@ pub fn run(filter: String, criterion: bool, baseline: Option, feature: F group.throughput(criterion::Throughput::Bytes(code.len() as u64)); group.bench_function(&id, |b| { b.iter(|| { - let _ = criterion::black_box(match feature { + match feature { FeatureToBenchmark::Parser => { - run_parse(code); + let _ = criterion::black_box(run_parse(code)); } FeatureToBenchmark::Formatter => { - run_format(code); + let _ = criterion::black_box(run_format(code).unwrap()); } - }); + }; }) }); group.finish(); @@ -116,7 +116,7 @@ pub fn run(filter: String, criterion: bool, baseline: Option, feature: F run_parse(code); } FeatureToBenchmark::Formatter => { - run_format(code); + run_format(code).unwrap(); } } } From a4b382404a0fb9d51e4be54dcd7b44b012655692 Mon Sep 17 00:00:00 2001 From: Emanuele Stoppa Date: Tue, 1 Feb 2022 14:26:33 +0000 Subject: [PATCH 7/8] fix: exclude parsing from benching for formatter --- xtask/bench/src/features/formatter.rs | 15 +++++------ xtask/bench/src/features/parser.rs | 7 +++-- xtask/bench/src/lib.rs | 39 ++++++++++++++++++--------- 3 files changed, 35 insertions(+), 26 deletions(-) diff --git a/xtask/bench/src/features/formatter.rs b/xtask/bench/src/features/formatter.rs index 3d0ecf354a8..c24ce440e05 100644 --- a/xtask/bench/src/features/formatter.rs +++ b/xtask/bench/src/features/formatter.rs @@ -1,6 +1,6 @@ use crate::BenchmarkSummary; -use rome_formatter::{format, FormatOptions, FormatResult, Formatted}; -use rslint_parser::{parse, Syntax}; +use rome_formatter::{format, FormatOptions}; +use rslint_parser::SyntaxNode; use std::fmt::{Display, Formatter}; use std::time::Duration; @@ -9,9 +9,9 @@ pub struct FormatterMeasurement { id: String, formatting: Duration, } -pub fn benchmark_format_lib(id: &str, code: &str) -> BenchmarkSummary { +pub fn benchmark_format_lib(id: &str, root: SyntaxNode) -> BenchmarkSummary { let formatter_timer = timing::start(); - run_format(code).unwrap(); + run_format(root); let formatter_duration = formatter_timer.stop(); BenchmarkSummary::Formatter(FormatterMeasurement { @@ -20,11 +20,8 @@ pub fn benchmark_format_lib(id: &str, code: &str) -> BenchmarkSummary { }) } -pub fn run_format(code: &str) -> FormatResult { - let syntax = Syntax::default().module(); - let root = parse(code, 0, syntax).syntax(); - - format(FormatOptions::default(), &root) +pub fn run_format(root: SyntaxNode) { + format(FormatOptions::default(), &root).unwrap(); } impl FormatterMeasurement { diff --git a/xtask/bench/src/features/parser.rs b/xtask/bench/src/features/parser.rs index 949551497c5..33199dea23e 100644 --- a/xtask/bench/src/features/parser.rs +++ b/xtask/bench/src/features/parser.rs @@ -1,8 +1,7 @@ use crate::BenchmarkSummary; use itertools::Itertools; use rslint_errors::Diagnostic; -use rslint_parser::ast::JsAnyRoot; -use rslint_parser::{Parse, Syntax}; +use rslint_parser::Syntax; use std::fmt::{Display, Formatter}; use std::ops::Add; use std::time::Duration; @@ -99,9 +98,9 @@ pub fn benchmark_parse_lib(id: &str, code: &str) -> BenchmarkSummary { }) } -pub fn run_parse(code: &str) -> Parse { +pub fn run_parse(code: &str) { let syntax = Syntax::default().module(); - rslint_parser::parse(code, 0, syntax) + rslint_parser::parse(code, 0, syntax); } impl ParseMeasurement { diff --git a/xtask/bench/src/lib.rs b/xtask/bench/src/lib.rs index 59c26f51ef2..55ec8b8697b 100644 --- a/xtask/bench/src/lib.rs +++ b/xtask/bench/src/lib.rs @@ -1,6 +1,8 @@ mod features; mod utils; +use criterion::BatchSize; +use rslint_parser::{parse, Syntax}; use std::fmt::{Display, Formatter}; use std::str::FromStr; use std::time::Duration; @@ -96,17 +98,22 @@ pub fn run(filter: String, criterion: bool, baseline: Option, feature: F } let mut group = criterion.benchmark_group(feature.to_string()); group.throughput(criterion::Throughput::Bytes(code.len() as u64)); - group.bench_function(&id, |b| { - b.iter(|| { - match feature { - FeatureToBenchmark::Parser => { - let _ = criterion::black_box(run_parse(code)); - } - FeatureToBenchmark::Formatter => { - let _ = criterion::black_box(run_format(code).unwrap()); - } - }; - }) + group.bench_function(&id, |b| match feature { + FeatureToBenchmark::Parser => b.iter(|| { + #[allow(clippy::unit_arg)] + criterion::black_box(run_parse(code)); + }), + FeatureToBenchmark::Formatter => b.iter_batched( + || { + let syntax = Syntax::default().module(); + parse(code, 0, syntax).syntax() + }, + |root| { + #[allow(clippy::unit_arg)] + criterion::black_box(run_format(root)); + }, + BatchSize::PerIteration, + ), }); group.finish(); } else { @@ -116,14 +123,20 @@ pub fn run(filter: String, criterion: bool, baseline: Option, feature: F run_parse(code); } FeatureToBenchmark::Formatter => { - run_format(code).unwrap(); + let syntax = Syntax::default().module(); + let root = parse(code, 0, syntax).syntax(); + run_format(root); } } } let result = match feature { FeatureToBenchmark::Parser => benchmark_parse_lib(&id, code), - FeatureToBenchmark::Formatter => benchmark_format_lib(&id, code), + FeatureToBenchmark::Formatter => { + let syntax = Syntax::default().module(); + let root = parse(code, 0, syntax).syntax(); + benchmark_format_lib(&id, root) + } }; summary.push(result.summary()); From ec1d0521336515034569d6bdee27fa8aea1bedf9 Mon Sep 17 00:00:00 2001 From: Emanuele Stoppa Date: Tue, 1 Feb 2022 14:50:58 +0000 Subject: [PATCH 8/8] fix: revisit black box call and return value on op --- xtask/bench/src/features/formatter.rs | 8 ++++---- xtask/bench/src/features/parser.rs | 7 ++++--- xtask/bench/src/lib.rs | 24 +++++++++--------------- 3 files changed, 17 insertions(+), 22 deletions(-) diff --git a/xtask/bench/src/features/formatter.rs b/xtask/bench/src/features/formatter.rs index c24ce440e05..829b1d38c43 100644 --- a/xtask/bench/src/features/formatter.rs +++ b/xtask/bench/src/features/formatter.rs @@ -1,5 +1,5 @@ use crate::BenchmarkSummary; -use rome_formatter::{format, FormatOptions}; +use rome_formatter::{format, FormatOptions, Formatted}; use rslint_parser::SyntaxNode; use std::fmt::{Display, Formatter}; use std::time::Duration; @@ -9,7 +9,7 @@ pub struct FormatterMeasurement { id: String, formatting: Duration, } -pub fn benchmark_format_lib(id: &str, root: SyntaxNode) -> BenchmarkSummary { +pub fn benchmark_format_lib(id: &str, root: &SyntaxNode) -> BenchmarkSummary { let formatter_timer = timing::start(); run_format(root); let formatter_duration = formatter_timer.stop(); @@ -20,8 +20,8 @@ pub fn benchmark_format_lib(id: &str, root: SyntaxNode) -> BenchmarkSummary { }) } -pub fn run_format(root: SyntaxNode) { - format(FormatOptions::default(), &root).unwrap(); +pub fn run_format(root: &SyntaxNode) -> Formatted { + format(FormatOptions::default(), root).unwrap() } impl FormatterMeasurement { diff --git a/xtask/bench/src/features/parser.rs b/xtask/bench/src/features/parser.rs index 33199dea23e..949551497c5 100644 --- a/xtask/bench/src/features/parser.rs +++ b/xtask/bench/src/features/parser.rs @@ -1,7 +1,8 @@ use crate::BenchmarkSummary; use itertools::Itertools; use rslint_errors::Diagnostic; -use rslint_parser::Syntax; +use rslint_parser::ast::JsAnyRoot; +use rslint_parser::{Parse, Syntax}; use std::fmt::{Display, Formatter}; use std::ops::Add; use std::time::Duration; @@ -98,9 +99,9 @@ pub fn benchmark_parse_lib(id: &str, code: &str) -> BenchmarkSummary { }) } -pub fn run_parse(code: &str) { +pub fn run_parse(code: &str) -> Parse { let syntax = Syntax::default().module(); - rslint_parser::parse(code, 0, syntax); + rslint_parser::parse(code, 0, syntax) } impl ParseMeasurement { diff --git a/xtask/bench/src/lib.rs b/xtask/bench/src/lib.rs index 55ec8b8697b..e458e670e8f 100644 --- a/xtask/bench/src/lib.rs +++ b/xtask/bench/src/lib.rs @@ -1,7 +1,6 @@ mod features; mod utils; -use criterion::BatchSize; use rslint_parser::{parse, Syntax}; use std::fmt::{Display, Formatter}; use std::str::FromStr; @@ -100,20 +99,15 @@ pub fn run(filter: String, criterion: bool, baseline: Option, feature: F group.throughput(criterion::Throughput::Bytes(code.len() as u64)); group.bench_function(&id, |b| match feature { FeatureToBenchmark::Parser => b.iter(|| { - #[allow(clippy::unit_arg)] criterion::black_box(run_parse(code)); }), - FeatureToBenchmark::Formatter => b.iter_batched( - || { - let syntax = Syntax::default().module(); - parse(code, 0, syntax).syntax() - }, - |root| { - #[allow(clippy::unit_arg)] - criterion::black_box(run_format(root)); - }, - BatchSize::PerIteration, - ), + FeatureToBenchmark::Formatter => { + let syntax = Syntax::default().module(); + let root = parse(code, 0, syntax).syntax(); + b.iter(|| { + criterion::black_box(run_format(&root)); + }) + } }); group.finish(); } else { @@ -125,7 +119,7 @@ pub fn run(filter: String, criterion: bool, baseline: Option, feature: F FeatureToBenchmark::Formatter => { let syntax = Syntax::default().module(); let root = parse(code, 0, syntax).syntax(); - run_format(root); + run_format(&root); } } } @@ -135,7 +129,7 @@ pub fn run(filter: String, criterion: bool, baseline: Option, feature: F FeatureToBenchmark::Formatter => { let syntax = Syntax::default().module(); let root = parse(code, 0, syntax).syntax(); - benchmark_format_lib(&id, root) + benchmark_format_lib(&id, &root) } };