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

feat: Implement `MemoryUsage` for `Store` #2199

Merged

bors merged 9 commits into wasmerio:master from Hywan:feat-memory-usage

Mar 23, 2021

Contributor

Hywan commented Mar 19, 2021 •

edited

Loading

Description

This patch implements loupe::MemoryUsage for wasmer::Store.

Review

Add a short description of the the change to the CHANGELOG.md file

Hywan requested review from losfair, MarkMcCaskey, nlewycky and syrusakbary as code owners

March 19, 2021 10:03

Hywan changed the title ~~feat: Implement MemoryUsage for Store~~ WIP feat: Implement MemoryUsage for Store

Hywan mentioned this pull request

feat: Implement MemoryUsage for Module #2200

Merged

1 task

syrusakbary reviewed

View reviewed changes

lib/api/Cargo.toml Outdated Show resolved Hide resolved

syrusakbary reviewed

View reviewed changes

lib/compiler/src/target.rs Outdated Show resolved Hide resolved

syrusakbary reviewed

View reviewed changes

lib/compiler/src/target.rs Outdated Show resolved Hide resolved

syrusakbary reviewed

View reviewed changes

lib/engine/Cargo.toml Outdated Show resolved Hide resolved

syrusakbary reviewed

View reviewed changes

lib/middlewares/Cargo.toml Outdated Show resolved Hide resolved

syrusakbary reviewed

View reviewed changes

lib/vm/Cargo.toml Outdated Show resolved Hide resolved

syrusakbary reviewed

View reviewed changes

lib/wasmer-types/Cargo.toml Outdated Show resolved Hide resolved

syrusakbary reviewed

View reviewed changes

tests/lib/engine-dummy/Cargo.toml Outdated Show resolved Hide resolved

syrusakbary reviewed

View reviewed changes

lib/compiler-cranelift/Cargo.toml Outdated Show resolved Hide resolved

syrusakbary reviewed

View reviewed changes

lib/compiler-llvm/Cargo.toml Outdated Show resolved Hide resolved

syrusakbary reviewed

View reviewed changes

lib/compiler-singlepass/Cargo.toml Outdated Show resolved Hide resolved

syrusakbary reviewed

View reviewed changes

lib/compiler/Cargo.toml Outdated Show resolved Hide resolved

syrusakbary reviewed

View reviewed changes

lib/engine-jit/Cargo.toml Outdated Show resolved Hide resolved

syrusakbary reviewed

View reviewed changes

lib/engine-native/Cargo.toml Outdated Show resolved Hide resolved

syrusakbary reviewed

View reviewed changes

lib/engine-object-file/Cargo.toml Outdated Show resolved Hide resolved

Hywan added 3 commits

March 22, 2021 11:42


          feat: Start implementing loupe::MemoryUsage on wasmer::Store.


          Merge branch 'master' into feat-memory-usage

fc4637b


          feat: Use latest version of loupe.

2afed4d

Hywan force-pushed the feat-memory-usage branch from 4c4e8c6 to 2afed4d Compare

March 22, 2021 10:44

Hywan changed the title ~~WIP feat: Implement MemoryUsage for Store~~ feat: Implement MemoryUsage for Store

Contributor Author

Hywan commented Mar 22, 2021

bors try

bors bot added a commit that referenced this pull request


          Try #2199:

744c8c5

Contributor

bors bot commented Mar 22, 2021

try

Build failed:

Code lint


          test: Implement MemoryUsage on test middlewares.

c212f74


          doc: Update the tunable exemple to include MemoryUsage.

10c6f26

Hywan requested a review from jubianchi as a code owner

March 22, 2021 14:39

Contributor Author

Hywan commented Mar 22, 2021

bors try

bors bot added a commit that referenced this pull request


          Try #2199:

ff1527b

Contributor

bors bot commented Mar 22, 2021

try

Build failed:

Test on windows-x64

MarkMcCaskey approved these changes

View reviewed changes

Contributor

MarkMcCaskey left a comment

Looks good to me other than the fact that we shouldn't merge this upstream until we publish loupe to crates.io 👍

examples/tunables_limit_memory.rs

Comment on lines +136 to +141

+              impl<T: Tunables> MemoryUsage for LimitingTunables<T> {
+                  fn size_of_val(&self, _tracker: &mut dyn MemoryUsageTracker) -> usize {
+                      mem::size_of_val(self)
+                  }
+              }

Contributor

MarkMcCaskey Mar 22, 2021

It's a bit surprising to me that this isn't something the macro can generate. I'm not sure how common of a base case it is, maybe it isn't worth it.

Contributor Author

Hywan Mar 22, 2021

It's because of the generic here. I need to dig into loupe-derive to see what's going on.

Contributor Author

Hywan Mar 23, 2021

Fixed by wasmerio/loupe#14. I'll update this file in #2200 probably.

lib/compiler/src/target.rs

Comment on lines +164 to 170

+              #[derive(Clone, Debug, PartialEq, Eq, Hash, MemoryUsage)]
               pub struct Target {
+                  #[loupe(skip)]
                   triple: Triple,
+                  #[loupe(skip)]
                   cpu_features: EnumSet<CpuFeature>,
               }

Contributor

MarkMcCaskey Mar 22, 2021

This might be the same as the thing I mentioned above, not high priority and easy to add later, but might be able to do something like

#[loupe(skip_all)]
pub struct Target {
...
}

To just get the local size and not any of the heap size stuff

Contributor Author

Hywan Mar 22, 2021

I've created wasmerio/loupe#12.

Hywan mentioned this pull request

Add the #[loupe(skip_all)] attr wasmerio/loupe#12

Open


          fix(engine-jit) Fix MemoryUsage on Windows.

1a2dbda

Contributor Author

Hywan commented Mar 23, 2021

bors try

bors bot added a commit that referenced this pull request


          Try #2199:

6dc9477

Contributor

bors bot commented Mar 23, 2021

try

Build failed:

Code lint

Hywan added 3 commits

March 23, 2021 11:18


          chore: Use loupe 0.1.0.

4b752ed


          doc(changelog) Add wasmerio#2199.

0fe618a


          Merge branch 'master' into feat-memory-usage

5ff82e5

Contributor Author

Hywan commented Mar 23, 2021

bors r+

syrusakbary approved these changes

View reviewed changes

Contributor

bors bot commented Mar 23, 2021

Build succeeded:

bors bot merged commit af59fcb into wasmerio:master

bors bot added a commit that referenced this pull request


          Merge #2200

0f13e23

2200: feat: Implement `MemoryUsage` for `Module` r=Hywan a=Hywan

# Description

This patch implements `loupe::MemoryUsage` for `wasmer::Module`.

~This PR includes #2199. To review unique patches: https://github.com/Hywan/wasmer/compare/feat-memory-usage...feat-memory-usage-module?expand=1~

# Review

- [x] Add a short description of the the change to the CHANGELOG.md file


Co-authored-by: Ivan Enderlin <[email protected]>

bors bot added a commit that referenced this pull request


          Merge #2200

169e8ad

2200: feat: Implement `MemoryUsage` for `Module` r=Hywan a=Hywan

# Description

This patch implements `loupe::MemoryUsage` for `wasmer::Module`.

~This PR includes #2199. To review unique patches: https://github.com/Hywan/wasmer/compare/feat-memory-usage...feat-memory-usage-module?expand=1~

# Review

- [x] Add a short description of the the change to the CHANGELOG.md file


Co-authored-by: Ivan Enderlin <[email protected]>

bors bot added a commit that referenced this pull request


          Merge #2200

4026b3a

2200: feat: Implement `MemoryUsage` for `Module` r=Hywan a=Hywan

# Description

This patch implements `loupe::MemoryUsage` for `wasmer::Module`.

~This PR includes #2199. To review unique patches: https://github.com/Hywan/wasmer/compare/feat-memory-usage...feat-memory-usage-module?expand=1~

# Review

- [x] Add a short description of the the change to the CHANGELOG.md file


Co-authored-by: Ivan Enderlin <[email protected]>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

MarkMcCaskey MarkMcCaskey approved these changes

syrusakbary syrusakbary approved these changes

losfair Awaiting requested review from losfair

nlewycky Awaiting requested review from nlewycky

jubianchi Awaiting requested review from jubianchi

Labels

None yet