Add plugin for autoderiving HeapSize #6188

Manishearth · 2015-05-26T18:24:08Z

Fixes #5914

r? @nnethercote for the gfx changes

r? @kmcallister or @jdm for the plugin

highfive · 2015-05-26T18:24:12Z

Warning

These commits modify unsafe code. Please review it carefully!

hoppipolla-critic-bot · 2015-05-26T18:24:13Z

Critic review: https://critic.hoppipolla.co.uk/r/5103

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend) when updating this pull request.

jdm · 2015-05-26T18:27:14Z

2.66s$ ./mach test-tidy
components/plugins/heapsize.rs:1: incorrect license
components/plugins/heapsize.rs:10: (much) overlong line
components/plugins/lib.rs:11: (much) overlong line
components/util/mem.rs:188: no newline at EOF

nnethercote · 2015-05-27T06:20:47Z

Basically it looks good but seems like some clean-up needs to be done before landing; I'd like to see it again before that happens. In particular...

The naming is inconsistent: heap_size, heapsize, expand_heapsize, known_heap_size, etc. At the very least there should always be a _ in heap_size.
Lots of scalar fields are currently marked with #[ignore_heapsize]. It would be better to remove these, using known_heap_size if necessary.
Once that's done, it should be easier to achieve the following, worthy goal: every #[ignore_heap_size] annotation should have an accompanying comment explaining why it's ignored. There are a couple of examples in there currently. (One nice thing about the new style is that it's much easier to comment an annotation than to comment an omitted field measurement in heap_size_of_children.)

Manishearth · 2015-05-27T06:22:50Z

Sounds good -- I'll fix up these nits later today, and work on unignoring scalar fields and we can work on explaining ignores later.

I could also force #[ignore_heap_size] annotations to have an associated reason btw. (e.g. #[ignore_heap_size = "fixme(njn)"])

nnethercote · 2015-05-27T06:24:48Z

The reason is typically a short-to-medium length sentence. I'm not sure if that fits well within a #[...] annotation? But maybe there are other cases where that it acceptable? Not sure.

Manishearth · 2015-05-27T06:25:35Z

Annotations can be indented across lines :)

Manishearth · 2015-05-27T17:23:32Z

@nnethercote: removed most of the ignores.

nnethercote · 2015-05-27T23:02:00Z

components/gfx/display_list/mod.rs

 /// Represents one CSS stacking context, which may or may not have a hardware layer.
 pub struct StackingContext {
    /// The display items that make up this stacking context.
    pub display_list: Box<DisplayList>,

    /// The layer for this stacking context, if there is one.
+    #[ignore_heap_size = "FIXME(njn): other fields may be measured later, esp. `layer`"]


It looks like this is the only unmeasured field in StackingContext now, in which case this comment should just say something like "FIXME(njn): should measure this at some point".

nnethercote · 2015-05-27T23:05:49Z

components/gfx/display_list/mod.rs

 pub struct TextDisplayItem {
    /// Fields common to all display items.
    pub base: BaseDisplayItem,

    /// The text run.
+    #[ignore_heap_size = "We exclude this because it is non-owning"]


"Because it is non-owning" is probably enough for the comment? The "We exclude this" part is redundant once you know that the string is an explanation for the ignore.

nnethercote · 2015-05-27T23:06:05Z

components/gfx/display_list/mod.rs

 pub struct ImageDisplayItem {
    pub base: BaseDisplayItem,
+    #[ignore_heap_size = "We exclude this here because it is non-owning"]


Again, "Because it is non-owning" is enough.

nnethercote · 2015-05-27T23:09:16Z

components/plugins/heapsize.rs

+use syntax::ext::build::AstBuilder;
+use syntax::ext::deriving::generic::*;
+
+pub fn expand_heapsize(cx: &mut ExtCtxt, span: Span, mitem: &MetaItem,


expand_heap_size, for consistency

nnethercote · 2015-05-27T23:09:31Z

components/plugins/heapsize.rs

+
+// Mostly copied from syntax::ext::deriving::hash
+/// Defines how the implementation for `trace()` is to be generated
+fn heapsize_substructure(cx: &mut ExtCtxt, trait_span: Span, substr: &Substructure) -> P<Expr> {


heap_size_substructure, for consistency

nnethercote · 2015-05-27T23:12:11Z

components/plugins/heapsize.rs

+    };
+
+    fields.iter().fold(cx.expr_usize(trait_span, 0),
+                       |acc, ref item| {


Nit: is it considered ok Rust style to move the |acc, ref item| { to the previous line? Then the body of the closure would only need to be indented four spaces. I've seen something similar to that used a lot in JS code.

nnethercote · 2015-05-27T23:12:57Z

components/plugins/lib.rs

@@ -31,6 +31,8 @@ use syntax::parse::token::intern;
 // Public for documentation to show up
 /// Handles the auto-deriving for `#[jstraceable]`
 pub mod jstraceable;
+/// Handles the auto-deriving for `#[heapsize]`
+pub mod heapsize;


heap_size, for consistency

nnethercote · 2015-05-27T23:14:22Z

components/util/mem.rs

+use geometry::Au;
+use range::Range;
+use cursor::Cursor;
+use azure::azure_hl::Color;


This is one of those cases where everybody has a different way of ordering and grouping their use items, right? :(

nnethercote · 2015-05-27T23:14:42Z

components/util/mem.rs

@@ -158,3 +165,38 @@ impl<T> Drop for LinkedList2<T> {
    fn drop(&mut self) {}
 }

+/// For use on types defined in external crates
+/// with known heap sizes


Nit: end sentence with '.', please :)

nnethercote · 2015-05-27T23:15:50Z

components/plugins/heapsize.rs

+fn heapsize_substructure(cx: &mut ExtCtxt, trait_span: Span, substr: &Substructure) -> P<Expr> {
+    let fields = match *substr.fields {
+        Struct(ref fs) | EnumMatching(_, _, ref fs) => fs,
+        _ => cx.span_bug(trait_span, "impossible substructure in `heapsize`")


heap_size

nnethercote · 2015-05-27T23:17:22Z

components/plugins/heapsize.rs

+}
+
+// Mostly copied from syntax::ext::deriving::hash
+/// Defines how the implementation for `trace()` is to be generated


Nit: end sentence with '.'.

nnethercote · 2015-05-27T23:18:20Z

components/plugins/heapsize.rs

+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+use syntax::ext::base::{Annotatable, ExtCtxt};


I think this file should have a top-level comment briefly explaining what it's for, and when to use the ignore_heap_size annotation.

nnethercote · 2015-05-27T23:19:20Z

components/plugins/heapsize.rs

+                                    if a.check_name("ignore_heap_size") {
+                                        match a.node.value.node {
+                                            MetaNameValue(..) => (),
+                                            _ => cx.span_err(a.span, "#[ignore_heap_size] \


Would ignore_heap_size_of or ignore_for_heap_size_of be better? I like having the "of" in there for consistency with HeapSizeOf and heap_size_of_children.

nnethercote · 2015-05-27T23:20:35Z

I have lots of nits above, but generally this is looking really nice. Avoiding all this boilerplate will make this code less error-prone, and easier to improve coverage in the future. Thank you for doing it.

Manishearth · 2015-05-28T01:13:08Z

Comments addressed.

You're welcome! :)

pcwalton · 2015-05-28T16:12:37Z

I think this may be worth a blog post! It's certainly a nice example of using Rust to speed up browser development.

Manishearth · 2015-05-28T16:33:24Z

On blog.s.org? Sounds good! 😄

(Two months of being blocked on things has finally paid off!)

nnethercote · 2015-05-28T23:06:23Z

I think this may be worth a blog post!

I've been thinking about writing one myself, comparing the C++ code for this stuff in Firefox to the Rust code for this in Servo. We should avoid both writing one, though...

bors-servo · 2015-06-03T01:40:34Z

💔 Test failed - mac1

…Of` impls (fixes #5914)

Manishearth · 2015-06-03T01:45:12Z

@bors-servo r=nnethercote

bors-servo · 2015-06-03T01:45:13Z

📌 Commit d2496a4 has been approved by nnethercote

bors-servo · 2015-06-03T02:05:56Z

⌛ Testing commit d2496a4 with merge 60ac15e...

bors-servo · 2015-06-03T02:11:43Z

💔 Test failed - linux1

Manishearth · 2015-06-03T03:50:43Z

@bors-servo retry

bors-servo · 2015-06-03T04:02:09Z

⌛ Testing commit d2496a4 with merge 1b75693...

@nnethercote

Fixes #5914 r? @nnethercote for the gfx changes r? @kmcallister or @jdm for the plugin  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/6188)

bors-servo · 2015-06-03T04:07:21Z

💔 Test failed - linux1

Manishearth · 2015-06-03T04:21:23Z

Ugh, messed up the rebase

Manishearth · 2015-06-03T04:27:55Z

@bors-servo r=nnethercote

bors-servo · 2015-06-03T04:27:55Z

📌 Commit d347aed has been approved by nnethercote

bors-servo · 2015-06-03T04:48:03Z

⌛ Testing commit d347aed with merge 2ca606a...

@nnethercote

Fixes #5914 r? @nnethercote for the gfx changes r? @kmcallister or @jdm for the plugin  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/6188)

bors-servo · 2015-06-03T05:27:38Z

☀️ Test successful - android, gonk, linux1, linux2, linux3, mac1, mac2

nnethercote · 2015-06-03T05:36:49Z

\o/

highfive added the S-awaiting-review label May 26, 2015

jdm added the S-fails-tidy label May 26, 2015

Manishearth removed the S-fails-tidy label May 26, 2015

nnethercote reviewed May 27, 2015
View changes

components/plugins/heapsize.rs Outdated

}

// Mostly copied from syntax::ext::deriving::hash

/// Defines how the implementation for `trace()` is to be generated

This comment has been minimized.

Sign in to view

nnethercote May 27, 2015
Contributor

Nit: end sentence with '.'.

nnethercote reviewed May 27, 2015
View changes

Manishearth force-pushed the Manishearth:sizeof branch from 80ad7da to 733acdf Jun 1, 2015

Manishearth added 8 commits Mar 31, 2015

Add #[heapsize]/#[derive(HeapSizeOf)] plugin to auto-derive `HeapSize…

f4bc925

…Of` impls (fixes #5914)

use HeapSizeOf plugin in gfx

c1daf88

use knownheapsize for geom stuff

a5975e8

remove more heapsize ignores

5447d2a

Remove all Arc-less ignores, force reasons, ignore_heap_size_of

13b4bcf

address review comments

2551cce

Add unit test for plugin

7bc88d5

fix rebase issues

Loading status checks…

d2496a4

Manishearth force-pushed the Manishearth:sizeof branch from 1b9293b to d2496a4 Jun 3, 2015

fix rebase conflicts

Loading status checks…

d347aed

Manishearth force-pushed the Manishearth:sizeof branch from 54b12f2 to d347aed Jun 3, 2015

bors-servo merged commit d347aed into servo:master Jun 3, 2015
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

Manishearth deleted the Manishearth:sizeof branch Jun 3, 2015

servo / servo

Join GitHub today

GitHub is where the world builds software

Add plugin for autoderiving HeapSize #6188

Add plugin for autoderiving HeapSize #6188

Conversation

Manishearth commented May 26, 2015

highfive commented May 26, 2015

hoppipolla-critic-bot commented May 26, 2015

jdm commented May 26, 2015

nnethercote commented May 27, 2015

Manishearth commented May 27, 2015

nnethercote commented May 27, 2015

Manishearth commented May 27, 2015

Manishearth commented May 27, 2015

This comment has been minimized.

nnethercote May 27, 2015 Contributor

This comment has been minimized.

nnethercote May 27, 2015 Contributor

This comment has been minimized.

nnethercote May 27, 2015 Contributor

This comment has been minimized.

nnethercote May 27, 2015 Contributor

This comment has been minimized.

nnethercote May 27, 2015 Contributor

This comment has been minimized.

nnethercote May 27, 2015 Contributor

This comment has been minimized.

nnethercote May 27, 2015 Contributor

This comment has been minimized.

nnethercote May 27, 2015 Contributor

This comment has been minimized.

nnethercote May 27, 2015 Contributor

This comment has been minimized.

nnethercote May 27, 2015 Contributor

This comment has been minimized.

nnethercote May 27, 2015 Contributor

This comment has been minimized.

nnethercote May 27, 2015 Contributor

This comment has been minimized.

nnethercote May 27, 2015 Contributor

nnethercote commented May 27, 2015

Manishearth commented May 28, 2015

pcwalton commented May 28, 2015

Manishearth commented May 28, 2015

nnethercote commented May 28, 2015

bors-servo commented Jun 3, 2015

Manishearth commented Jun 3, 2015

bors-servo commented Jun 3, 2015

bors-servo commented Jun 3, 2015

bors-servo commented Jun 3, 2015

Manishearth commented Jun 3, 2015

bors-servo commented Jun 3, 2015

bors-servo commented Jun 3, 2015

Manishearth commented Jun 3, 2015

Manishearth commented Jun 3, 2015

bors-servo commented Jun 3, 2015

bors-servo commented Jun 3, 2015

bors-servo commented Jun 3, 2015

nnethercote commented Jun 3, 2015

Essential cookies

Always active

Analytics cookies

nnethercote May 27, 2015
Contributor

nnethercote May 27, 2015
Contributor

nnethercote May 27, 2015
Contributor

nnethercote May 27, 2015
Contributor

nnethercote May 27, 2015
Contributor

nnethercote May 27, 2015
Contributor

nnethercote May 27, 2015
Contributor

nnethercote May 27, 2015
Contributor

nnethercote May 27, 2015
Contributor

nnethercote May 27, 2015
Contributor

nnethercote May 27, 2015
Contributor

nnethercote May 27, 2015
Contributor

nnethercote May 27, 2015
Contributor