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

Measure heap memory usage for more types. Fixes #6951 #7097

Merged
merged 1 commit into from Aug 13, 2015

Conversation

@boghison
Copy link
Contributor

boghison commented Aug 8, 2015

Also adds HeapSizeOf implementations/derive for some types. I've used "Cannot calculate Heap size" as a reason everywhere, because my imagination is rather limited. If you'd like me to change this message for specific types, please write something like this: "Trusted - Cannot calculate Heap size for Trusted" so that it would be easier for me to replace them through a script :)

Review on Reviewable

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Aug 8, 2015

☔️ The latest upstream changes (presumably #7099) made this pull request unmergeable. Please resolve the merge conflicts.

@boghison

This comment has been minimized.

Copy link
Contributor Author

boghison commented Aug 11, 2015

Is anyone there?

@pcwalton

This comment has been minimized.

Copy link
Contributor

pcwalton commented Aug 11, 2015

cc @jdm

@jdm

This comment has been minimized.

Copy link
Member

jdm commented Aug 11, 2015

Sorry about this. A bunch of large PRs have all suddenly appeared on my plate and I'm slowly working through them.

@jdm jdm self-assigned this Aug 11, 2015
@jdm

This comment has been minimized.

Copy link
Member

jdm commented Aug 11, 2015

-S-awaiting-review +S-needs-code-changes


Reviewed 174 of 174 files at r1.
Review status: all files reviewed at latest revision, 75 unresolved discussions, all commit checks successful.


components/msg/constellation_msg.rs, line 27 [r1] (raw file):
nit: move this up with the other util imports.


components/net_traits/image/base.rs, line 10 [r1] (raw file):
nit: mem comes before vec alphabetically.


components/net_traits/image/base.rs, line 28 [r1] (raw file):
"Defined in ipc-channel"


components/net_traits/lib.rs, line 65 [r1] (raw file):
"defined in hyper"


components/net_traits/lib.rs, line 68 [r1] (raw file):
"defined in hyper"


components/net_traits/lib.rs, line 258 [r1] (raw file):
"defined in hyper"


components/script/cors.rs, line 36 [r1] (raw file):
nit: mem comes before task alphabetically


components/script/cors.rs, line 50 [r1] (raw file):
"defined in hyper"


components/script/dom/bindings/error.rs, line 13 [r1] (raw file):
nit: mem comes before str


components/script/dom/bindings/js.rs, line 467 [r1] (raw file):
I don't think we should add this. Root values should not be stored on the heap. Any type that contains one should not have HeapSizeOf derived.


components/script/dom/canvasrenderingcontext2d.rs, line 68 [r1] (raw file):
IpcSender - "defined in ipc-channel"


components/script/dom/customevent.rs, line 23 [r1] (raw file):
MutHeapJSVal - "defined in rust-mozjs"


components/script/dom/dedicatedworkerglobalscope.rs, line 105 [r1] (raw file):
Receiver - "defined in std"


components/script/dom/dedicatedworkerglobalscope.rs, line 107 [r1] (raw file):
Sender - "defined in std"


components/script/dom/dedicatedworkerglobalscope.rs, line 109 [r1] (raw file):
Trusted - "Trusted<T> has unclear ownership like JS<T>"


components/script/dom/dedicatedworkerglobalscope.rs, line 111 [r1] (raw file):
ScriptChan - "can't measure trait objects"


components/script/dom/document.rs, line 1907 [r1] (raw file):
I would just remove this annotation; this shouldn't be stored in a measured heap value.


components/script/dom/element.rs, line 102 [r1] (raw file):
We should be able to measure PropertyDeclarationBlock in components/style/properties.rs.mako.


components/script/dom/errorevent.rs, line 32 [r1] (raw file):
MutHeapJSVal - "defined in rust-mozjs"


components/script/dom/htmlcollection.rs, line 36 [r1] (raw file):
"contains a trait object; can't measure due to #6870"


components/script/dom/htmlimageelement.rs, line 69 [r1] (raw file):
Let's just remove this annotation.


components/script/dom/htmlinputelement.rs, line 73 [r1] (raw file):
Why can't we measure TextInput? It's in script/textinput.rs.


components/script/dom/htmllinkelement.rs, line 243 [r1] (raw file):
I don't think we need this annotation.


components/script/dom/htmlscriptelement.rs, line 78 [r1] (raw file):
EncodingRef - "defined in rust-encoding"


components/script/dom/htmlscriptelement.rs, line 172 [r1] (raw file):
I think we can get rid of this annotation.


components/script/dom/htmlscriptelement.rs, line 633 [r1] (raw file):
I think we can get rid of this.


components/script/dom/htmltextareaelement.rs, line 44 [r1] (raw file):
I think we can measure TextInput.


components/script/dom/htmltextareaelement.rs, line 389 [r1] (raw file):
We can remove this annotation.


components/script/dom/keyboardevent.rs, line 757 [r1] (raw file):
Why not measure KeyEventProperties?


components/script/dom/node.rs, line 388 [r1] (raw file):
I don't think this is necessary.


components/script/dom/node.rs, line 1252 [r1] (raw file):
Same.


components/script/dom/node.rs, line 1270 [r1] (raw file):
Same.


components/script/dom/node.rs, line 1288 [r1] (raw file):
Same.


components/script/dom/node.rs, line 1333 [r1] (raw file):
Same.


components/script/dom/node.rs, line 1380 [r1] (raw file):
Same.


components/script/dom/node.rs, line 1398 [r1] (raw file):
Same.


components/script/dom/node.rs, line 1416 [r1] (raw file):
Same.


components/script/dom/nodeiterator.rs, line 26 [r1] (raw file):
MutHeap - "defined in rust-mozjs"


components/script/dom/nodeiterator.rs, line 30 [r1] (raw file):
Filter - "can't measure due to #6870"


components/script/dom/servohtmlparser.rs, line 56 [r1] (raw file):
I think we can get rid of this annotation.


components/script/dom/servohtmlparser.rs, line 166 [r1] (raw file):
Tokenizer - "defined in html5ever"


components/script/dom/storage.rs, line 160 [r1] (raw file):
Let's remove this annotation.


components/script/dom/textdecoder.rs, line 30 [r1] (raw file):
EncodingRef - "defined in rust-encoding"


components/script/dom/textencoder.rs, line 32 [r1] (raw file):
EncodingRef - "defined in rust-encoding"


components/script/dom/webglbuffer.rs, line 22 [r1] (raw file):
IpcSender - "defined in ipc-channel"


components/script/dom/webglframebuffer.rs, line 22 [r1] (raw file):
IpcSender - "defined in ipc-channel"


components/script/dom/webglprogram.rs, line 28 [r1] (raw file):
IpcSender - "defined in ipc-channel"


components/script/dom/webglrenderbuffer.rs, line 22 [r1] (raw file):
IpcSender - "defined in ipc-channel"


components/script/dom/webglrenderingcontext.rs, line 58 [r1] (raw file):
IpcSender - "defined in ipc-channel"


components/script/dom/webglshader.rs, line 28 [r1] (raw file):
IpcSender - "defined in ipc-channel"


components/script/dom/webgltexture.rs, line 22 [r1] (raw file):
IpcSender - "defined in ipc-channel"


components/script/dom/websocket.rs, line 74 [r1] (raw file):
Mutex - "defined in std"


components/script/dom/worker.rs, line 47 [r1] (raw file):
Sender - "defined in std"


components/script/dom/worker.rs, line 207 [r1] (raw file):
We can remove this annotation.


components/script/dom/workerglobalscope.rs, line 45 [r1] (raw file):
We can remove this annotation.


components/script/dom/workerglobalscope.rs, line 67 [r1] (raw file):
Rc - "defined in std"


components/script/dom/workerglobalscope.rs, line 70 [r1] (raw file):
ResourceTask - "defined in std"


components/script/dom/workerglobalscope.rs, line 77 [r1] (raw file):
ProfilerChan - "defined in std"


components/script/dom/workerglobalscope.rs, line 79 [r1] (raw file):
IpcSender - "defined in ipc-channel"


components/script/dom/workerglobalscope.rs, line 87 [r1] (raw file):
Receiver - "defined in std"


components/script/dom/workerglobalscope.rs, line 96 [r1] (raw file):
Sender - "defined in std"


components/script/dom/xmlhttprequest.rs, line 54 [r1] (raw file):
nit: mem before str


components/script/dom/xmlhttprequest.rs, line 87 [r1] (raw file):
Let's remove this annotation.


components/script/dom/xmlhttprequest.rs, line 120 [r1] (raw file):
Why do we need this?


components/script/dom/xmlhttprequest.rs, line 151 [r1] (raw file):
Headers - "defined in hyper"


components/script/dom/xmlhttprequest.rs, line 157 [r1] (raw file):
Headers - "defined in hyper"


components/script/dom/xmlhttprequest.rs, line 166 [r1] (raw file):
Sender - "defined in std"


components/script/dom/xmlhttprequest.rs, line 225 [r1] (raw file):
Let's remove this.


components/script/dom/xmlhttprequest.rs, line 1012 [r1] (raw file):
Let's remove this.


components/script/mem.rs, line 9 [r1] (raw file):
nit: move this above eventtarget.


components/script/mem.rs, line 240 [r1] (raw file):
We should be able to remove this branch now, I believe. If not, we should add the remaining cases and remove it.


components/script/mem.rs, line 241 [r1] (raw file):
Let's remove this and add the remaining missing branches (and return 0)


components/script/textinput.rs, line 12 [r1] (raw file):
nit: move this above util::str


components/script/textinput.rs, line 44 [r1] (raw file):
"can't easily measure this generic type"


components/util/mem.rs, line 417 [r1] (raw file):
Why can't we derive LengthOrPercentageOrAuto?


Comments from the review on Reviewable.io

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Aug 11, 2015

☔️ The latest upstream changes (presumably #7164) made this pull request unmergeable. Please resolve the merge conflicts.

@boghison

This comment has been minimized.

Copy link
Contributor Author

boghison commented Aug 12, 2015

components/util/mem.rs, line 417 [r1] (raw file):
failed to resolve. Maybe a missing extern crate util
use of undeclared trait name `util::mem::HeapSizeOf

and it's 0 anyways.


Comments from the review on Reviewable.io

@boghison

This comment has been minimized.

Copy link
Contributor Author

boghison commented Aug 13, 2015

components/script/mem.rs, line 241 [r1] (raw file):
What do you mean by this? What missing branches?


Comments from the review on Reviewable.io

@boghison

This comment has been minimized.

Copy link
Contributor Author

boghison commented Aug 13, 2015

components/script/dom/xmlhttprequest.rs, line 120 [r1] (raw file):
Because we can't ignore enum fields.


Comments from the review on Reviewable.io

@boghison

This comment has been minimized.

Copy link
Contributor Author

boghison commented Aug 13, 2015

components/script/dom/nodeiterator.rs, line 30 [r1] (raw file):
I don't think this makes sense. I didn't measure Filter because it has 2 fields which we can't calculate and 1 that is 0. There would be no point in ignoring the aforementioned 2 just to calculate Filter, which would be 0.


Comments from the review on Reviewable.io

@boghison

This comment has been minimized.

Copy link
Contributor Author

boghison commented Aug 13, 2015

components/script/dom/htmlcollection.rs, line 36 [r1] (raw file):
This situation is the same as filter. Are you sure this should be the explanation?


Comments from the review on Reviewable.io

@boghison

This comment has been minimized.

Copy link
Contributor Author

boghison commented Aug 13, 2015

Review status: all files reviewed at latest revision, 55 unresolved discussions, some commit checks failed.


components/script/dom/element.rs, line 102 [r1] (raw file):
Well I added the derive, is there a way to update the template ie generate it with the edits, because it still says that method not found (heap_size_of_children)


components/script/dom/htmlinputelement.rs, line 73 [r1] (raw file):
Apparently we can't, probably because of that T, ignoring it doesn't seem to work


Comments from the review on Reviewable.io

@boghison

This comment has been minimized.

Copy link
Contributor Author

boghison commented Aug 13, 2015

@jdm

This comment has been minimized.

Copy link
Member

jdm commented Aug 13, 2015

Review status: all files reviewed at latest revision, 55 unresolved discussions, some commit checks failed.


components/script/dom/htmlcollection.rs, line 36 [r1] (raw file):
Yes. CollectionTypeId contains a variant with a trait object that we can't ignore.


components/script/dom/htmlinputelement.rs, line 73 [r1] (raw file):
Fascinating! Please file an issue like "Can't derive HeapSizeOf for types with type arguments that don't derive HeapSizeOf" and use that in this message instead.


components/script/dom/nodeiterator.rs, line 30 [r1] (raw file):
I think there is a point - we may be able to measure trait objects in the future, and then we could just update the specific cases where it's a problem, rather than looking for types we ignored because it used to be a problem.


components/script/dom/xmlhttprequest.rs, line 120 [r1] (raw file):
I don't think we need to measure XHRProgress, so let's remove this.


components/script/mem.rs, line 241 [r1] (raw file):
Any branches that _ still covers. I want this match to be exhaustive.


Comments from the review on Reviewable.io

@boghison

This comment has been minimized.

Copy link
Contributor Author

boghison commented Aug 13, 2015

components/script/dom/element.rs, line 102 [r1] (raw file):
But how do I get past errors such as:
the method heap_size_of_children exists but the following trait bounds were not satisfied: properties::longhands::perspective_origin::SpecifiedValue : util::mem::HeapSizeOf, properties::longhands::perspective_origin::SpecifiedValue : util::mem::HeapSizeOf


Comments from the review on Reviewable.io

@jdm

This comment has been minimized.

Copy link
Member

jdm commented Aug 13, 2015

Review status: all files reviewed at latest revision, 55 unresolved discussions, some commit checks failed.


components/script/dom/element.rs, line 102 [r1] (raw file):
Those are covered by the commits in #7038, but for now let's just add ignore attributes to the members of PropertyDeclarationBlock and reference that issue.


Comments from the review on Reviewable.io

@boghison

This comment has been minimized.

Copy link
Contributor Author

boghison commented Aug 13, 2015

components/script/mem.rs, line 241 [r1] (raw file):
You mean like WebSocket and FileReader? I don't know how to cover those. If I for example do WebSocketCast..., I get error: the trait dom::bindings::codegen::InheritTypes::WebSocketDerived is not implemented for the type dom::eventtarget::EventTarget


Comments from the review on Reviewable.io

@jdm

This comment has been minimized.

Copy link
Member

jdm commented Aug 13, 2015

Review status: all files reviewed at latest revision, 55 unresolved discussions, some commit checks failed.


components/script/mem.rs, line 241 [r1] (raw file):
Right, I said above that we should just return 0 for them.


Comments from the review on Reviewable.io

@boghison

This comment has been minimized.

Copy link
Contributor Author

boghison commented Aug 13, 2015

@jdm Is this ok now?

@jdm

This comment has been minimized.

Copy link
Member

jdm commented Aug 13, 2015

I think there's only one missing change.
-S-awaiting-review -S-needs-rebase +S-needs-code-changes


Reviewed 47 of 48 files at r2.
Review status: all files reviewed at latest revision, 6 unresolved discussions, all commit checks successful.


components/script/dom/workerglobalscope.rs, line 96 [r1] (raw file):
What about this one?


Comments from the review on Reviewable.io

@boghison

This comment has been minimized.

Copy link
Contributor Author

boghison commented Aug 13, 2015

Oh, sorry :P What about now?

@jdm

This comment has been minimized.

Copy link
Member

jdm commented Aug 13, 2015

Sorry, one more!


Reviewed 1 of 48 files at r2, 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 7 unresolved discussions, all commit checks successful.


components/script/dom/nodelist.rs, line 23 [r1] (raw file):
What happened here?


Comments from the review on Reviewable.io

@boghison

This comment has been minimized.

Copy link
Contributor Author

boghison commented Aug 13, 2015

@jdm

This comment has been minimized.

Copy link
Member

jdm commented Aug 13, 2015

Review status: all files reviewed at latest revision, 7 unresolved discussions, all commit checks successful.


components/script/dom/nodelist.rs, line 0 [r1] (raw file):
The changes to add HeapSizeOf were reverted while rebasing.


Comments from the review on Reviewable.io

@boghison

This comment has been minimized.

Copy link
Contributor Author

boghison commented Aug 13, 2015

components/script/dom/nodelist.rs, line 23 [r1] (raw file):
I'm afraid I'm not following you.


Comments from the review on Reviewable.io

@jdm

This comment has been minimized.

Copy link
Member

jdm commented Aug 13, 2015

Review status: all files reviewed at latest revision, 7 unresolved discussions, all commit checks successful.


components/script/dom/nodelist.rs, line 0 [r1] (raw file):
Reviewable is not showing useful results for this comment. Take a look at the complete diff for the changes on github - there is no HeapSizeOf added to NodeList, as it was accidentally removed in the rebase. Reviewable shows this if you look at nodelist.rs for r1->r2.


Comments from the review on Reviewable.io

@boghison

This comment has been minimized.

Copy link
Contributor Author

boghison commented Aug 13, 2015

Just notifying you of the commit 🐱

@jdm

This comment has been minimized.

Copy link
Member

jdm commented Aug 13, 2015

@bors-servo: r+
Thanks for doing this work!


Reviewed 1 of 2 files at r4.
Review status: 174 of 175 files reviewed at latest revision, 7 unresolved discussions, all commit checks successful.


Comments from the review on Reviewable.io

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Aug 13, 2015

📌 Commit 4514510 has been approved by jdm

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Aug 13, 2015

⌛️ Testing commit 4514510 with merge a03616f...

bors-servo pushed a commit that referenced this pull request Aug 13, 2015
bors-servo
Measure heap memory usage for more types. Fixes #6951

Also adds HeapSizeOf implementations/derive for some types. I've used "Cannot calculate Heap size" as a reason everywhere, because my imagination is rather limited. If you'd like me to change this message for specific types, please write something like this: "Trusted - Cannot calculate Heap size for Trusted" so that it would be easier for me to replace them through a script :)

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7097)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Aug 13, 2015

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

@bors-servo bors-servo merged commit 4514510 into servo:master Aug 13, 2015
2 checks passed
2 checks passed
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
Projects
None yet
5 participants
You can’t perform that action at this time.