mozilla :: #servo

8 Sep 2017
00:00bholleynjn: oh interesting, is that really 2 megs?
00:00bholley(or 2.8)
00:00bholleynjn: probably a data:URI?
00:00njnbholley: there's another record with 1.2 MB, too
00:01njnbholley: maybe, I'm investigating
00:01bholleynjn: duplicating a data:uri just to make it utf8 would be very silly :-)
00:01bholleynjn: or wait, I guess the other way around
00:01njnbholley: we're duplicating to make it utf16!
00:01bholleynjn: right, because we've already converted the stylesheet to utf8
00:02* bholley njn: (actually, we don't convert anymore - hsivonen fixed that in bug 1354989
00:02firebothttps://bugzil.la/1354989 FIXED, hsivonen@hsivonen.fi stylo: Dont go through UTF-16 for external stylesheets
00:03gwanyone around to r+ ^ ?
00:03mbrubecksure
00:04bholleyif those stylesheets are really embedding megabytes of data:URIs, I'm sure glad we're not round-tripping through utf8
00:07njnbholley: I added some printfs to Gecko_ImageValue_Create(): https://pastebin.mozilla.org/9031807
00:07njnbholley: there are some data: URIs, but that's not much of it
00:07njnbholley: just *lots* of normal-ish URIs, AFAICT
00:08njnbholley: with *lots* of repeats
00:08njnbholley: maybe a candidate for sharing somehow
00:09bholleynjn: file all this as a bug? Seems like a good candidate for jdm or Simon
00:09njnbholley: ok
00:15bholleynjn: also, question - this whole business about increased stylo memory reporting causing increases in image memory is weird to me - are these svg documents? CSS image values we weren't finding before?
00:16njnbholley: I would guess SVG docs, but I'm not sure
00:16bholleynjn: do we report the layout data of an svg document under the image category?
00:16njnbholley: I don't remember
00:17bholleynjn: is it something you could figure out easily? I just need to ack the data from jmaher, and want to make sure I understand it
00:17njnbholley: I can look if you think it's more important than investigating gmail heap-unclassified, which is what I'm currently doing
00:18bholleynjn: if this is something you can answer in 10 or 15 minutes, I think that would be a worthwhile interruption. If you think it's a bigger task, then perhaps not
00:18njnok
00:19njnbholley: ok, I filed bug 1397971 for the image URL stuff
00:19birtlesnox: ping?
00:19firebothttps://bugzil.la/1397971 NEW, nobody@mozilla.org stylo: lots of memory used for SpecifiedURLs relating to images for gmail
00:20noxbirtles: Pong but going to bed soon.
00:20bholleynjn: thanks
00:20njnbholley: where do those URLs live? is it hanging off PropertyDeclaration somewhere?
00:20birtlesnox: ok, I just wanted to ask you about bug 1397442 -- would you be able to look into it tomorrow?
00:20firebothttps://bugzil.la/1397442 NEW, nobody@mozilla.org stylo: crash under Servo_AnimationCompose
00:20birtlesnox: I'll pass on STR once I have them
00:20noxI can do that, but I wonder if the culprit is really any of that code.
00:21birtlesnox: yeah, it could be the calling code
00:22bholleynjn: yeah presumably, since this is part of the parsed representation
00:22njnbholley: the mako stuff makes memory reporting there tricky
00:22njnbholley: #18404 captures a lot of it -- the boxes and vectors -- but not the stuff within the boxes and vectors
00:22crowbotPR #18404: Measure PropertyDeclaration better. - https://github.com/servo/servo/pull/18404
00:22noxbirtles: Yep and I know nothing about the glue etc.
00:23njnbholley: there are a million SpecifiedValues types, and I have trouble even working out where they are defined :/
00:24bholleynjn: nox can probably help you with that
00:24bholleynox: he did a lot of derive() stuff to make that code easier
00:24Manishearthnjn: I use doc.servo.org
00:24Manishearthor, well, http://doc.servo.org/geckolib/
00:24noxThere are still a gazillion SpecifiedValue types because of the Python generated code.
00:25noxWhy don't we just use HeapSizeOf btw?
00:25Manishearthfor what?
00:25noxOh, different way to measure.
00:25Manishearthoh, yeah. no.
00:25Manishearthdifferent way to measure
00:25noxThat trait should probably be derived.
00:26njnManishearth: that's useful, but doesn't tell me where in the code these things are defined, and the mako stuff makes that difficult (for me)
00:26noxnjn: Why do you need to know where there are defined? To implement that trait?
00:26njnnox: https://github.com/servo/servo/pull/18404#issuecomment-327952624
00:26crowbotPR #18404: Measure PropertyDeclaration better. - https://github.com/servo/servo/pull/18404
00:26njnnox: more or less
00:27Manishearthnjn: oh, right
00:27noxI can help with the deriving, but for navigating .mako.rs files I think everyone just does it the hard way.
00:28noxnjn: Any properties::foo module comes from some kind of mako declaration in one of the templates.
00:29njnnox: I understand that much; what I don't understand is where to find the SpecifiedValue for BackgroundImage, for example
00:30noxnjn: ${helpers.predefined_type("background-image", "ImageLayer",
00:30noxnjn: means "use specified::ImageLayer and computed::ImageLayer"
00:30noxnjn: https://github.com/servo/servo/blob/master/components/style/values/specified/mod.rs#L41
00:31noxnjn: And given there is vector=True, that means SpecifiedValue is generated in some of the Python code, and is SpecifiedValue(Vec<ImageLayer>)
00:31njnnox: ok, thanks
00:31noxIt is not the most obvious setup for sure.
00:32noxnjn: https://github.com/servo/servo/blob/master/components/style/properties/helpers.mako.rs#L182
00:32noxI can help with the deriving, I would rather not land that trait without the associated derive macro.
00:42bholleyheycam: see bug 1397976 - seems to work well. I&#39;d be interested to see how it interacts with the reset caching
00:42firebothttps://bugzil.la/1397976 NEW, bobbyholley@gmail.com stylo: Do a second pass on the style sharing cache after computing the rule node
00:43heycambholley: that&#39;s great! I expect that to reduce some of the gains from reset struct caching, which were really making up for lower style sharing
00:44heycambut by how much I&#39;m not sure
00:50njnbholley: we do report SVG document data under images in about:memory. https://bugzilla.mozilla.org/show_bug.cgi?id=1395576#c11 has the explanation
00:50firebotBug 1395576 WONTFIX, nobody@mozilla.org 9.6 - 9.76% Images (linux64-stylo, macosx64-stylo) regression on push 6b9d06ba6f769234530ae67d83533
00:51bholleynjn: thanks!
01:01bholleyheycam: can you easily run some numbers on my patch with your data pipeline?
01:01* bholley isn&#39;t sure how much manual work it is now that you&#39;ve got it set up
01:02heycambholley: pushing and popping patches and pushing them to try for the differenct configurations, and then clicking on each Sy job in the results and copying and pasting one link, is the manual work involved. plus running about 10 command line things to generate the results, though they&#39;re in my history so I don&#39;t have to work them out again :)
01:02heycambholley: but yes I can!
01:03bholleyheycam: ok. I think the two sets of numbers we want are 2ndpass on its own, without reset sharing, and 2ndpass with some form of reset sharing (probably doesn&#39;t matter too much which)
01:04heycambholley: ok
01:05bholleyheycam: the nice thing about 2ndpass is that it helps us with style contexts as well as style structs
01:05heycambholley: yeah indeed, I think that&#39;s important
01:05heycambholley: style contexts aren&#39;t small
01:06heycambholley: do you care about number of threads?
01:06heycamprobably yes
01:06bholleyheycam: ideally both 1 and 4, like the other numbers
01:06bholleyheycam: yeaj
01:06heycamok
01:06bholley*yeah
01:06bholleyheycam: (you need the two patches, including the rename, for it to compile btw. obviously not the printf patch)
01:07heycamk
01:10glandiumbholley, heycam: do we have large bin-unused differences between gecko and stylo on mac and windows?
01:10heycamglandium: I don&#39;t know so far I&#39;ve only been testing on linux
01:11heycamglandium: if you grab the TabsOpenForceGC-2 memory report from some recent m-c AWSY runs you could compare
01:16bholleyheycam: (if you haven&#39;t pushed yet, try removing the is_native_anonymous check)
01:16bholleyheycam: I didn&#39;t realize how much sharing we&#39;d lose when I added it
01:17heycambholley: not yet just building locally first
01:17bholleyheycam: oh wait
01:18bholleyheycam: crap
01:18bholleyheycam: I also realized that I didn&#39;t do the parent check
01:18bholleyheycam: I forgot that we put multiple things in the cache
01:18bholleyheycam: ok. I probably need to bake this patch a bit more before we measure
01:18bholleyheycam: and I need to run right now
01:18bholleyheycam: so feel free to pause
01:21heycambholley: ok
01:56njnheycam: any chance you can review #18404? make a big difference on gmail
01:56crowbotPR #18404: Measure PropertyDeclaration better. - https://github.com/servo/servo/pull/18404
01:57heycamnjn: sure. I&#39;ll look at this after these other couple of reviews I&#39;ve got.
01:57njnthx
02:16heycamwhat version of rust are we currently targetting in stylo?
02:20heycamnm
02:41heycambirtles: sorry I haven&#39;t yet extracted the helpful part of my patch for you. it&#39;s still not clear whether my patch will land or not.
02:41heycambirtles: maybe I should land it anyway even if my patch won&#39;t land
02:41heycam(land the extracted bit)
03:02birtlesheycam: no problem -- I&#39;m actually away next week, so I guess either someone else will need to fix that SMIL bug or we should just ship with that regression
03:29dmhnot much beginner stuff atm other than saltstack? I&#39;ve never seen it so lopsided before
03:29dmher, PRs
03:29dmher issues :/
03:37* heycam finally has a script that downloads all memory reports from all AWSY re-triggers given a single treeherder rev link, saving him much clicking
03:49Jerry_IRCCloudgw: ping. the conclusion of https://github.com/servo/webrender/pull/1668#issuecomment-327931658 is from https://github.com/servo/webrender/issues/1646
03:49crowbotIssue #1646: Handle the zero length of data for external raw data. - https://github.com/servo/webrender/issues/1646
03:49crowbotPR #1668: Handle the failed case of ext-image callback. - https://github.com/servo/webrender/pull/1668
03:50Jerry_IRCCloudgw: what do you think for the zero length data problem?
03:52heycamnjn: when you file a PR, please put the &quot;r? @someone&quot; in a separate comment. otherwise the reviewer gets pinged when the servo-vcs-sync generated commit (which includes the whole first comment) gets merged around to different repos
03:52njnheycam: ok
03:52heycamthanks :)
03:55Mateon1Hm, so may I ask, what is saltstack? I haven&#39;t heard of it before apart from the open good-first-issues
03:55Mateon1What&#39;s its function?
04:03gwJerry_IRCCloud: having the dummy PBO seems good to me. I was just going to give kvark a final chance to reply, but this should be good to merge, I think
04:04gwJerry_IRCCloud: I&#39;ll merge it now - kvark can add a comment for any follow ups if necessary :)
04:05bzHow big is a Vec?
04:05bzAn empty one
04:05bzIs it just one word, or more?
04:06sp3dthree words
04:06bzLength, capacity, buffer pointer?
04:06sp3dptr,cap,len
04:06sp3didk order
04:07njnwhat does a line like this mean? #[cfg_attr(feature = &quot;servo&quot;, derive(HeapSizeOf))]
04:07njnis |feature = &quot;servo&quot;| ever not true within Servo?
04:07bzsp3d: right, good. Don&#39;t care about order. ;)
04:08bznjn: feature = &quot;servo&quot; means &quot;not stylo&quot;
04:08bznjn: stylo is feature=&quot;gecko&quot;
04:08gwplaybot: std::mem::size_of::<Vec<i32>>()
04:08gwbz: ^
04:08njnbz: ok, thanks
04:08bzgw: perfect, thanks!
04:08* bz was about to ask that question
04:09* bz does some std::mem::size_of logging
04:11Mateon1Is it a good idea for a beginner to Servo code to look for &quot;TODO:&quot; &quot;XXX:&quot; &quot;NOTE:&quot; &quot;HACK:&quot; &quot;FIXME:&quot; comments and possibly try to fix them?
04:11Mateon1A quick grep yields a lot of results
04:13gwMateon1: at least in WR, I tend to leave TODO comments when it&#39;s a bit speculative what is involved, or a large amount of refactoring is involved... but there&#39;s probably a heap of small-ish ones in Servo itself...
04:32bzOption<RefPtr<ImageValue>> is two words? :(
04:34njn&quot;error: the lock file needs to be updated but --locked was passed to prevent this&quot;
04:35bzBoy
04:35bzusing nested enums is sure a nice way to use tons of memory. :(
04:35njnbz: yeah
04:35njnbz: xidorn wants auto-flattening
04:36njnManishearth: how do I deal with the lock file error above? (four lines up)
04:36* njn tried editing Cargo.lock by hand
04:37Mateon1bz: Isn&#39;t Option<> completely removed when containing non-zero pointers?
04:37bzpointers, yes
04:37bzBut it doesn&#39;t realize nsRefPtr is a pointer
04:37bzer, RefPtr
04:38bzEither that, or Arc is more than one word
04:38heycamisn&#39;t there some way to declare that RefPtr will be zero
04:38heycamor am I imagining
04:38xidornI think there is
04:38xidornbut I&#39;m not sure :P
04:38bzhttps://pastebin.mozilla.org/9031822 -- that struct is 32 bytes according to std:mem::size_of
04:39heycamnot stable yet: https://doc.rust-lang.org/core/nonzero/struct.NonZero.html
04:39heycamwhich is why servo_arc/lib.rs has NonZeroPtrMut...
04:39bzmmm
04:39xidornnjn: enum flattening wouldn&#39;t fix the issue of Option<RefPtr<_>>
04:39heycamcan we make RefPtr use NonZeroPtrMut?
04:39bzSeems like a good idea
04:39bzSo next question
04:39njnbz: Arc is a word: https://docs.google.com/presentation/d/1q-c7UAyrUlM-eZyTo1pd8SZ0qwA_wYxmPZVOQkoDmH4/edit#slide=id.p
04:40bznjn: right
04:40bznjn: more to the point, the thing called &quot;Arc&quot; in stylo is a word
04:40bz(it&#39;s not a normal Arc, it&#39;s http://searchfox.org/mozilla-central/rev/44c693914255638d74bcf1ec3b0bcd448dfab2fd/servo/components/servo_arc/lib.rs#635-637
04:40xidornbut Vec is three words in Rust :/
04:41bzSo now my real question...
04:41bzxidorm: sure, but there&#39;s no Vec in there, right?
04:41xidornbz: there are Vecs everywhere
04:41bzSure
04:41bzI mean, unless the Vec is empty...
04:41xidornbz: we should probably convert all Vec to Box<[_]>...
04:41bzThe length/capacity have to live somewhere
04:41bzIf empty vecs are common, then I agree it sucks
04:42* heycam ducks out to get some lunch
04:42Mateon1Box<[T]> is tricky to work with
04:42bzIt&#39;d have to be Option<Box<>> or something
04:42bzOh, I see, Box of slice
04:42bzmmmhm
04:42bzOK, so a more realistic question
04:42xidornthat&#39;s only two words
04:42bzif I have an empty vec
04:42bzand I push into it?
04:42bzHow much will it allocate?
04:43xidornplaybot: vec![1].capacity()
04:43bzmmhm
04:43* bz is having a hard time reconciling that with what dmd claims
04:43xidornplaybot: { let v = Vec::new(); v.push(1); v.capacity() }
04:44xidornplaybot: { let mut v = Vec::new(); v.push(1); v.capacity() }
04:44xidornbz: so 4 ^
04:44bzAh
04:44bzwell _that_ explains it
04:44bzSo horrible
04:44Mateon1Is vec![] not equivalent to Vec::new() + pushing?
04:44xidornplaybot: { let mut v = Vec::new(); v.push([0u8; 1024]); v.capacity() }
04:45bzThis is how we get a 1KB allocation for every background-image value.
04:45xidornMateon1: no, it&#39;s not. vec![] has a different impl
04:45Mateon1xidorn: In that case you want vec.extend, I&#39;m pretty sure
04:45bzThis code is like designed to suck at memory usage
04:46Mateon1playbot: {let mut v = Vec::new(); v.extend([0u8; 1024]); v.capacity() }
04:46xidornbz: I wonder whether it works if we shrink it
04:46Mateon1playbot: {let mut v = Vec::new(); let arr = [0u8; 1024]; v.extend(&arr[..]); v.capacity() }
04:47xidornplaybot: { let mut v = Vec::new(); v.push(1); v.shrink_to_fit(); v.capacity() }
04:47Mateon1Right..
04:47xidornplaybot: { let mut v = Vec::new(); v.push(1); let ptr1 = v.as_ptr(); v.shrink_to_fit(); (ptr1, v.as_ptr()) }
04:48xidornbz: yeah it works, if we shrink it we get a different memory area ^
04:48njnbz: want to break your &quot;no reviews&quot; policy for a one-liner in bug 1397472 ?
04:48firebothttps://bugzil.la/1397472 NEW, n.nethercote@gmail.com stylo: Add a memory reporter for ElementData
04:48njnbz: since you filed that bug :)
04:49bzit&#39;s not a no reviews policy
04:49bzit&#39;s a &quot;I am way behind, can&#39;t promise speedy reviews&quot;
04:49bzBut I can do this one
04:49bzjust needinfo me?
04:50njno
04:50njnk
04:50njnbz: you have &quot;no reviews&quot; set in bugzilla, apparently
04:51bzRight
04:51bzBecause otherwise I would be swamped with them too much
04:51bzright now
04:51* bz hopes to unflag that early next week
04:57bznjn: so what bucket will this patch put the servodata number in?
04:57bznjn: in terms of about:memory?
04:58njnbz: dom/element-nodes, I htink
04:58bzhmm
04:59njnbz: doesn&#39;t have to go there if you don&#39;t want
04:59bzbasically wondering whether we can report it separately, like slots
04:59bzso we can track how much it&#39;s using...
05:00njnbz: is a separate bucket worthwhile just for the ElementData structs? it was only 100-somethig KiB for gmail
05:01bznjn: it&#39;s 4MB on html5 spec...
05:01bznjn: But yes, hard to say
05:01njnhuh
05:01bznjn: html5 spec has a lot more elements. ;)
05:01njnbz: what path would you like?
05:02njnlayout/element-data-structs ?
05:02bzSo bug 1397614 is like a cautinary tale of how to get 1KB of memory used instead of 24 bytes
05:02firebothttps://bugzil.la/1397614 NEW, xidorn+moz@upsuper.org stylo: shrink background-images representation
05:02bznjn: That sounds lovely
05:02njnok
05:03njnbz: ouch (that bug)
05:04njnat least it should be easy to improve
05:06bznjn: Note that the computed values are presumably super-big too....
05:10bzI guess shrink_to_fit() exists
05:11bzbut of course that incurs extra maloc traffic..
05:11xidornyes
05:11bzaha
05:11xidornso probably we should use smallvec
05:11bzSo what we could do is reserve(length + 1)
05:11bzAnd then push
05:12bzplaybot: { let mut v = Vec::new(); v.reserve(v.len() + 1); v.push(1); let ptr1 = v.as_ptr();
05:12xidornisn&#39;t that O(n^2) then...
05:12bzYes
05:12bzin the number of background images
05:12bzWhich is ... how many?
05:13* bz goes to see what Gecko does
05:13bzOh, Gecko uses a linked list anyway
05:13xidornthat may not be a lot... but you are still doing lots of allocation and memmove
05:14xidornwhich doesn&#39;t sound fun...
05:14bzBut even if it did use an array, it would be an array of nsCSSValue
05:14xidornyeah, gecko use linked list for lists
05:14bzwhich are two words
05:14bzAs in, everything is boxed all the time, basically
05:14bzSo servo is a bit less allocation-happy here, that&#39;s good
05:16bzBut it needs 192 bytes to store what is fundamentally 3 words of information...
05:17bz(not to mention the 3-word Vec thing for when there is no background image...)
05:17bzSo much bloat. :(
05:17xidornbackground-image cannot be an empty vec
05:17bzwhy not?
05:18xidornbecause none is a value of <bg-image>
05:18bzyeees....
05:18xidornso its default value is vec![none]
05:18xidornnot vec![]
05:18bzIn the _specified_ case?
05:18xidornI mean, initial value
05:18xidornyes
05:18bzIn the computed case, I agree
05:18bzBut in the specified case the default thing is &quot;not specified&quot;
05:18bzHowever that&#39;s represented
05:19xidornin the specified case, the default is not present, right?
05:19bzsure
05:19xidornand in that case, the problem is just the size of PropertyDeclaration
05:20xidornand if its smallvec is large enough, we would need to box it...
05:20bzmmhm
05:21xidornhmmm, background-image is already a vec, so we should have already been using SmallVec for it...
05:25bzUsing SmallVec makes sense to me
05:25xidornoh we always use Vec for specified value of vec properties :/
05:26bzAt least then the common &quot;only one background image&quot; case doesn&#39;t pay the extra cost
05:26bzThis could be changed... ;)
05:26xidornbz: and people don&#39;t usually want to re-emphasize the initial value
05:27xidornbz: I believe the issue for using SmallVec in specified value is the size check of PropertyDeclaration
05:27xidornwe may need to actually do Box<SmallVec<_>>
05:28bzmmmmm
05:29xidornthe threshold is 24 bytes, which is the size of Vec
05:29xidornso SmallVec would never fit
05:29xidornso we always need Box<SmallVec<_>>
05:29xidornwhich is probably fine I guess
05:31bholleysewardj_: I was generally thinking that we should propagate OOM up to the caller that&#39;s either (a) trying to parse the stylesheet or (b) rebuild the stylist
05:31bholleybz: how high up do you think we should propagate OOM?
05:31bholleybz: I was suggesting that if we OOM once, we probably shouldn&#39;t continue parsing
05:31bzbholley: I agree
05:32bzbholley: fwiw, I&#39;d suspect Gecko has fatal OOM while parsing sheets....
05:32bholleybz: does it store the rules in a vec?
05:33* bz thought so
05:33bzIt calls http://searchfox.org/mozilla-central/source/layout/style/CSSStyleSheet.cpp#523
05:33bzOh
05:33bzThat&#39;s a fallible append
05:34bzAnd it ignores failure
05:34bzhmm
05:34bzis that a fallible append?
05:35bzI don&#39;t think it is anymore
05:35bzThought it&#39;s hard to tell with all the templates....
05:35Mateon1bz: On one hand, that doesn&#39;t sound like a good idea, but on the other, it sounds somewhat reasonable
05:35bzOnce you&#39;re accepting user input, all ideas are bad
05:35bzbasically
05:35Mateon1The first being, parse it correctly, or fail, the other being, display something to the user, even if it&#39;s ugly/incorrect
05:36bholleybz: do you think it would still be prudent to do this fallibly?
05:36bholleybz: or is it not worth it?
05:45bzbholley: I don&#39;t know offhand...
05:45bzbholley: If it&#39;s easy, might as well, I guess
05:46* bz really needs to sleep
05:46bzbholley: so I expect fixing this background image stuff will help a lot with gmail...
05:46bzbholley: in general, gmail seems to be the &quot;some nodes, huge numbers of rules&quot; testcase
05:46bzbholley: while html5 is the &quot;some rules, huge number of nodes&quot; testcase
05:47bzbholley: hence near-0 overlap in where they use memory... ;)
05:47bholleybz: that&#39;s good
05:47bholleybz: you focus on that, cam and I will focus on the sharing
05:47bholleybz: I&#39;m hoping I can improve my 2ndpass stuff
05:47bzbholley: xidorn is working on the background image thing
05:47bholleybz: a bit disappointing that the win got cut in half, but hopefully I can get some of it back
05:47bzbholley: I will see what else I can find....
05:48bzbholley: in the dmd stuff
05:48bzbut tomorrow
05:49* bholley too
05:49bholleygnite
05:59Jerry_IRCCloudgw: the image size calculation is not correct. so, I push another patch for https://github.com/servo/webrender/pull/1668
05:59crowbotPR #1668: Handle the failed case of ext-image callback. - https://github.com/servo/webrender/pull/1668
06:00Jerry_IRCCloudgw: how to tell bors to skip the merge?
06:04gwJerry_IRCCloud: if you force push into that branch, bors will not merge it
06:05gwJerry_IRCCloud: looks like that typo is fixed now, and it&#39;s ready to merge?
06:08bholleyheycam: if the try push for bug 1397976 comes back mostly green (modulo some visited stuff), it&#39;s probably worth doing the memory analysis we talked about. I&#39;ll try to optimize it more tomorrow, but would be good to have the numbers if the patch looks correct-ish and you have cycles
06:08firebothttps://bugzil.la/1397976 NEW, bobbyholley@gmail.com stylo: Do a second pass on the style sharing cache after computing the rule node
06:09heycambholley: righot
06:09heycam*righto
06:09Jerry_IRCCloudgw: yes.
06:09bholleythanks!
06:09heycambholley: fyi I plan to be offline for most of the weekend
06:09bholleyheycam: oh right, today is your friday?
06:09gwJerry_IRCCloud: ok, r+ed!
06:09heycambholley: yeah
06:09bholleyheycam: in that case definitely measure :-)
06:09heycamok :)
06:10bholleyheycam: (I think the patch is correct enough, at least to a first approximation)
06:10heycamI&#39;ll keep an eye on the try run
06:10bholleythanks
06:10* bholley -> bed
06:37emiliotruber: ping?
06:42njnemilio: how do I start using gecko_bindings in a new crate?
06:42emilionjn: presumably make that crate depend on style? (But I suspect that&#39;s not what you want :))
06:42xidornplaybot: vec![][1]
06:42xidornplaybot: (vec![])[1]
06:43xidornplaybot: (vec![0u8])[1]
06:43njnemilio: not really :) I&#39;m trying to move the MallocSizeOf stuff into its own crate
06:43emilionjn: moving them out of style would work, but it&#39;s (a bit of) work
06:43njnemilio: moving gecko_bindings out of style, you mean?
06:43emilionjn: yes
06:44njnemilio: hmm, it&#39;s just a single type I need :/
06:44emilionjn: which one?
06:44njnSeenPtrs
06:44njnit&#39;s a hash table passed from C++
06:45njnqueried via Gecko_HaveSeenPtr()
06:46emilionjn: is it just an opaque type? If so, you may as well just manually define it as `enum SeenPtrs(c_void)` or something like that
06:46emilionjn: err, `struct SeenPtrs`
06:47njnemilio: hmm, that could work. Would it be an empty struct?
06:47emilionjn: yes, but I&#39;m assuming you&#39;re passing around a pointer, right? (like `*mut SeenPtrs` or something like that)
06:47njnI am
06:48emilionjn: then that works
06:48njnwhat works?
06:49emilionjn: just using an empty struct that is non-copiable, wrapping `c_void`
06:50njnemilio: can you type out the exact type? I&#39;m having trouble imagining it
06:52emilionjn: `struct SeenPtrs(::std::os::raw::c_void)`, or `struct SeenPtrs([u8; 0])`, I don&#39;t remember the exact thing bindgen generated for those, but it&#39;s something like that.
06:52njnemilio: ok, thanks
06:52emilionjn: `enum SeenPtrs {}` may be better, since you can&#39;t dereference it, but I don&#39;t remember why we didn&#39;t use it...
07:30noxcrowbot: tell jdm I know how to avoid the double boxing for cancellable promises.
07:30crowbotyou got it!
08:08noxcrowbot: tell KiChjang I may end up reverting most of what he did due to https://github.com/servo/servo/pull/9217#issuecomment-170278822
08:08crowbot*sigh*
08:08crowbotPR #9217: Redesign ScriptMsg to be more specific to DOMManipulationTaskSource - https://github.com/servo/servo/pull/9217
08:24noxemilio: r? ^
08:24noxhiro++ thanks for taking the crash ticket.
08:28emilionox: which crash ticket? (Just curious)
08:28emilionox: looking, too
08:28noxemilio: Bug 1397442.
08:28firebothttps://bugzil.la/1397442 ASSIGNED, hikezoe@mozilla.com stylo: crash under Servo_AnimationCompose
08:30emiliohiro: nox: I&#39;m moderately sure it&#39;s a stack overflow that should be already fixed
08:30noxemilio: Nice.
08:36noxbz_sleep, xidorn: Re: Vec<T> is 3 words: just use Box<[T]>.
08:37noxMateon1: Box<[T]> isn&#39;t tricky to work with.
08:40noxxidorn, bz_sleep: What about just making ImageLayer be Either<None_, Box<Image>>?
08:47xidornnox: majority of background-image declarations are expected to take the image variant, so I expect that way to be even worse
08:47noxxidorn: Mmmh
08:47noxxidorn: aren&#39;t most just a single none?
08:48xidornnox: if you want none, why do you bother specifying it at all?
08:48noxEither<None_, Box<Image>> should be a single word, it&#39;s a glorified Option<Box<Image>>.
08:48noxxidorn: I don&#39;t understand your question. Is the issue of ImageLayer being large only for specified values?
08:49xidornnox: for stylo yes
08:49noxOh, ok.
08:50xidornnox: because we don&#39;t persist servo&#39;s computed value
08:50noxAh.
08:50xidornand for gmail&#39;s case yes as well
08:51xidornbecause that&#39;s a case that there are tons of rules but relatively fewer elements
08:53noxAck.
08:59wcpanI guess it&#39;s relatively dangerous to check the dirty bits on element which has animation :(
09:08emilioxidorn: nox: most of them could be `None` due to the background shorthand expansion
09:08noxemilio: Good point.
09:08emilioxidorn: nox: background: #ccc will generate a `background-image: none`
09:08emiliowcpan: what&#39;s up?
09:09emiliowcpan: (with the dirty bits I mean)
09:09emiliowcpan: why would it be dangerous?
09:10wcpanemilio: because we will clear mStyleContext in most of time, so I want to look the style context via mContent->GetPrimaryFrame()->StyleContext()
09:11emiliowcpan: don&#39;t we do that already?
09:11emiliowcpan: when we can at least?
09:13wcpanemilio: in my current patch, I just check if there is a mStyleContext already, but this only works for few cases
09:14emiliowcpan: that kind of defeats the point of the patch, right?
09:15emiliowcpan: we should be able to optimize it out even when there&#39;s no cached `mStyleContext`. Otherwise the optimization is quite useless, because people don&#39;t usually reuse the `nsComputedDOMStyle` object
09:15wcpanemilio: ya, so now I&#39;m trying to get the style context from mContent
09:15emiliowcpan: in stylo you can quite easily (StyleSet()->ResolveServoStyle(mContent->AsElement())), when the element is displayed
09:16wcpanemilio: wouldn&#39;t it do something like recascading?
09:16emiliowcpan: nope
09:17emiliowcpan: In gecko it would, not in stylo
09:18xidornemilio, nox: yes but that happens less than tenth of times than an actual background-image per https://bugzilla.mozilla.org/show_bug.cgi?id=1397614#c3
09:18firebotBug 1397614 NEW, xidorn+moz@upsuper.org stylo: shrink background-images representation
09:18wcpanemilio: and if the element is not displayed, it will return default style?
09:19emiliowcpan: it will crash, you can use `ResolveStyleLazily` for that, and it&#39;ll actually compute the style
09:19emilioxidorn: oh, ok, I was just pointing out without context :)
09:20wcpanemilio: do we need to treat pseudo elements differently for that too?
09:21emiliowcpan: maybe? If the pseudo-element exists, no, otherwise, yes, but ResolveStyleLazily takes care of that... That&#39;s what we use to look `mStyleContext` when there&#39;s no frame, basically
09:22emiliowcpan: in any case, I&#39;m not sure how this is related to animations
09:25wcpanemilio: I found some test cases failed in layout/style/test/test_transitions_per_property.html, the restyle root is nulltpr, but it still got wrong value
09:27emiliowcpan: note that that test may just be relying on the flush, given it tweaks the timer that the refresh driver uses
09:44paulmbrubeck: I&#39;m trying to reproduce the failures found in #18410 but I&#39;m getting different results. Should I just blindly change the results of these 3 tests?
09:44crowbotPR #18410: harfbuzz-sys update - https://github.com/servo/servo/pull/18410
10:08noxemilio: Forgot my review? :p
10:18SimonSapinnox: r? https://github.com/servo/servo/pull/18420
10:18crowbotPR #18420: Get rustc commit hash from channel manifest - https://github.com/servo/servo/pull/18420
10:57emilionox: whoops
10:58noxemilio: No probs. :)
11:54travis-ciServo failed to build with Rust nightly: https://travis-ci.org/servo/servo-with-rust-nightly/builds/273254388 CC nox, SimonSapin, jntrnr
11:59wcpanemilio: so I did some real transition tests (i.e. no advanceTimeAndRefresh), seems like during the transition, the restyle root wont be set, it only updates at start & end (which I think it makes sense)
12:06wcpanemilio: so I think I need to check has_non_animation_invalidations || for_animations for ancestors
12:09wcpanuh, I mean, also check those animation bits
12:11Mateon1Now I can&#39;t build Servo at all, because components/script crashes rustc. For release mode it&#39;s oom, for debug mode it&#39;s ??? (no output)
12:12Mateon1I have no idea how I was able to do so a week ago or so
12:13* wcpan heads off
12:50bakuwhich component should I use to file a bug in stylo?
12:53noxbaku: Core :: CSS Parsing & Whatever I Don&#39;t Remember Exactly
12:53noxbaku: With a &quot;Stylo:&quot; prefix.
12:55bakunox: done bug 1398132.
12:55firebotBug https://bugzil.la/1398132 is not accessible
12:55bakunox: hopefully it&#39;s dup.
12:55nox&quot;You are not authorized to access bug 1398132.&quot; I&#39;m happy with that.
12:59Mateon1Ah, I figured out my build issues
12:59Mateon1I disabled memory overcommit on my machine, as the OOM killer was killing not what I wanted
13:00Mateon1When re-enabled, servo compiles fine
13:14emiliobaku: can you cc me?
13:35noxcrowbot: Tell jdm that https://github.com/servo/servo/commit/0c33d6168e1eda13ee260ee71fa03365ab2cad52 confirms that there are at least no misused runnables.
13:35crowbotyou bet!
13:41noxSimonSapin: Any type that is in a Result or Option on which we call unwrap or expect needs Debug, right?
13:41noxSo I don&#39;t see how we could have Debug impls only in debug mode.
13:49SimonSapinnox: ugh, good point
13:50noxSimonSapin: We could also not use expect and unwrap.
13:50SimonSapinnox: we could have dummy implement, though
13:50noxAnd have a style-specific panic method that is #[cold] and #[inline(never)].
13:50noxs/method/function.
13:50noxErr, you got me.
13:53noxSimonSapin: We can also use #[css(derive_debug)]
13:59SimonSapinnox: whats that?
14:01noxSimonSapin: One of my things.
14:01noxSimonSapin: #[derive(ToCss)] #[css(derive_debug)] derives ToCss and uses it to implement Debug.
14:02noxSimonSapin: I even documented it. :) https://github.com/servo/servo/blob/tasks/components/style_traits/values.rs#L26:L27
14:03SimonSapinnice
14:04SimonSapinnox: yes, for large types it would help to have one large impl and one trivial impls that calls the first one, rather than two large impls
14:05noxSimonSapin: Also, https://github.com/servo/servo/pull/18415/files#r137796578
14:05noxThat doesn&#39;t help for AnimationValue or PropertyDeclaration though.
14:07SimonSapinnox: replied. I think its mostly string literal dedup
14:13SimonSapinbholley: would could mean &quot;assertion failed: distance_to_stack_bottom <= max_allowable_distance&quot; when upgrading the compiler? https://github.com/servo/servo/pull/18420#issuecomment-328114119
14:13crowbotPR #18420: Get rustc commit hash from channel manifest - https://github.com/servo/servo/pull/18420
14:16sewardjSimonSapin: ping
14:16SimonSapinhey sewardj
14:16sewardjSimonSapin: so .. we just did increase the stack size
14:16sewardj(at least, I r+&#39;d it. I don&#39;t know if it landed yet)
14:17SimonSapinsewardj: #18412? Looks like it landed before these failures occured
14:17crowbotPR #18412: Increase stack safety margin for stylo - https://github.com/servo/servo/pull/18412
14:18sewardjSimonSapin: yeah, that&#39;s what I meant
14:19sewardjSimonSapin: and you&#39;re sure that&#39;s in the build in question?
14:19SimonSapinsewardj: well, looks like that assertion is still failing on CI sometimes
14:20SimonSapinlet me check
14:22SimonSapinsewardj: yes, that commit is an ancestor of the merge that just failed to land
14:24sewardjSimonSapin: hmm, I wonder if those assertions are correct even in the case that the stacks are placed back-to-back, with no space in between
14:25SimonSapinsewardj: I barely guess what this assertion is for, I mostly see CI failing for seemingly unrelated reasons :]
14:47truberemilio: hey
14:48emiliotruber: hey, just wanted to ping you about bug 1397091 in case you had any testcase with a similar stack (with CharacterDataChanged)
14:48firebothttps://bugzil.la/1397091 NEW, emilio@crisal.io stylo: panic in Servo_ResolveStyle on betcity.ru
14:48emiliotruber: but I managed to construct one, so no big deal :)
15:38bzSo do we have bugs tracking the remaining failing stylo reftests?
15:38* bz doesn&#39;t see bug numbers attached to https://gist.github.com/Manishearth/086118c940ff86a6cfc573f53c508279#file-reftest-failures-L3 or https://gist.github.com/Manishearth/086118c940ff86a6cfc573f53c508279#file-reftest-failures-L10
15:38bzOn the other hand, at least the latter is now fixed....
15:49crowbotjdm: nox said I know how to avoid the double boxing for cancellable promises.
15:49crowbotjdm: nox said that https://github.com/servo/servo/commit/0c33d6168e1eda13ee260ee71fa03365ab2cad52 confirms that there are at least no misused runnables.
15:50jdmXidorn: I have a patch that makes bavkground-image&#39;s specified value use small; I gave up on it because it made perf worse, but I didn&#39;t think about the effect on PropertyDeclaration
15:51bzWhat about computed value?
15:51bzAfaict, that one is way bigger than it needs to be as well...
15:53jdmCV uses smallvec already
15:58jdmMateon1: try -j1
16:22Manishearthbz: https://bugzilla.mozilla.org/show_bug.cgi?id=1396044
16:22firebotBug 1396044 DUPLICATE, canaltinova@gmail.com stylo: Fix @page test failures
16:22Manishearthbz: https://bugzilla.mozilla.org/show_bug.cgi?id=1393603 (wontfix)
16:22firebotBug 1393603 NEW, nobody@mozilla.org stylo: Handle overflow:scroll for viewport units
16:36bzManishearth: Great, thanks
16:42* jdm tries creating a Vec wrapper that transparently tries to avoid the over-allocation problem
16:47ferjmcould anyone with permissions r=me on https://github.com/servo/servo/pull/18370 please?
16:47crowbotPR #18370: An observer disconnected after a mark must receive the mark - https://github.com/servo/servo/pull/18370
16:49Manishearthjdm: we have one?
16:49Manishearthjdm: sewardj wrote one
16:50jdm@manishearth: oh, where?
16:50ferjmjdm: thanks!
16:51Manishearthjdm: https://b