mozilla :: #servo

7 Sep 2017
00:00xidornfroydnj: :)
00:01heycamnjn: do you think a DMD run on AWSY would reveal anything different from a local run just on a wikipedia page?
00:02njnheycam: quite possibly
00:02njnheycam: have you tried a local AWSY run?
00:02heycamnjn: not yet
00:02njnit's a significantly different workload, so I can imagine the outcome being different
00:02heycamnjn: ok. I will try that today, and then get your help to interpret the results.
00:31xidornfroydnj: mind having a look at the new comment added there?
00:33froydnjxidorn: in 1:1, will look after
00:33xidornfroydnj: no rush
00:35heycamnjn: do you know off hand how I can enable DMD without the use of "./mach run --dmd" (since "./mach awsy-test" doesn't understand that option?)
00:35heycamlooks like there is some LD_PRELOAD / LD_LIBRARY_PATH stuff
00:55bzbholley: https://bugzilla.mozilla.org/show_bug.cgi?id=1397380#c6 and https://bugzilla.mozilla.org/show_bug.cgi?id=1397380#c8
00:55firebotBug 1397380 NEW, nobody@mozilla.org stylo: Investigate sources of stylo memory overhead on the html single-page spec
00:58njnheycam: yeah, there is...
00:59njnheycam: http://searchfox.org/mozilla-central/source/python/mozbuild/mozbuild/mach_commands.py#1437-1473 is the relevant part of `mach run`
00:59njnheycam: I suggest adding some print statements there to see what the env vars get set to on your machine
00:59heycamnjn: ok, thanks
01:00njnheycam: https://developer.mozilla.org/en-US/docs/Mozilla/Performance/DMD#Launch_11 is semi-relevant, too
01:01heycamnjn: oh, DMD=1
01:02* njn realizes he can remove that B2G docs
01:10bzheycam: ping
01:10heycambz: hello
01:10bzheycam: I just tried applying https://bugzilla.mozilla.org/attachment.cgi?id=8904785 locally but I get merge conflicts.... which rev does it apply on top of?
01:11* bz can probably just resolve the conflicts too, if the exact rev is not critical
01:11heycambz: probably better to grab yesterday's patch after I rebased again, from one of my try runs
01:11bholleyheycam: (heading out, will be back in a bit, but wanted to ask quick): I'm surprised that the overhead of storing unused reset structs cancels out the gains of more sharing
01:11njnbholley: I'm measuring Selectors from StyleRules. That gets a lot of them, but there are still some being missed
01:12bzheycam: link?
01:12heycambz: see the try hg links on the spreadsheet
01:12bholleyheycam: it seems like, at least after the initial styling, there should never be unused reset structs in the rule tree
01:12bzheycam: Thanks
01:12heycambholley: I'm think it's the structs for the cache storage itself
01:12bzSo like https://hg.mozilla.org/try/rev/0f0a276f8abe18b05d2289177e0f29e3500a092f ?
01:12heycambholley: I'm just working on shrinking that some more
01:12heycambz: yep
01:12bholleyheycam: ok
01:13heycambholley: because otherwise yes the numbers don't really make sense
01:13bholleyheycam: ok. Definitely worth investigating. And I guess the alternative is always to go with sharing instead of caching
01:13heycambholley: indeed
01:13bzheycam: thanks!
01:13bholleybz: would be good to measure sharing and compare against caching
01:14bholleyit does seem like the sharing approach gets a lot worse with STYLO_THREADS=4
01:15heycambholley: I'm a little worried about noise in these numbers
01:15heycambholley: given how different the toplines numbers can be on treeherder
01:15bzhold on
01:15bzwhat do we mean by "sharing" and "caching"?
01:15bzIs the rev I am looking at something I should measure?
01:15bholleybz: sharing is emilio's TLS approach. caching is heycam's rule tree approach
01:15* bz was goign to try it on the HTML spec and see what happens
01:16bholleybz: they are mutually exclusive strategies for reusing reset struts
01:16bzah
01:16bholley*structs
01:16bzSo https://hg.mozilla.org/try/rev/0f0a276f8abe18b05d2289177e0f29e3500a092f is "sharing"?
01:16bholleybz: we decided to pursue caching because (a) it works across threads, and (b) it works across restyles
01:16heycambz: no that's "caching"
01:16bholleybz: https://docs.google.com/spreadsheets/d/1B-UcOIAvA6SEvYRVABV51aoud8JUr-dbJzJ1Z8-lBWY/edit#gid=2059636988
01:16heycam(see the names to the left of those links on the spreadsheet)
01:16bzmmm.. commit message says "sharing". ;)
01:16bzalright
01:17heycambz: oops ok :)
01:17bholleybz: yes, the terminology distinction grew organically when heycam made the spreadsheet
01:17bzGotcha
01:17bzSo ok
01:17bzHere is my plan
01:17bzI am going to apply "caching"
01:17bzAnd then redo my HTML spec measurements at 1, 3, 6 threads
01:17bzThen remov that and apply sharing and redo those
01:17bzSeem plausible?
01:17bholleybz: yes, perfect
01:18bzI'll post all the about:memory dumps in the bug so people can grab and diff
01:18heycambz: sounds good
01:18bholleyheycam: meanwhile, would it be feasible to alter your data pipeline to do multiple retriggers and then average the values from the reports?
01:18bzI'll also try to poke at gmail, but that might not happen tonight... we'll see
01:18bholleyheycam: that would cut down the noise a lot
01:18heycambholley: yes I'm thinking to do that today
01:18* bz spent a while reading DMD output
01:18bholleysounds good
01:18bzsome of it is sad reading.
01:18bholleyxidorn: can you pursue the font family thing?
01:18bzLike all those textnode slots we allocate just to store a bindingparent...
01:19bholleyxidorn: (assuming it's straightforward)
01:19bzNote that the font family thing is less of a problem with better caching.
01:19bzer, sharing
01:19bzwhatever
01:19bz"fewer font structs"
01:19heycam:)
01:19njnbz, heycam: would wikipedia pages contains any of: MediaRule, DocumentRule, SupportsRule?
01:19heycamdo we have any plans to improve regular CV sharing?
01:19heycams/plans/ideas/
01:19heycamI guess no
01:20heycambz: MediaRule for sure
01:20bholleyheycam: I'm a bit surprised it's as bad as it is with STYLO_THREADS=1
01:20bznjn: Seems likely
01:20heycamSupportsRule could be. DocumentRule unlikely.
01:20njnok
01:20bholleyper https://bugzilla.mozilla.org/show_bug.cgi?id=1397380#c6
01:20firebotBug 1397380 NEW, nobody@mozilla.org stylo: Investigate sources of stylo memory overhead on the html single-page spec
01:20njnI'm missing some Selectors, those rule types have nested CSSRules which I'm not measuring
01:20bzSo with STYLO_THREADS=1 we typically get about 2x as many ComputedValues as style contexts
01:20xidornbholley: I can, but not sure whether it's worth if we are going to have some better style struct sharing
01:20bz(gecko style contexts)
01:20bholleybz: how do the sizes compare? Do we actually have more ServoStyleContexts than GeckoStyleContexts? Or are they just bigger?
01:20heycamxidorn: unlikely we'll do any struct sharing thing that helps Font structs
01:20heycam(being an inherited struct)
01:21bzbholley: we have more ServoStyleContexts
01:21bzbholley: typically by 2x with STYLO_THREADS=1, by 4x with more threads
01:21bholleybz: here's an idea
01:21bholleybz: once we compute the rule node
01:21bholleybz: doing a second pass over the cache
01:21bzbholley: at least that seems to be approximately what happens on pages I've measured....
01:22xidornheycam: hmmm. I guess putting refcounted FontFamilyList into specified value may help, but that doesn't look very good from servo's point of view I guess
01:22heycamxidorn: mmm. conditional compilation? :)
01:22heycamas with other things, like urls, it's a bit awkward
01:22xidornheycam: well... maybe?
01:22xidornI can probably try
01:22bholleybz: that _should_ give us identical sharing to gecko in the STYLO_THREADS=1 case
01:23bholleybz: heycam wdyt?
01:23heycambholley: are you talking about priming of the style sharing cache with structs from the rule node cache?
01:23bholleyheycam: no
01:23bholleyheycam: I'm saying right now the style sharing cache is oriented towards avoiding selector matching and cascading
01:24bholleyheycam: but once we do selector matching and get a rule node, we can still potentially avoid cascading, by seeing if any siblings/cousins have the same rule node
01:24bholleyheycam: which is exactly how the gecko style context tree works
01:24heycambholley: oh, interesting
01:25heycambholley: no need to look over style context tree (which we can't do any more), but we can look at our DOM siblings
01:25bzbholley: Doing a second pass over which cache?
01:25bholleyheycam: right, and cousins (whatever's in the cache)
01:25bholleybz: the style sharing cache
01:25bzbholley: the style sharing cache?
01:25bholleybz: yes, see what I just wrote to heycam
01:25njnbz: question about this code: http://searchfox.org/mozilla-central/source/servo/components/style/stylesheets/mod.rs#93-135
01:25bzbholley: Yes, we could do that
01:25njnbz: for the Style case, I'm not doing anything to check for duplicates even though it's an Arc
01:26bzbholley: I don't have a good feel for how often we'd hit if we didn't hit before...
01:26njnis that ok? DMD is happy with it. i.e. is this the "primary" reference?
01:26bholleybz: wouldn't we get exactly the same characteristics as gecko?
01:26njnbz: and can I do likewise with all the other Arcs, for the other Rule kinds?
01:26bholleybz: which should be a 2x improvement in the STYLO_THREADS=1 case, per your numbers?
01:26bzbholley: I don't see why we would. For one thing, the style sharing cache is size-limited....
01:26bzbholley: Gecko's cache is also size-limited, but on a per-parent basis, not per-level basis
01:26heycamthe style context re-use in gecko is also limited
01:27bznjn: looking
01:27bholleybz: if _that_'s the problem, we can always try increasing the size of the cache
01:27bzbholley: Sure, though there's some perf cost to doing so
01:27bzbholley: anyway, this is certainly worth trying.
01:28bholleybz: right. But it's not a parameter we've tuned
01:28bzI tuned it some
01:28bholleyoh, right
01:28bzMaking it bigger is actually not super-trivial. ;)
01:28bzThere are comments about why.
01:28bholleyanyway, I think we should try the second pass thing
01:28bzIt's certainly worth trying
01:28bholleywho wants to write it up?
01:28bzIf nothing else to see what the numbers look like
01:29bznjn: Pretty sure this should be good
01:29njnbz: k, thx
01:29bznjn: In that the primary reference for a rule is the corresponding parent rule or inner
01:29bznjn: And there is always only one
01:30bholleyok, I'm going to head out for a bit. I think bz should continue with the measurements he proposed
01:30bzs/inner/sheet inner/
01:30bzyes, I am doing that right now
01:30bholleyheycam has various things on his plate, if he doesn't get to it today I'll do it tonight or tomorrow
01:30njnbz: even for the nested ones, like MediaRule?
01:30heycambholley: sounds good. I'll do the retriggering/averaging and my struct cache shrinkage, then look at the style sharing post-rule-node-calculation thing if I get time
01:31bholleysounds good - thanks all
01:31bznjn: yes
01:31njngreat
01:35bzheycam: so I just tried your struct cache on the html5 spec
01:35bzheycam: it's pretty good!
01:36* bz is not seeing heap-unclassified popping up to eat the savings
01:37heycambz: so the cache usage size should be measured, if you want to look at that
01:37heycamunder servo-style-sets/stylist/...
01:37heycamthat will be the size of the structs that hold the cached structs, plus any cached structs that aren't used anywhere else
01:38heycamoh, and one point against what bholley said before: after an initial styling, we *can* have cached structs that aren't used, because we cache before performing style adjustments
01:38bzheycam: yes, I see that
01:38bzheycam: I'm looking at the about:memory diffs
01:38heycamok
01:39bzSo for STYLO_THREADS=3
01:39bzhttps://pastebin.mozilla.org/9031657
01:39bzThat's no-cache vs with-cache
01:39bzYes, there's 0.5MB of struct-cache
01:39bzAlso -17.88MB servo-style-structs
01:40bzThe regression over Gecko was 23MB
01:40heycamok, that's reasonable
01:40bzin style structs
01:40bzSo this is getting back most of that
01:41* bz diffs against Gecko directly
01:41bzSo we have 8MB of structs instead of 3
01:41bz2MB of Font instead of 0.7
01:41heycammmm
01:41bz1.9 of Text instead of 0.6
01:41bzThose aren't cached, right?
01:41bzbecause not reset?
01:41heycamright
01:42bzAbout 2x as much Display as we used to have
01:42bzWay more Border
01:42bz0.79MB vs 0.04MB
01:42heycamthat's interesting
01:42bzThat's with your patch
01:42bzwithout your patch the numbers are clearly worse. ;)
01:42heycamok :)
01:43heycamin the current struct caching patch, as soon as we decide to cache one struct on a rule node, we allocate an object with 15 pointers on it (one for each style struct). so I'm wondering if the AWSY workload just doesn't reuse as many structs to make up for that
01:43bzah
01:43bzthat's possible
01:43bz15 pointers because 15 reset structs?
01:43heycamyep
01:43heycamI'm going to try just using a single list
01:44* bz pretty sure Gecko just has 15 pointers in all rulenodes
01:44bzwithout an out of band object
01:44heycamyeah, it does
01:44heycamand I bet we don't measure it
01:44bzsure we do
01:45heycamah, it's arena allocated
01:45bzyep
01:45xidornI heard that gecko caches inherit structs in rule node as well? that would help things like font and color supposedly
01:45bzMeasured automagically
01:45bzrule-nodes in about:memory
01:45heycamxidorn: it does, if all properties are set
01:45bzxidorn: if they're fully-specified, yes
01:45heycamxidorn: it used to be the case that the font shorthand would set all properties on the Font struct
01:45heycamxidorn: a long time ago
01:46heycamxidorn: but I don't think that's the case any more
01:46heycamxidorn: I could special case Color
01:46heycamthough sometimes I wonder if having a whole struct for color is a waste
01:47xidornheycam: we should probably move properties which are not set by font shorthand out from nsStyleFont if possible...
01:47heycamxidorn: we maybe should
01:47xidornheycam: we can store it inline in rule node directly :)
01:47xidornmaybe
01:47heycamxidorn: yes I wondered if we should :-)
01:47heycamwell, inline in the style context
01:48xidornyeah, probably
01:48xidornit's even smaller than a pointer...
01:48heycamright
01:48heycambz: how much Color struct do you see?
01:49bzheycam: in which test condition?
01:49heycambz: say STYLO_THREADS=3
01:49bzheycam: with/without your patch, stylo/nonstylo, how many threads?
01:49heycamstylo
01:49heycamdoesn't matter with or without patch
01:49bzheycam: 0.38MB
01:50bzheycam: 0.41 without your patch; probably noise due to sharing differences
01:50bzheycam: I mean ComputedValues sharing
01:50heycamok, so a bit but not as much as the other inherited structs
01:50bzFont and Text are the big ones
01:50bzAgain, this is the HTML5 spec
01:50bzI will have gmail data in a bit...
01:51* heycam should check the AWSY struct usage
01:52heycamhmm guess that's not so easy to aggregate without more scripts
02:30bzheycam, bholley: ok, I have both heycam-cache and emilio-share numbers for the html5 spec
02:30bzfor 1, 3, 6 threads
02:30bzAll the about:memories are in https://bugzilla.mozilla.org/show_bug.cgi?id=1397380
02:30firebotBug 1397380 NEW, nobody@mozilla.org stylo: Investigate sources of stylo memory overhead on the html single-page spec
02:30* bz working on getting gmail numbers now
02:37njnam I right to assume that memory usage is Stylo's biggest problem right now?
02:39bzIn terms of shipping, yes
02:39bzGod
02:39bzmemory reports on gmail are horrid
02:40bzbecause they have these randomly generated subframe urls. :(
02:40bzmmmhm
02:40bzI thought we had memory reporting for servo stylesheets
02:41bzAlso... loading gmail allocates 14MB of arraybuffers for hangouts
02:41bzwhy hangouts if I just want to read my mail?
02:46bzOK
02:46bzApparently we do NOT have memory reporting for servo stylesheets
02:46bzat least afaict
02:47bzhmm
02:47bzOr maybe we do?
02:47heycamI thought we did
02:47* bz pokes
02:47bzSo on gmail...
02:47bzI see 4.33MB style-sheets with servo
02:47bzMore like 9.3MB with Gecko
02:48bzalso, heap-unclassified goes from 17MB to 55MB
02:48* bz pulls out DMD
02:52bzheycam: more importantly, I was seeing sheets in unreported DMD output on the html5 page...
02:52heycambz: hrm
02:53heycambz: could it be sheets in the Loader's cache?
02:54heycambz: seems like we do look at those...
02:56bzI don't know
02:56bzAnyway, gmail has a huge heap-unclassified
02:56bzI'm going to DMD that and see what it tells me
03:06bznjn: Are you still there?
03:06bznjn: If I run dmd.py with two files, does it do a diff?
03:07bzAh, looks like it does
03:12bzyeah
03:13bzSo we're clearly failing to report a bunch of stylo sheet memory
03:14njnbz: hi, yes
03:14njnthe two files need to come from the same build, I think
03:14njnbz: I tried one the other day from two different builds and got trash
03:15njnbz: I'm just finishing off a patch that increases the style-sheets number, by measuring selectors
03:16bzRight
03:16bzI'm using the same build
03:17bznjn: So the context here is https://bugzilla.mozilla.org/show_bug.cgi?id=1392314#c16
03:17firebotBug 1392314 NEW, nobody@mozilla.org stylo: causes memory consumption to double for gmail
03:17bznjn: we're not reporting all sorts of sheet stuff in there
03:17njnbz: I can totally believe that
03:17bznjn: whether because we never reach the sheets to report them or for some other reason....
03:18bznjn: The headline number is that gmail has 38MB more heap-unclassified in stylo than in Gecko
03:18njnbz: I've generally been adding reporting code working from the top of the DMD reports and working down
03:18bznjn: makes sense
03:18njnbz: I haven't been trying to do a thorough job on every type
03:18njnbz: and I've also only been looking at wikipedia
03:18bznjn: btw, after working with it a lot these last two days, about:memory has so much awesome functionality. ;)
03:18njncool
03:18njndmd needs a tree-style view, within about:memory
03:18bznjn: and DMD is really quite easy to use, and the docs are awesome.
03:19bznjn: Thank you!
03:19njnyw
03:19bznjn: I mean, there's all sorts of stuff that would be nice-to-haves
03:19njnbz: https://twitter.com/nnethercote/status/905277268649496576
03:19njnI wrote that in response to seeing you going at this stuff yesterday :)
03:19bznjn: like "know about gmail generating random subframe urls that just differ in the random part and diff them against each other"
03:19njnthat's a hard one
03:19bznjn: Right
03:20bznjn: it's not clear how to have it auto-work without breaking other stuff
03:20njncachegrind's diff script can take regexes that you use to tweak filenames and function names
03:20bzAnyway, I've been pretty happy.
03:20njngood
03:20njngreat to see other people using these tools, esp. when they are domain experts, unlike me!
03:20bzMakes sense
03:21* bz had a pretty easy time just skimming dmd output for things that might matter
03:21bzI can see how that's easier when the function names mean something. ;)
03:21njnthe main problem with DMD is choosing the right -f value; that's where a tree display would improve things
03:21njnstart with the top of the stack, gradually drill down
03:22njnbz: I think my current patch will help with the PropertyDeclarationBlock::extend_common
03:23njncatches about 160KiB of that for wikipedia
03:25njnbz: BTW, "style-sheets" for some reason is not under "layout/" and I'm planning to change that
03:26bznjn: sounds good
03:26bz_sleepSo the good news is that html5 and gmail are different problems...
03:26bz_sleepOh, the other context is that gmail has _huge_ stylesheets
03:26njnthat sounds like bad news...
03:27bz_sleepThe Gecko ones are 8MB
03:27bz_sleepWe don't know what the stylo version is
03:27bz_sleepnjn: it's good news in that we knew that solving some of the html5 problems did not have much effect on awsy
03:27njnbz_sleep: what reporting do you want the most? what should I work on next?
03:27njnI could do the ElementData you mentioned
03:27bz_sleepnjn: Because while debugging/measuring/etc gmail may suck, it's less suck than awsy
03:28bz_sleepnjn: I think the stuff in https://bugzilla.mozilla.org/show_bug.cgi?id=1392314#c15 would be best
03:28firebotBug 1392314 NEW, nobody@mozilla.org stylo: causes memory consumption to double for gmail
03:28bz_sleepnjn: I mean, long-term we want the ElementData reporter
03:28njnok
03:29njnI'm happy to do all of it, just need to choose an order :)
03:29bz_sleepnjn: but for now we understand that it's not there, and figuring out how much ElementData we might have is easy: 32 * docuemnt.querySelectorAll("*").length or so
03:29bz_sleepnjn: I'd like to make progress on the gmail heap-unclassified
03:29njnok
03:29njnI should start doing gmail myself instead of wikipedia, then
03:30bz_sleepnjn: Would be awesome
03:30njnbz_sleep: are you running DMD on Linux?
03:30bz_sleepnjn: Also, it might be useful to have some sort of breakdown under "style-sheets", if it turns out that it's big
03:30njnok
03:30bz_sleepnjn: e.g. selectors, property storage, etc...
03:30bz_sleepnjn: so far, yes
03:31njndmd.py is slow, because fix_linux_stacks.py is slow, because addr2line is slow :(
03:31bz_sleepnjn: Mostly because I was doing various rebuilds and my Linux box builds 6-10x faster than my Mac now
03:31bz_sleepnjn: it hasn't been a problem for me so far, because I only needed to capture about 4 dmd logs
03:31njnok
03:31njnpost-processing each file takes ~20 minutes for me
03:31bz_sleepnjn: I can see how when doing iteration on memory reporters it would be a bigger problem
03:31bz_sleepoh, wow
03:32bz_sleepDefinitely did not take that long here
03:32njnI'm running with --stacks=full, not sure if that makes a big diffrence
03:32bz_sleepI was using --stacks=full
03:32bz_sleepOne thing I do have is that my file/lines are bogus
03:32bz_sleepI get things like....
03:33bz_sleepmozilla::detail::RunnableMethodImpl<mozilla::dom::HTMLStyleElement*, void (mozilla::dom::HTMLStyleElement::*)(), true, (mozilla::RunnableKind)0>::Run() (crtstuff.c:?)
03:33bz_sleepAnd LT$alloc..raw_vec..RawVec$LT$T$GT$$GT$::double::h0c1d6627f7c9a754 (style.cgu-0.rs:?)
03:33bz_sleepAnd style::properties::shorthands::background::parse_value::h6ec976353b4a94cd (/home/bzbarsky/mozilla/vanilla/obj-firefox-opt/dist/bin/libxul.so)
03:34njnI get the crtstuff too
03:34bz_sleep(for the rust code, that might also be a matter of whether the rust compiler is outputting the right debuginfo)
03:34bz_sleepok
03:34bz_sleepDunno
03:34bz_sleepIn my case it probably took several minutes
03:34bz_sleepbut not 20
03:34njnand style.cgu
03:34bz_sleepAnd it seems to cache the mappings?
03:34njnI don&#39;t get the RawVec$LT stuff, though
03:34bz_sleepIn that if I ran it on a different workload without rebuilding it was fast?
03:34njnthe first time you run dmd.py, it symbolizes, which is slow
03:34njnsubsequent runs on the same data file are *much* faster
03:35njn<alloc::raw_vec::RawVec<T>>::double (crtstuff.c:?)
03:36bz_sleephuh
03:36bz_sleepthat&#39;s much nicer!
03:36njnyou&#39;re not getting Rust demangling, for whatever reason
03:36bz_sleepRight
03:36bz_sleepWhich matches all my other tools, sadly... ;)
03:36njnValgrind can demangle Rust, if y ou have a new enough version
03:38bz_sleep3.13.0
03:38bz_sleepis what I have
03:39bz_sleepIt&#39;s the current version on Fedora 26...
03:39* bz_sleep checks, sees that is the current release valgrind
03:39bz_sleepOK
03:39bz_sleepI really should sleep
03:39bz_sleepit&#39;s almost midnight
03:39bz_sleepg&#39;night!
03:39njnbye
03:40njnheycam: thank you for the fast review!
03:40heycamnjn: np!
04:02heycamnjn: (oops, switched tabs) so just to confirm, Selectors are allocated in a single array, and we use a ThinArc to reference array, yes?
04:15globlooks like a backout of a servo change failed to be applied to github - https://hg.mozilla.org/integration/autoland/rev/ab26002b1029
04:16njnheycam: yes
04:17globhttps://bugzilla.mozilla.org/show_bug.cgi?id=1397594
04:17firebotBug 1397594 NEW, nobody@mozilla.org servo-backout-pr: backout failed - you need to resolve your current index first
04:19heycamglob: does the attempted merge leave some state around that hg revert doesn&#39;t clean
04:20heycamoh, that error is in the git tree
04:21globin generated code, looks like the ids changed
04:21globhttps://irccloud.mozilla.com/pastebin/wpA7q22o/
04:22globexcept the backout didn&#39;t touch that. hrm
04:30xidornheycam: text style is resolved on-demand rather than during parallel traversal, is that correct?
04:30glandiumheycam: where would I need to look at to insert markers when stylo starts and ends working on a document?
04:30heycamxidorn: yes it&#39;s not resolved during the parallel traversal
04:31heycam(I think it&#39;s in the post-traversal)
04:31heycamglandium: over multiple possible restyles of a document?
04:31xidornheycam: it seems to be called from StyleSet::ResolveStyleForText?
04:31heycamglandium: or, just to bracket each restyle/traversal it might do?
04:31glandiumheycam: the latter I guess
04:32heycamxidorn: that&#39;s right. so yes on demand during frame construction, and in ServoRestyleManager::TextPostTraversalState during restyle
04:33heycamglandium: Servo_TraverseSubtree is the main one
04:33heycamso its caller in ServoStyleSte.cpp
04:33heycamwhich is ServoStyleSet::StyleDocument
04:33heycamthere are various other FFI functions that are called in ServoStyleSet.cpp to resolve one off styles, are you interested in those too?
04:34globcan i get someone to manually create that backout PR with highest priority please?
04:34glandiumheycam: probably
04:34heycamglob: Servo_ResolveStyleLazily, Servo_ResolvePseudoStyle
04:35heycamglandium: (I think you don&#39;t need to worry aobut Servo_ResolveStyle, which just returns a ComputedValues object that has already been created; it shouldn&#39;t be doing any allocations)
04:35heycamglob: ok
04:36glandiumheycam: ok, thanks
04:38heycamglob: so https://hg.mozilla.org/integration/autoland/raw-rev/ab26002b1029 doesn&#39;t have any of those bindings changes in it
04:38heycamis that expected?
04:38heycam(i.e. the bindings stuff is unrelated?)
04:39globheycam: i _think_ they are unconnected
04:40globi&#39;m seeing a merge conflict on our git backout branch when merging from master, which is totally unexpected
04:41heycam(why I typed &quot;background&quot; instead of &quot;backout&quot; I&#39;m not sure)
04:45globso, i think what happened is the last backout we did also failed and needed to be manually fixed (failed due to merge conflicts in github)
04:46globand those fixes are in conflict with the state of our backout branch
04:46globbecause generated ids i guess
04:49heycamglob: when changes are brought over from servo to gecko, are patches applied, or do we just reset the entire state of the servo/ directory to what&#39;s in the servo repo?
04:50globheycam: each commit is applied (with some massaging of the commit desc, etc)
04:51heycamglob: in the case of a manually fixed up backout on the servo side, would it make sense to just reset the state of the servo/ directory?
04:51xidornwhy do we have two bindgens in tree?
04:51globheycam: that isn&#39;t really possible
04:52globheycam: but i have reset the state of our git backout branch which should address the merge conflict when updating to master
04:52heycamok
04:52heycamthanks
04:56glandiumheycam: should I be seeing ServoStyleSet::StyleDocument called 60 times in the content process alone when starting firefox, opening the wikipedia page for obama, and quitting?
04:57glob(bbiab)
04:57heycamglandium: doesn&#39;t sound exceptionally unreasonable. also svg images will use stylo, so that might account for some
04:57heycamglandium: although I haven&#39;t really looked to see how dynamic wikipedia is
04:58waffles*glob
04:58waffles(oops)
04:58heycamwell, &quot;*&quot; is a glob...
04:58waffles:D
05:02njnheycam: The command &quot;bash etc/ci/manifest_changed.sh&quot; exited with 1.&#39;
05:02njnhttps://travis-ci.org/servo/servo/jobs/272747101
05:02njnthat&#39;s a travis failure, any idea what it means?
05:03heycamnjn: I do not, but I usually ignore test results except for homu...
05:04njnheycam: will it let me land with that?
05:04njnI&#39;m updating the patch to add a comment, maybe it&#39;ll pass this time...
05:04heycamnjn: I *think* only homu needs to pass... is that right waffles?
05:07wafflesheycam, njn: yes, that&#39;s #18346 - ignore that
05:07crowbotIssue #18346: manifest_changed.sh returns 1 with empty output on travisci - https://github.com/servo/servo/issues/18346
05:21globheycam: good and bad news: https://github.com/servo/servo/pull/18402
05:21crowbotPR #18402: Backed out changeset 118a2b0b07c2 for assertion failures - https://github.com/servo/servo/pull/18402
05:21heycamglob: sh... should I cancel my PR?
05:22heycamhaha it was interrupted
05:22globheycam: dunno :) sorry; initially i thought the merge conflict was with the backout itself, not during setting up the backout branch
05:23globi guess one of those will fail; i don&#39;t think it really matters who wins that race
05:24heycamglob: I&#39;ll just close mine
05:30* glob leaves this here, just in case (mostly as a &quot;note to self&quot;) - http://mozilla-version-control-tools.readthedocs.io/en/latest/vcssync/servo.html#out-of-sequence-servo-commit
05:30njnheycam: do you understand this stack trace? https://pastebin.mozilla.org/9031666
05:30njnthe complicated Either<...> type is `ImageLayer`
05:31heycamnjn: while parsing a -moz-image-rect value
05:31heycaminside a background shorthand property
05:32njnheycam: any idea what data structure it ends up in?
05:32heycamnjn: the Background style struct
05:32heycam(though that&#39;s just because it&#39;s the value of background-image, as part of the background shorthand)
05:32heycam(-moz-image-rect values can be used in other image-y places)
05:33njnheycam: where is the Background style struct defined?
05:33heycamnjn: does that mean 6% of heap allocations are -moz-image-rect? or they could be any ImageLayer allocations
05:33heycamnjn: on the gecko side, nsStyleBackground
05:34njnheycam: some kind of Vec<ImageLayer> AFAICT
05:34heycamoh hang on
05:34heycamthis is a specified value parsing
05:34heycamso that&#39;s not stored in the style struct
05:34heycamthis is inside a PropertyDeclarationBlock somewhere
05:34heycame.g. a style rule
05:35heycamso you want to know about the Rust-side specified value storage for these things
05:35njnheycam: we measure PropertyDeclarationBlocks, but not the PropertyDeclarations within them
05:37heycamnjn: so ImageLayer is a values::generics::Image
05:37heycamin values/generics/image.rs
05:37heycamhow big is one of these allocations?
05:38njn1024 bytes, but it&#39;s a vec
05:39heycamnjn: maybe we should box up the Image part of the Either<None_, Image>
05:40glandiumheycam: where are stylo things free()ed? The last I exit ServoStyleSet::StyleDocument, there&#39;s plenty still allocated, but when I get to xpcom shutdown, most is freed
05:40njnheycam: maybe; depends how big Image is
05:41heycamglandium: most ComputedValues are freed in Element::UnbindFromTree
05:41heycamglandium: some will be freed by frames being destroyed, shortly before that
05:42glandiumheycam: thanks
05:42heycamglandium: style sheet stuff... not sure exactly but probably whenever the ServoStyleSet goes away
05:44glandiumoh, there&#39;s a timeline marker associated with the ServoStyleSet... maybe that&#39;s what I should look at
05:51heycamnjn: Image is 184 bytes
05:52njnheycam: ok, then boxing might be worthwhile if None is more common
05:52heycamnjn: although maybe instead we should box the uncommon enum variants of Image
05:52heycamnjn: None is definitely more common
05:52njnheycam: I still have no idea where this Vec ends up being stored
05:52heycamnjn: it should be in the PropertyDeclarationBlock
05:52heycamnjn: as the value of the background-image property
05:53njnah, ok
05:54heycamnjn: so how much memory in total is being taken up by these Vec<Either<None_, Image>> ?
05:54njnheycam: for gmail it&#39;s about 12 MiB
05:54heycamnjn: wow ok
05:54njnhighest unreported record from DMD
05:55heycamnjn: also, one image is far more common than >1
05:55heycamnjn: so we should maybe use a SmallVec
05:55njnok
05:56njnheycam: I can&#39;t find where background_image::SpecifiedValue is defined
05:56heycamnjn: do you want to file a bug blocking the gmail one and I&#39;ll write a patch?
05:56njnthese mako templates are complicated
05:56heycamnjn: yeah :(
05:56heycamnjn: it&#39;ll be in helpers.mako.rs
05:56njnheycam: a patch to change the representation?
05:56heycamnjn: yeah
05:56njnok
05:59sewardjheycam: ping
06:00heycamsewardj: hi
06:01njnheycam: bug 1397614
06:01firebothttps://bugzil.la/1397614 NEW, nobody@mozilla.org stylo: shrink background-images representation
06:01sewardjheycam: can you look at https://github.com/servo/servo/pull/18397 and tell me if I need to re-PR it?
06:01crowbotPR #18397: Add fallible append APIs for Vec and SmallVec - https://github.com/servo/servo/pull/18397
06:01sewardjI seem to have offended the style checker
06:02sewardjbut am unclear of its status now
06:02heycamglob: I can&#39;t see it, but is bug 1397613 spam?
06:02firebotBug https://bugzil.la/1397613 is not accessible
06:02globheycam: yup, will clean it up
06:02heycamthanks
06:02heycamsewardj: so, yes, you&#39;ll need to re-push with fixes for those style errors
06:03heycamsewardj: and then because Manishearth has delegated you, after you re-push, you can &quot;@bors-servo r=Manishearth&quot; yourself on the PR
06:04sewardjheycam: Do I just put that magic bit of text into one of the comment fields in the PR ?
06:05heycamsewardj: yep
06:06sewardjheycam: so, just to be clear .. if I make /home/sewardj/.cargo/bin/rustfmt run clean on the files I changes, will that satisfy the style checker?
06:07glandiumsewardj: I think clippy is run too
06:07heycamsewardj: I am not sure
06:07heycamsewardj: I always just run test-tidy and manually fix things up...
06:07sewardjAch
06:08sewardjwhat&#39;s test-tidy?
06:08heycamsewardj: you can &quot;./mach test-tidy&quot; from a servo tree
06:08Manishearthglandium: hm? we don&#39;t do clippy
06:08heycamand it should report those errors that will stop the merge from happening
06:08njnsewardj: https://public.etherpad-mozilla.org/p/stylo may help
06:08njnsewardj: are you doing a servo-only patch?
06:08sewardjnjn: thanks, and yes, servo-only
06:09njnsewarjd: look at the &quot;Make sure the following commands succeed&quot; bit
06:09njnbut don&#39;t look at anything else, you&#39;ll just confuse yourself O_o
06:09sewardjok!
06:09* sewardj tries
06:09glandiumManishearth: iirc I had a clippy error once on a PR
06:09njnsewardj: you probably don&#39;t need all five of those commands for a Servo-only, but it can&#39;t hurt
06:10glandiumbut maybe that was for something else
06:10njn&quot;The tree is currently closed for pull requests below priority 9000, this pull request will be tested once the tree is reopened&quot;
06:10njnboo
06:11njnsewardj: http://build.servo.org/homu/queue/servo shows the status of the landing queue
06:12njnit&#39;s not every day you get a compile error on line 104695
06:12glandiumO_o
06:13njnglandium: generated code, hardly even counts
06:13sewardjnjn: I foolishly loaded exactly that file into emacs yesterday. Then it took 80 cpu minutes to do the font colorisation :-/
06:14njnsewardj: vim&#39;s search has a distinct lag
06:14njnit&#39;s disconcerting, easy to think something hasn&#39;t been found because lag is so non-typical
06:15sewardjnjn: btw, do you know whether Tree Style Tab works again with the latest nightlies?
06:16njnsewardj: there has been some action, but I haven&#39;t tried it
06:16njnI&#39;m stil using release for now
06:19sewardjI have a nightly build from ~6 weeks ago, which works fine, but I feel like I should &quot;upgrade&quot;
06:19njnheycam: is SpecifiedValue always a Vec?
06:19heycamnjn: for vector-defined properties (like background-image) yes
06:19njnbut not for all properties? bummer
06:20heycamright
06:20heycamonly ones defined with vector=&quot;True&quot; in their mako invocation thing, or which use the helpers:vector_longhand mako thing to define them
06:20globheycam: i guess that spammer didn&#39;t believe me when i said &quot;If you continue to abuse bugzilla.mozilla.org your account will be disabled&quot;. active user count decreased by one
06:21heycamheh
06:25glandiumman, looking at what is rebuilt when touching ServoStyleSet.h is heartbreaking
06:25glandiumUnified_cpp_modules_libpref0.o
06:25glandiumUnified_cpp_protocol_http1.o
06:25glandiumand many other things
06:26glandiumand the style crate
06:26glandiumI&#39;m up for a long incremental build
06:30heycamnjn: do you know would the PresShell (and so style set) get measured before the DOM? or vice versa?
06:30njnheycam: le