Closed Bug 1552080 Opened 5 months ago Closed 5 months ago

Add refcount logging for servo_arc

Categories

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

task

Tracking

()

RESOLVED FIXED

People

(Reporter: emilio, Assigned: emilio)

References

(Regressed 1 open bug)

Details

Attachments

(3 files)

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.

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

Flags: needinfo?(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?

Flags: needinfo?(emilio) → needinfo?(n.nethercote)

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

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.

Flags: needinfo?(n.nethercote)

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.

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

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)

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".

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

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

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

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.

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.

Type: defect → task
Keywords: leave-open
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a4cb3416b991
Don't clobber library features with test features in the make backend. r=chmanchester
Attachment #9067746 - Attachment description: Bug 1552080 - Regigger a bit rust features so that rusttests still link. r=#build → Bug 1552080 - Regigger a bit rust features so that rusttests still link. r=froydnj
Pushed by emilio@crisal.io:
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
Attachment #9067746 - Attachment description: Bug 1552080 - Regigger a bit rust features so that rusttests still link. r=froydnj → Bug 1552080 - Rejigger a bit rust features so that rusttests still link. r=froydnj
Status: NEW → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Regressions: 1569325
You need to log in before you can comment on or make changes to this bug.