Skip to content

Commit

Permalink
Fix big bug in size estimation of array buffers (#2991)
Browse files Browse the repository at this point in the history
An off-by-one bug in `estimated_bytes_size` caused the size estimations
of Tensors to completely ignore the actual payload, causing the memory
consumption of images to be vastly underestimated.

I decided to rewrite the code to be more clear for future readers.

This discovered a difference in how validity bitmaps are written by Rust
and Python+C++, fixed in
a828441.

To help find this problem a lot of work was also put into improving the
output of `scripts/ci/run_e2e_roundtrip_tests.py`.

* Discovered while working on
#2435

### Checklist
* [x] I have read and agree to [Contributor
Guide](https://github.com/rerun-io/rerun/blob/main/CONTRIBUTING.md) and
the [Code of
Conduct](https://github.com/rerun-io/rerun/blob/main/CODE_OF_CONDUCT.md)
* [x] I've included a screenshot or gif (if applicable)
* [x] I have tested [demo.rerun.io](https://demo.rerun.io/pr/2991) (if
applicable)

- [PR Build Summary](https://build.rerun.io/pr/2991)
- [Docs
preview](https://rerun.io/preview/pr%3Aemilk%2Ffix-size-estimation-bug/docs)
- [Examples
preview](https://rerun.io/preview/pr%3Aemilk%2Ffix-size-estimation-bug/examples)

---------

Co-authored-by: Jeremy Leibs <[email protected]>
  • Loading branch information
emilk and jleibs authored Aug 16, 2023
1 parent 0f60eb9 commit 04d5491
Show file tree
Hide file tree
Showing 46 changed files with 1,387 additions and 645 deletions.
53 changes: 32 additions & 21 deletions crates/re_log_types/src/data_table.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
use std::collections::BTreeMap;

use re_types::ComponentName;

use ahash::HashMap;
use itertools::Itertools as _;
use itertools::{izip, Itertools as _};
use nohash_hasher::IntSet;
use re_types::ComponentName;
use smallvec::SmallVec;

use crate::{
Expand Down Expand Up @@ -1138,11 +1139,11 @@ impl DataTable {
cells: ref cells2,
} = row2;

for (c1, c2) in cells1.0.iter().zip(&cells2.0) {
for (c1, c2) in izip!(&cells1.0, &cells2.0) {
if c1 != c2 {
anyhow::ensure!(
c1.datatype() == c2.datatype(),
"Found discrepancy in row #{ri}, cells' datatypes don't match!\n{}",
"Found discrepancy in row #{ri}: cells' datatypes don't match!\n{}",
similar_asserts::SimpleDiff::from_str(
&format!("{:?}", c1.datatype()),
&format!("{:?}", c2.datatype()),
Expand All @@ -1160,7 +1161,7 @@ impl DataTable {
) {
anyhow::ensure!(
arr1.validity() == arr2.validity(),
"Found discrepancy in row #{ri}, union arrays' validity bitmaps don't match!\n{}\n{}",
"Found discrepancy in row #{ri}: union arrays' validity bitmaps don't match!\n{}\n{}",
similar_asserts::SimpleDiff::from_str(&row1.to_string(), &row2.to_string(), "row1", "row2"),
similar_asserts::SimpleDiff::from_str(
&format!("{:?}", arr1.validity()),
Expand All @@ -1171,7 +1172,7 @@ impl DataTable {
);
anyhow::ensure!(
arr1.types() == arr2.types(),
"Found discrepancy in row #{ri}, union arrays' type indices don't match!\n{}\n{}",
"Found discrepancy in row #{ri}: union arrays' type indices don't match!\n{}\n{}",
similar_asserts::SimpleDiff::from_str(&row1.to_string(), &row2.to_string(), "row1", "row2"),
similar_asserts::SimpleDiff::from_str(
&format!("{:?}", arr1.types()),
Expand All @@ -1182,7 +1183,7 @@ impl DataTable {
);
anyhow::ensure!(
arr1.offsets() == arr2.offsets(),
"Found discrepancy in row #{ri}, union arrays' offsets don't match!\n{}\n{}",
"Found discrepancy in row #{ri}: union arrays' offsets don't match!\n{}\n{}",
similar_asserts::SimpleDiff::from_str(&row1.to_string(), &row2.to_string(), "row1", "row2"),
similar_asserts::SimpleDiff::from_str(
&format!("{:?}", arr1.offsets()),
Expand All @@ -1196,10 +1197,10 @@ impl DataTable {
}

let mut size_mismatches = vec![];
for (c1, c2) in cells1.0.iter().zip(&cells2.0) {
for (c1, c2) in izip!(&cells1.0, &cells2.0) {
if c1.total_size_bytes() != c2.total_size_bytes() {
size_mismatches.push(format!(
"Found discrepancy in row #{ri}, cells' sizes don't match! {} ({}) vs. {} ({}) bytes",
"Sizes don't match! {} ({}) vs. {} ({}) bytes. Perhaps the validity differs?",
c1.total_size_bytes(),
c1.component_name(),
c2.total_size_bytes(),
Expand All @@ -1208,7 +1209,7 @@ impl DataTable {

fn cell_to_bytes(cell: DataCell) -> Vec<u8> {
let row = DataRow::from_cells1(
RowId::random(),
RowId::ZERO,
"cell",
TimePoint::default(),
cell.num_instances(),
Expand All @@ -1229,17 +1230,25 @@ impl DataTable {
}

let c1_bytes = cell_to_bytes(c1.clone());
let c2_bytes = cell_to_bytes(c1.clone());
let c2_bytes = cell_to_bytes(c2.clone());

size_mismatches.push(
similar_asserts::SimpleDiff::from_str(
&format!("{c1_bytes:?}"),
&format!("{c2_bytes:?}"),
"cell1_ipc",
"cell2_ipc",
)
.to_string(),
);
size_mismatches.push(format!(
"IPC size is {} vs {} bytes",
c1_bytes.len(),
c2_bytes.len()
));

if c1_bytes.len().max(c2_bytes.len()) < 300 {
size_mismatches.push(
similar_asserts::SimpleDiff::from_str(
&format!("{c1_bytes:#?}"),
&format!("{c2_bytes:#?}"),
"cell1_ipc",
"cell2_ipc",
)
.to_string(),
);
}
}
}

Expand All @@ -1248,7 +1257,9 @@ impl DataTable {
&& entity_path1 == entity_path2
&& num_instances1 == num_instances2
&& cells1 == cells2,
"Found discrepancy in row #{ri}:\n{}\n{}",
"Found discrepancy in row #{ri}:\n{}\n{}\
\n\nrow1:\n{row1}
\n\nrow2:\n{row2}",
similar_asserts::SimpleDiff::from_str(
&row1.to_string(),
&row2.to_string(),
Expand Down
2 changes: 1 addition & 1 deletion crates/re_types/source_hash.txt

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 9 additions & 1 deletion crates/re_types/src/components/line_strip2d.rs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 9 additions & 1 deletion crates/re_types/src/components/line_strip3d.rs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 9 additions & 1 deletion crates/re_types/src/components/origin3d.rs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 9 additions & 1 deletion crates/re_types/src/components/point2d.rs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 9 additions & 1 deletion crates/re_types/src/components/point3d.rs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 9 additions & 1 deletion crates/re_types/src/components/vector3d.rs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 9 additions & 1 deletion crates/re_types/src/datatypes/mat3x3.rs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 9 additions & 1 deletion crates/re_types/src/datatypes/mat4x4.rs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 9 additions & 1 deletion crates/re_types/src/datatypes/quaternion.rs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 9 additions & 1 deletion crates/re_types/src/datatypes/rotation3d.rs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 9 additions & 1 deletion crates/re_types/src/datatypes/rotation_axis_angle.rs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 9 additions & 1 deletion crates/re_types/src/datatypes/scale3d.rs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

20 changes: 18 additions & 2 deletions crates/re_types/src/datatypes/translation_and_mat3x3.rs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 9 additions & 1 deletion crates/re_types/src/datatypes/translation_rotation_scale3d.rs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading

0 comments on commit 04d5491

Please sign in to comment.