Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

Allow the allocator to track the heap changes.#9291

Merged
5 commits merged intomasterfrom
kiz-track-allocator
Jul 7, 2021
Merged

Allow the allocator to track the heap changes.#9291
5 commits merged intomasterfrom
kiz-track-allocator

Conversation

@kianenigma
Copy link
Copy Markdown
Contributor

@kianenigma kianenigma commented Jul 6, 2021

This will greatly help with try-runtime's offchain_worker. I can now re-execute the last offchain worker's election code, as such, and get this nice report of heap usage:

    Finished dev [unoptimized + debuginfo] target(s) in 2m 23s
     Running `target/debug/substrate try-runtime --block-at 0x32f4f40c9bf09e7406721c562356f6b3ca718fc405a0164cbca31da6fd8ece7a --execution Wasm --url 'ws://kianenigma-archive:9944' offchain-worker live -m ElectionProviderMultiPhase`
2021-07-06 20:51:23.441  INFO main remote-ext: initializing remote client to "ws://kianenigma-archive:9944"
2021-07-06 20:51:23.517  INFO main remote-ext: scraping key-pairs from remote @ 0x32f4f40c9bf09e7406721c562356f6b3ca718fc405a0164cbca31da6fd8ece7a
2021-07-06 20:51:23.580 DEBUG main remote-ext: last page received: 6
2021-07-06 20:51:23.580  INFO main remote-ext: Querying a total of 6 keys
2021-07-06 20:51:24.928  INFO main remote-ext: downloaded data for module ElectionProviderMultiPhase (count: 6 / prefix: ede8e4fdc3c8b556f0ce2f77fc2575e3).
2021-07-06 20:51:24.928 DEBUG main remote-ext: adding data for hashed key: 26aa394eea5630e07c48ae0c9558cef7f9cce9c888469bb1a0dceaa129672ef8
2021-07-06 20:51:24.968 DEBUG main remote-ext: adding data for hashed key: 3a636f6465
2021-07-06 20:51:25.404  INFO main remote-ext: extending externalities with 0 manually injected key-values
2021-07-06 20:51:25.405  INFO main remote-ext: injecting a total of 8 keys
2021-07-06 20:51:28.527 DEBUG main wasm-heap: allocator being destroyed, max total_size 800, max bumper 1269856
2021-07-06 20:52:14.502 DEBUG main wasm-heap: allocator being destroyed, max total_size 29645680, max bumper 70057920
2021-07-06 20:52:14.505  INFO main try_runtime_cli: OffchainWorkerApi_offchain_worker executed without errors.

which shows the bumper being around 70mb, still far from the current 128mb.

@kianenigma kianenigma requested review from bkchr, emostov and pepyakin July 6, 2021 18:53
@kianenigma kianenigma added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D2-notlive 💤 PR contains changes in a runtime directory that is not deployed to a chain that requires an audit. labels Jul 6, 2021
@kianenigma kianenigma mentioned this pull request Jul 6, 2021
Open
pepyakin
pepyakin reviewed Jul 6, 2021
View reviewed changes
client/allocator/src/freeing_bump.rs Outdated
}
macro_rules! log {
($level:tt, $( $args:expr ),+ ) => {
log::$level!(target: "wasm-heap", $( $args ),+);
Copy link
Copy Markdown
Contributor

@pepyakin pepyakin Jul 6, 2021

Choose a reason for hiding this comment

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

TBH, this macro looks (at the call sites) a bit weird. Why don't we just remove it altogether, and stick to a regular logging as found in the rest of codebase?

Copy link
Copy Markdown
Contributor Author

@kianenigma kianenigma Jul 6, 2021

Choose a reason for hiding this comment

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

this is a patter that I use a lot in pallets, but I will respect the fact that this is the client realm and most logs are not like this, so will revert.

Copy link
Copy Markdown
Contributor Author

@kianenigma kianenigma Jul 6, 2021

Choose a reason for hiding this comment

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

FWIW, my main preference for this is that I don't like repeating target: "..." all over the place.

bkchr
bkchr approved these changes Jul 6, 2021
View reviewed changes
client/allocator/Cargo.toml Outdated
"sp-core/std",
"sp-wasm-interface/std",
"log",
"log/std",
Copy link
Copy Markdown
Member

@bkchr bkchr Jul 6, 2021

Choose a reason for hiding this comment

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

Suggested change
"log/std",

Copy link
Copy Markdown
Contributor Author

@kianenigma kianenigma Jul 6, 2021

Choose a reason for hiding this comment

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

ahh about this: now the allocator is even in client. Should this crate really have a separate "std" feature? I can just remove that.

Copy link
Copy Markdown
Contributor

@pepyakin pepyakin Jul 6, 2021

Choose a reason for hiding this comment

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

The std feature is not needed. We should remove it IMO

kianenigma and others added 2 commits July 6, 2021 21:47
@kianenigma @bkchr
Update client/allocator/Cargo.toml
a7447cc 
Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
@kianenigma @bkchr
Update client/allocator/Cargo.toml
55fd368 
Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
emostov
emostov reviewed Jul 6, 2021
View reviewed changes
emostov
emostov reviewed Jul 6, 2021
View reviewed changes
emostov
emostov reviewed Jul 6, 2021
View reviewed changes
emostov
emostov approved these changes Jul 6, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@emostov emostov left a comment

Choose a reason for hiding this comment

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

Approving with the caveat that I don't feel qualified to fully asses the performance implications.

@kianenigma
Copy link
Copy Markdown
Contributor Author

kianenigma commented Jul 7, 2021

bot merge

@ghost
Copy link
Copy Markdown

ghost commented Jul 7, 2021

Trying merge.

@ghost ghost merged commit 933e9c5 into master Jul 7, 2021
@ghost ghost deleted the kiz-track-allocator branch July 7, 2021 05:32
@kianenigma kianenigma mentioned this pull request Jul 7, 2021
Closed
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

4 more reviewers

@shawntabrizi shawntabrizi shawntabrizi left review comments

@pepyakin pepyakin pepyakin left review comments

@bkchr bkchr bkchr approved these changes

@emostov emostov emostov approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D2-notlive 💤 PR contains changes in a runtime directory that is not deployed to a chain that requires an audit.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@kianenigma @shawntabrizi @pepyakin @bkchr @emostov