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.

Sign up for GitHub

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

Jump to bottom

Improve resizing log message #1939

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Improve resizing log message #1939

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4aa9d98
Improve resizing log message
georgwiese Oct 23, 2024
8c401b6
Improve message, undo change in test
georgwiese Oct 23, 2024
f7435d6
Apply suggestions from code review
georgwiese Oct 25, 2024
6123923
Improve error message
georgwiese Oct 25, 2024
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
13 changes: 2 additions & 11 deletions executor/src/witgen/machines/block_machine.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ use super::{EvalResult, FixedData, MachineParts};
use crate::witgen::affine_expression::AlgebraicVariable;
use crate::witgen::block_processor::BlockProcessor;
use crate::witgen::data_structures::finalizable_data::FinalizableData;
use crate::witgen::machines::compute_size_and_log;
use crate::witgen::processor::{OuterQuery, Processor};
use crate::witgen::rows::{Row, RowIndex, RowPair};
use crate::witgen::sequence_iterator::{
Expand Down Expand Up @@ -310,17 +311,7 @@ impl<'a, T: FieldElement> Machine<'a, T> for BlockMachine<'a, T> {
This might violate some internal constraints."
);
}

let new_degree = self.data.len().next_power_of_two() as DegreeType;
let new_degree = self.degree_range.fit(new_degree);
log::info!(
"Resizing variable length machine '{}': {} -> {} (rounded up from {})",
self.name,
self.degree,
new_degree,
self.data.len()
);
self.degree = new_degree;
self.degree = compute_size_and_log(&self.name, self.data.len(), self.degree_range);

if matches!(self.connection_type, ConnectionType::Permutation) {
// We have to make sure that *all* selectors are 0 in the dummy block,
Expand Down
13 changes: 2 additions & 11 deletions executor/src/witgen/machines/double_sorted_witness_machine_16.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ use std::iter::once;
use itertools::Itertools;

use super::{Machine, MachineParts};
use crate::witgen::machines::compute_size_and_log;
use crate::witgen::rows::RowPair;
use crate::witgen::util::try_to_simple_poly;
use crate::witgen::{EvalError, EvalResult, FixedData, MutableState, QueryCallback};
Expand Down Expand Up @@ -297,17 +298,7 @@ impl<'a, T: FieldElement> Machine<'a, T> for DoubleSortedWitnesses16<'a, T> {
set_selector(None);
}

let current_size = addr.len();
let new_size = current_size.next_power_of_two() as DegreeType;
let new_size = self.degree_range.fit(new_size);
log::info!(
"Resizing variable length machine '{}': {} -> {} (rounded up from {})",
self.name,
self.degree,
new_size,
current_size
);
self.degree = new_size;
self.degree = compute_size_and_log(&self.name, addr.len(), self.degree_range);

while addr.len() < self.degree as usize {
addr.push(*addr.last().unwrap());
Expand Down
13 changes: 2 additions & 11 deletions executor/src/witgen/machines/double_sorted_witness_machine_32.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ use std::iter::once;
use itertools::Itertools;

use super::{Machine, MachineParts};
use crate::witgen::machines::compute_size_and_log;
use crate::witgen::rows::RowPair;
use crate::witgen::util::try_to_simple_poly;
use crate::witgen::{EvalError, EvalResult, FixedData, MutableState, QueryCallback};
Expand Down Expand Up @@ -266,17 +267,7 @@ impl<'a, T: FieldElement> Machine<'a, T> for DoubleSortedWitnesses32<'a, T> {
set_selector(None);
}

let current_size = addr.len();
let new_size = current_size.next_power_of_two() as DegreeType;
let new_size = self.degree_range.fit(new_size);
log::info!(
"Resizing variable length machine '{}': {} -> {} (rounded up from {})",
self.name,
self.degree,
new_size,
current_size
);
self.degree = new_size;
self.degree = compute_size_and_log(&self.name, addr.len(), self.degree_range);

while addr.len() < self.degree as usize {
addr.push(*addr.last().unwrap());
Expand Down
32 changes: 32 additions & 0 deletions executor/src/witgen/machines/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ use powdr_ast::analyzed;
use powdr_ast::analyzed::DegreeRange;
use powdr_ast::analyzed::PolyID;

use powdr_number::DegreeType;
use powdr_number::FieldElement;

use crate::Identity;
Expand Down Expand Up @@ -214,3 +215,34 @@ impl<'a, T: FieldElement> MachineParts<'a, T> {
self.fixed_data.column_name(poly_id)
}
}

/// The minimum size for which a warning is logged if the used rows are less than half of the size.
/// This number coincides with 2**powdr_linker::MIN_DEGREE_LOG.
Copy link
Member

Choose a reason for hiding this comment

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

It doesn't because there is no static_assert.

Copy link
Member

Choose a reason for hiding this comment

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

Is it important that it does?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is not really important. If the total number of rows, the overhead of proving them becomes negligible and the log would just be noise. Arguably 32 is a already very small.
If this number was below 2**powdr_linker::MIN_DEGREE_LOG, we'd get the log message often, that's why I chose it to be equal.

Yesterday, we also discussed with Thibaut and Leo that we want to remove MIN_DEGREE_LOG and let the user always be explicit about the min degree. Then, this comment goes away.

/// It's probably not worth introducing a dependency to the linker just for this constant.
const MIN_RERPORTING_SIZE: DegreeType = 32;

pub fn compute_size_and_log(name: &str, used_rows: usize, degree_range: DegreeRange) -> DegreeType {
let size = used_rows.next_power_of_two() as DegreeType;
let size = degree_range.fit(size);
let fraction_used = used_rows as f64 / size as f64;

if size > MIN_RERPORTING_SIZE && fraction_used < 0.5 {
// In a machine configured to use VADCOP, we would expect the next power of two to be used.
log::info!(
"Only {} / {} rows are used in machine {}, which is configured to support sizes in the range {}..{}. If the backend supports it, consider lowering the min_degree.",
used_rows,
size,
name,
degree_range.min,
degree_range.max,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"Only {} / {} rows are used in machine {}, which is configured to support sizes in the range {}..{}. If the backend supports it, consider lowering the min_degree.",
used_rows,
size,
name,
degree_range.min,
degree_range.max,
"Only {used_rows} of {size} rows are used in machine '{name}', which is configured to support sizes in the range {degree_range}. If the backend supports it, consider lowering the min_degree.",

Copy link
Member

Choose a reason for hiding this comment

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

I think we should also say what the actual probleme here is. I'm not sure, but I guess that the machine could be sized down, but min_degree prevents the size being reduced?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK, added that! Also logging the percentage now and mention that the goal should be to be at least at 50%.

);
} else {
log::debug!(
"{} / {} rows are used in machine {}.",
used_rows,
size,
name,
);
georgwiese marked this conversation as resolved.
Show resolved Hide resolved
}
size
}
16 changes: 6 additions & 10 deletions executor/src/witgen/vm_processor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ use std::cmp::max;
use std::time::Instant;

use crate::witgen::identity_processor::{self};
use crate::witgen::machines::compute_size_and_log;
use crate::witgen::IncompleteCause;
use crate::Identity;

Expand Down Expand Up @@ -180,17 +181,12 @@ impl<'a, 'b, 'c, T: FieldElement, Q: QueryCallback<T>> VmProcessor<'a, 'b, 'c, T
"Found loop with period {p} starting at row {row_index}"
);

let new_degree = self.processor.len().next_power_of_two() as DegreeType;
let new_degree = self.degree_range.fit(new_degree);
log::info!(
"Resizing variable length machine '{}': {} -> {} (rounded up from {})",
self.machine_name,
self.degree,
new_degree,
self.processor.len()
self.degree = compute_size_and_log(
&self.machine_name,
self.processor.len(),
self.degree_range,
);
self.degree = new_degree;
self.processor.set_size(new_degree);
self.processor.set_size(self.degree);
}
}
if let Some(period) = looping_period {
Expand Down