1552080 - Add refcount logging for servo_arc

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, task, P3)

Description

[Emilio Cobos Álvarez [:emilio]]

Should be useful to catch silly mistakes like the nsString logging caught https://phabricator.services.mozilla.com/D31319?vs=104569&id=104702.

Though we leak some stuff "on purpose" (the UA cascade data cache may come to mind, the static arc that I added for ArcSlice::default() too) and we may need to do something that accounts for that / cleans those up.

Comment 1

[Emilio Cobos Álvarez [:emilio]]

Hmm, the ThinArc / RawOffsetArc business may not make this exactly trivial...

Comment 2

[Emilio Cobos Álvarez [:emilio]]

I tried this, but I quickly hit assertions about reporting different sizes for different type classes.

This is not great, because we cannot just move the refcount logging one level up everywhere, and we cannot use different names because std::intrinsics::type_name is unstable.

Nick, any idea?

Comment 3

[Emilio Cobos Álvarez [:emilio]]

In particular this is the assertion I'm hitting (expectedly, I guess, given what I'm doing). Maybe Eric / Andrew also have ideas?

https://searchfox.org/mozilla-central/rev/a887c90ea9c19a0b5529a1f5fa351929944887ba/xpcom/base/nsTraceRefcnt.cpp#339

Comment 4

[Andrew McCreight [:mccr8]]

I talked to Emilio about this in IRC. Basically, your choices are to either statically generate strings for type names, which sounds like it won't really work, or just use the same size for all of the arcs. I argued that losing specific size information isn't a huge loss. If it turns out to be, some followup work can investigate improvements.

Comment 5

[Emilio Cobos Álvarez [:emilio]]

So current try push is:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=910b474ac929e35fe64c4520adb0e09018d05dbf

Looks like we're leaking really small amount of references but consistently. So my guess is that we're leaving some Arc<> in a static or such.

Well, no wonder, here's one:

https://searchfox.org/mozilla-central/rev/a887c90ea9c19a0b5529a1f5fa351929944887ba/servo/components/style_traits/arc_slice.rs#42

Others could be the UA data cache or what not... I guess I'll have to hack up something to see the type (probably using RUSTC_BOOTSTRAP=1 and the intrinsic thing), because otherwise this is going to be fun to chase down.

Comment 6

[Nicholas Nethercote [inactive]]

I don't know much about this bloat stuff, so I defer to mccr8's opinion.

Comment 7

[Emilio Cobos Álvarez [:emilio]]

These allocation stacks are not very encouraging :(

allocation stack:
#00: ???[/home/emilio/src/moz/gecko-artifact/obj-x86_64-pc-linux-gnu/dist/bin/libxul.so +0x6bf69ed]
#01: _$LT$servo_arc..Arc$LT$servo_arc..HeaderSlice$LT$H$C$$u20$$u5b$T$u5d$$GT$$GT$$GT$::from_header_and_iter_alloc::ha8e308758b837277[/home/emilio/src/moz/gecko-artifact/obj-x86_64-pc-linux-gnu/dist/bin/libxul.so +0x127dd697]
12 @0x7f7ac0d3dc80 (0 references; 0 from COMPtrs)
allocation stack:
#00: ???[/home/emilio/src/moz/gecko-artifact/obj-x86_64-pc-linux-gnu/dist/bin/libxul.so +0x6bf69ed]
#01: _$LT$servo_arc..Arc$LT$servo_arc..HeaderSlice$LT$H$C$$u20$$u5b$T$u5d$$GT$$GT$$GT$::from_header_and_iter_alloc::h1c0599374377bfe6[/home/emilio/src/moz/gecko-artifact/obj-x86_64-pc-linux-gnu/dist/bin/libxul.so +0x1314f95b]
2101 @0x7f7ac0d72000 (0 references; 0 from COMPtrs)
allocation stack:
#00: ???[/home/emilio/src/moz/gecko-artifact/obj-x86_64-pc-linux-gnu/dist/bin/libxul.so +0x6bf69ed]
#01: _$LT$servo_arc..Arc$LT$T$GT$$GT$::new::h83923b4cf302a259[/home/emilio/src/moz/gecko-artifact/obj-x86_64-pc-linux-gnu/dist/bin/libxul.so +0x127b5ad4]
28 @0x7f7ac0d40ea0 (0 references; 0 from COMPtrs)
allocation stack:
#00: ???[/home/emilio/src/moz/gecko-artifact/obj-x86_64-pc-linux-gnu/dist/bin/libxul.so +0x6bf69ed]
#01: _$LT$servo_arc..Arc$LT$T$GT$$GT$::new::hef89c649fce1be16[/home/emilio/src/moz/gecko-artifact/obj-x86_64-pc-linux-gnu/dist/bin/libxul.so +0x127b6f9b]
2102 @0x7f7abf06e000 (0 references; 0 from COMPtrs)
allocation stack:
#00: ???[/home/emilio/src/moz/gecko-artifact/obj-x86_64-pc-linux-gnu/dist/bin/libxul.so +0x6bf69ed]
#01: _$LT$servo_arc..Arc$LT$T$GT$$GT$::new::hfd7bda5abbd2adc8[/home/emilio/src/moz/gecko-artifact/obj-x86_64-pc-linux-gnu/dist/bin/libxul.so +0x127b7574]

(Forget about the gecko-artifact part of the name, it's a regular debug build, just used that spare directory)

Comment 8

[Emilio Cobos Álvarez [:emilio]]

I found most of them using rr, but I want to see if I can get proper stack traces by using RUSTFLAGS="-C force-frame-pointers=y".

Comment 9

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1552080 - Add refcount logging to servo_arc. r=bholley,heycam

Comment 10

[Andrew McCreight [:mccr8]]

You need to run tools/rb/fix_linux_stack.py on the stack trace output to symbolicate it.

Comment 11

[Emilio Cobos Álvarez [:emilio]]

Yeah, that helps, but the stack trace is only two frames deep unfortunately, so I don't see the interesting stuff.

Comment 12

[Andrew McCreight [:mccr8]]

Oh, sorry. I didn't notice that it was the same stack over and over again.

Comment 13

[Emilio Cobos Álvarez [:emilio]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=238e558fb73569cd5b769203d927efc8fa435289

Comment 14

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1552080 - Rejigger a bit rust features so that rusttests still link. r=froydnj

We cannot compile with just feature(gecko + debug_assertions), since that's how
debug rusttests get compiled and they don't have the refcount logging stuff.

We were getting away with it for the pre-existing usage of the style crate,
because it wasn't used during any test and presumably the linker didn't
complain. But servo_arc is definitely used in tests.

Comment 15

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1552080 - Don't clobber library features with test features in the make backend. r=#build

We weren't honoring the case where the library features differ from the tests
features (situation which my previous patch does).

We were incorrectly overriding rust_feature_flags, which of course ended up
with a working rusttests with my patches, but a bunch of negative leaks :)

Name the test features differently so that they don't affect the regular library
features.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a4cb3416b991
Don't clobber library features with test features in the make backend. r=chmanchester

Comment 17

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/a4cb3416b991

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0edd8c431471
Add refcount logging to servo_arc. r=heycam
https://hg.mozilla.org/integration/autoland/rev/0a53924e7249
Rejigger a bit rust features so that rusttests still link. r=froydnj

Comment 19

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/0edd8c431471
https://hg.mozilla.org/mozilla-central/rev/0a53924e7249