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.

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

Introduce and start using the MallocSizeOf trait. #17044

Merged
merged 1 commit into from May 29, 2017

Conversation

Projects
None yet
5 participants
@nnethercote
Copy link
Contributor

commented May 26, 2017

MallocSizeOf is similar to the existing HeapSizeOf trait from the
heapsize crate. The only difference is that MallocSizeOf's
malloc_size_of_children() function takes an additional MallocSizeOfFn
argument, which is used to measure heap blocks. This extra argument
makes MallocSizeOf match how Gecko's memory measurements work, and is
required for Stylo to integrate with DMD.

The patch also introduces a second trait, MallocSizeOfWithGuard, which
is much the same as MallocSizeOf, but with a |guard| argument for the
global style lock.

Finally, the patch uses the new traits to measure a small amount of
Stylo's memory usage.


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #__ (github issue number if applicable).
  • There are tests for these changes OR
  • These changes do not require tests because code is only for Gecko integration.

This change is Reviewable

@highfive

This comment has been minimized.

Copy link

commented May 26, 2017

Heads up! This PR modifies the following files:

  • @bholley: components/style/gecko/generated/bindings.rs, components/style/stylesheets.rs, components/style/properties/declaration_block.rs, components/style/properties/properties.mako.rs
  • @emilio: components/style/gecko/generated/bindings.rs, components/style/stylesheets.rs, components/style/properties/declaration_block.rs, components/style/properties/properties.mako.rs, ports/geckolib/glue.rs
@highfive

This comment has been minimized.

Copy link

commented May 26, 2017

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
  • These commits modify style code, but no tests are modified. Please consider adding a test!
@nnethercote

This comment has been minimized.

Copy link
Contributor Author

commented May 26, 2017

@heycam: This is the Servo part of the code you reviewed in https://bugzilla.mozilla.org/show_bug.cgi?id=1353998. The only changes are:

  • I addressed your comments.
  • I added handling of empty allocations (with pointer value of 0x1) as @emilio suggested.
  • I added #[cfg(feature = "gecko")] throughout.
@jdm

This comment has been minimized.

Copy link
Member

commented May 26, 2017

0x1 no longer works; we need to use the same technique as servo/heapsize#81.

@nnethercote

This comment has been minimized.

Copy link
Contributor Author

commented May 26, 2017

Thanks, @jdm. I see that Servo is still using heapsize 0.3.9, and so hasn't received that change.

@nnethercote nnethercote force-pushed the nnethercote:MallocSizeOf branch from e00bcf5 to 2d40f2c May 26, 2017

@nnethercote nnethercote force-pushed the nnethercote:MallocSizeOf branch from 2d40f2c to 65f6cb1 May 26, 2017

pub type MallocSizeOfFn = unsafe extern "C" fn(ptr: *const c_void) -> usize;

/// Call malloc_size_of on ptr, first checking that the allocation isn't empty.
#[cfg(feature = "gecko")]

This comment has been minimized.

Copy link
@emilio

emilio May 26, 2017

Member

Is there a reason this all needs to be cfg'd out? If we can compile it in servo mode too it'd be better IMO (less differences among builds).

This comment has been minimized.

Copy link
@nnethercote

nnethercote May 26, 2017

Author Contributor

Just that it's not used in Servo.

This comment has been minimized.

Copy link
@emilio

emilio May 28, 2017

Member

I'd prefer to not compile it out if possible, even if it's not used, because if someone is changing related code and causes this not to compile would only see the error when compiling the geckolib version.

@emilio

This comment has been minimized.

Copy link
Member

commented May 28, 2017

That being said, this was reviewed already no? If you could do that bit, that'd be great. Otherwise, go ahead and land it as you wish :)

@bors-servo delegate+

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented May 28, 2017

✌️ @nnethercote can now approve this pull request

Introduce and start using the MallocSizeOf trait.
MallocSizeOf is similar to the existing HeapSizeOf trait from the
heapsize crate. The only difference is that MallocSizeOf's
malloc_size_of_children() function takes an additional MallocSizeOfFn
argument, which is used to measure heap blocks. This extra argument
makes MallocSizeOf match how Gecko's memory measurements work, and is
required for Stylo to integrate with DMD.

The patch also introduces a second trait, MallocSizeOfWithGuard, which
is much the same as MallocSizeOf, but with a |guard| argument for the
global style lock.

Finally, the patch uses the new traits to measure a small amount of
Stylo's memory usage.

@nnethercote nnethercote force-pushed the nnethercote:MallocSizeOf branch from 65f6cb1 to 6d5b124 May 29, 2017

@nnethercote

This comment has been minimized.

Copy link
Contributor Author

commented May 29, 2017

@bors-servo: r=emilio

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented May 29, 2017

📌 Commit 6d5b124 has been approved by nnethercote

@highfive highfive assigned nnethercote and unassigned jdm May 29, 2017

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented May 29, 2017

⌛️ Testing commit 6d5b124 with merge c558c55058721a856b29c3a79cea49e53bbc4a84...

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented May 29, 2017

💡 This pull request was already approved, no need to approve it again.

  • This pull request is currently being tested. If there's no response from the continuous integration service, you may use retry to trigger a build again.
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented May 29, 2017

📌 Commit 6d5b124 has been approved by nnethercote

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented May 29, 2017

⌛️ Testing commit 6d5b124 with merge 1ec79b6756e012ad2c59264912b2774f103f3aef...

@nnethercote

This comment has been minimized.

Copy link
Contributor Author

commented May 29, 2017

@bors-servo: r=emilio

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented May 29, 2017

💡 This pull request was already approved, no need to approve it again.

  • This pull request is currently being tested. If there's no response from the continuous integration service, you may use retry to trigger a build again.
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented May 29, 2017

📌 Commit 6d5b124 has been approved by emilio

@highfive highfive assigned emilio and unassigned nnethercote May 29, 2017

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented May 29, 2017

⌛️ Testing commit 6d5b124 with merge aca0943...

bors-servo added a commit that referenced this pull request May 29, 2017

Auto merge of #17044 - nnethercote:MallocSizeOf, r=emilio
Introduce and start using the MallocSizeOf trait.

MallocSizeOf is similar to the existing HeapSizeOf trait from the
heapsize crate. The only difference is that MallocSizeOf's
malloc_size_of_children() function takes an additional MallocSizeOfFn
argument, which is used to measure heap blocks. This extra argument
makes MallocSizeOf match how Gecko's memory measurements work, and is
required for Stylo to integrate with DMD.

The patch also introduces a second trait, MallocSizeOfWithGuard, which
is much the same as MallocSizeOf, but with a |guard| argument for the
global style lock.

Finally, the patch uses the new traits to measure a small amount of
Stylo's memory usage.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [X] These changes do not require tests because code is only for Gecko integration.

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17044)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented May 29, 2017

@bors-servo bors-servo merged commit 6d5b124 into servo:master May 29, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.