Skip to content

Refactor address_transform.rs to use less memory #1260

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.

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

Merged

Conversation

yurydelendik
Copy link
Contributor

The crates/debug/src/transform/address_transform.rs is unoptimized in terms of data structures. This PR refactors this file to remove creation of intermediate in-heap structures, thus improves overall performance of the DWARF transformation.

@yurydelendik yurydelendik force-pushed the refactor-addr-transform branch from f28f2ef to 56a6fc7 Compare March 14, 2020 14:07
@yurydelendik yurydelendik requested a review from fitzgen March 14, 2020 14:08
Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

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

Looks good to me! A few ideas for follow up PRs in the comments below.

@@ -50,13 +49,16 @@ struct Range {
positions: Box<[Position]>,
}

type RangeIndex = usize;
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we could make this a u32 in practice? Do we ever have to process Wasm files larger than 4GiB?

This would let us shave off half the overhead on 64-bit platforms.

index.insert(position, active_ranges.clone().into_boxed_slice());
let mut sorted_ranges = active_ranges.clone();
sorted_ranges.sort();
index.insert(position, sorted_ranges.into_boxed_slice());
Copy link
Member

Choose a reason for hiding this comment

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

Rather than having our index be a BTreeMap<WasmAddress, Box<[RangeIndex]>, which implies a bunch of allocated boxed slices that are all relatively small and potentially contribute to internal and/or external memory fragmentation, we should be able to save some memory by doing:

index_ranges: Vec<RangeIndex>,
// A map from the wasm address to the range within `index_ranges` for the associated wasm address.
index: BTreeMap<WasmAddress, std::ops::Range<usize>>,

Also if we can get away with it, we could make it a Range<u32> for extra savings.

To do this, rather than inserting sorted_ranges into the index here, we would do something like:

let start = all_ranges.len();
let end = start + sorted_ranges.len();
index_ranges.extend(sorted_ranges);
index.insert(position, start..end);

@yurydelendik yurydelendik force-pushed the refactor-addr-transform branch from ffc108b to ae14339 Compare March 23, 2020 20:37
@yurydelendik yurydelendik merged commit 021ebb3 into bytecodealliance:master Mar 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants