Skip to content

/ servo

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

Jump to bottom

Report memory usage of Stylist in LayoutTaskData #7038

Closed
jdm opened this issue Aug 6, 2015 · 23 comments
Closed

Report memory usage of Stylist in LayoutTaskData #7038

jdm opened this issue Aug 6, 2015 · 23 comments
Labels
E-less easy I-perf-bloat

Comments

@jdm
Copy link
Member

@jdm jdm commented Aug 6, 2015

As part of collect_reports in layout_task.rs, we should measure the heap usage of the stylist member of LayoutTaskData. This involves adding #[derive(HeapSizeOf)] to Stylist in components/style/selector_matching.rs, calling heap_size_of on the member in collect_reports, and fixing any compile errors that result.
@notriddle
Copy link
Contributor

@notriddle notriddle commented Aug 6, 2015

✋ Can I take this one?
@jdm jdm added the C-assigned label Aug 6, 2015
@jdm
Copy link
Member Author

@jdm jdm commented Aug 6, 2015

You bet!
@notriddle
Copy link
Contributor

@notriddle notriddle commented Aug 7, 2015

Is this how I'm supposed to be measuring the stuff in rust-selectors (the code in components/util/mem.rs)? SelectorMap is a real problem, since it's members are private I can't measure them.
@SimonSapin
Copy link
Member

@SimonSapin SimonSapin commented Aug 7, 2015

If necessary we can always make private fields public, but maybe HeapSizeOf and its derive plugin should move to external crates? What do you think @jdm?
@SimonSapin
Copy link
Member

@SimonSapin SimonSapin commented Aug 7, 2015

Note that so far we’ve moved things to separate source repositories when publishing then to crates.io, but we don’t need to.

(Though I think servo users should still get the crate form crates.io rather than through a path dependency, to force us to keep the crates.io version up to date.)
@jdm
Copy link
Member Author

@jdm jdm commented Aug 7, 2015

Yes, I think at this point it makes sense to move the definition of HeapSizeOf, heap_size_of, the plugin, and implementations for the various types from std into an external crate.
@notriddle
Copy link
Contributor

@notriddle notriddle commented Aug 7, 2015

So, should I do that? The repo really belongs in the Servo organization.
@jdm
Copy link
Member Author

@jdm jdm commented Aug 7, 2015

Like @SimonSapin said, it doesn't need to be in a separate repository.
@jdm
Copy link
Member Author

@jdm jdm commented Aug 7, 2015

I propose a new components/heapsize.
@notriddle
Copy link
Contributor

@notriddle notriddle commented Aug 7, 2015

So you're suggesting that components/heapsize be in the Servo repo, but be published to crates.io?
@jdm
Copy link
Member Author

@jdm jdm commented Aug 7, 2015

Yep.
@notriddle
Copy link
Contributor

@notriddle notriddle commented Aug 7, 2015

If it's to be built as part of Servo, wouldn't that result in two copies of the same crate? One pulled in by rust-selectors from crates.io, and the other from the checked-out repository?
@SimonSapin
Copy link
Member

@SimonSapin SimonSapin commented Aug 7, 2015

That’s why everything should be using it from crates.io, with none of the Cargo.toml files having something like path = "../heapsize".

Thinking a bit more about this, this would make CI tricky: to make and use changes to heapsize you need to first make a servo/servo PR with the changes, have it go through CI running the entire Servo test suite, then publish it, then make another PR to use it. So maybe it’s not such a good idea after all :)
@jdm
Copy link
Member Author

@jdm jdm commented Aug 7, 2015

Well, I've made https://github.com/servo/heapsize so we can make PRs against it now.
@jdm jdm mentioned this issue Aug 13, 2015
Merged
@edunham
Copy link
Contributor

@edunham edunham commented Sep 21, 2015

@notriddle Thank you for all the hard work on this PR!

Since the PR is merged, this issue can probably be closed now.
@jdm
Copy link
Member Author

@jdm jdm commented Sep 21, 2015

The PR mentioned was separate but related. The work here merely exists in a branch of notriddle/servo so far.
@KiChjang
Copy link
Member

@KiChjang KiChjang commented Mar 3, 2016

Is this being worked on by @notriddle or is it actually open to other contributors?
@notriddle
Copy link
Contributor

@notriddle notriddle commented Mar 3, 2016

I'm not working on it right now.

On Thu, Mar 3, 2016, 09:56 Keith Yeung notifications@github.com wrote:

Is this being worked on by @notriddle https://github.com/notriddle or
is it actually open to other contributors?

—
Reply to this email directly or view it on GitHub
#7038 (comment).
@KiChjang KiChjang removed the C-assigned label Mar 3, 2016
@pkondzior pkondzior mentioned this issue Mar 19, 2016
Merged
pkondzior added a commit to pkondzior/servo that referenced this issue Mar 19, 2016
@pkondzior
Report memory usage from LayoutThreadData Stylist [servo#7038]
db2e10a
@jdm jdm added the C-assigned label Mar 21, 2016
bors-servo added a commit that referenced this issue Mar 21, 2016
@bors-servo
Auto merge of #10088 - pkondzior:7038-report-memory-usage-in-layout-t…
Loading status checks…
96dc0f0 
…hread-data-stylist, r=ecoal95

Report memory usage from LayoutThreadData Stylist [#7038]

@jdm PTAL I'm not sure what is the approach of updating cargo components here, I've made a pull request servo/heapsize#54 but it has to be landed first before merge and version bump.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/10088)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Mar 22, 2016
@bors-servo
Auto merge of #10088 - pkondzior:7038-report-memory-usage-in-layout-t…
Loading status checks…
9a8ba23 
…hread-data-stylist, r=ecoal95

Report memory usage from LayoutThreadData Stylist [#7038]

@jdm PTAL I'm not sure what is the approach of updating cargo components here, I've made a pull request servo/heapsize#54 but it has to be landed first before merge and version bump.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/10088)
<!-- Reviewable:end -->
tyagiarpit added a commit to tyagiarpit/servo that referenced this issue Mar 23, 2016
@pkondzior @tyagiarpit
Report memory usage from LayoutThreadData Stylist [servo#7038]
b4fa2c5
@jdm
Copy link
Member Author

@jdm jdm commented Apr 5, 2016

Fixed by #10088.
@jdm jdm closed this Apr 5, 2016
@SimonSapin
Copy link
Member

@SimonSapin SimonSapin commented Jun 21, 2016

There are still a couple instances of #[ignore_heap_size_of = "#7038"] in components/style/properties/properties.mako.rs. @jdm, can these be fixed now?
@SimonSapin SimonSapin reopened this Jun 21, 2016
@jdm
Copy link
Member Author

@jdm jdm commented Jun 21, 2016

I would expect so.
@jdm
Copy link
Member Author

@jdm jdm commented Jun 21, 2016

It would be worth filing issues for any in particular that cannot be removed.
@KiChjang KiChjang removed the C-assigned label Jul 8, 2016
@jdm
Copy link
Member Author

@jdm jdm commented Jul 13, 2016

Unfortunately those cannot be removed because we don't know how to measure Arc values effectively.
@jdm jdm closed this Jul 13, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
E-less easy I-perf-bloat
Projects
None yet
Milestone
No milestone
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
@jdm @SimonSapin @edunham @notriddle @KiChjang
You can’t perform that action at this time.