mozilla :: #servo

8 Aug 2017
00:03xidornnjn: nvm, I found it
00:04njnxidorn: it's printed to the console
00:04xidornnjn: I used the save report
00:04xidornnjn: it would be helpful if I know the name pattern at the very beginning...
00:04xidornwindows' temp dir tends to be messy :/
00:05njnxidorn: on my windows machine I think it just ends up in /tmp/dmd*
00:05xidornnjn: it's in C:\Users\upsuper\AppData\Local\Temp for me
00:05xidornnjn: but that dir contains 2k+ items
00:05njnxidorn: just look for the files that start with "dmd" :)
00:06xidornnjn: yeah, I didn't know that the file name starts with dmd
00:06xidornprobably it would be good to show the path in about:memory when saving
00:10njnxidorn: I added &quot; It&#39;s always in a temp directory, and the filenames are always of the form dmd-<pid>.&quot; to the MDN docs
00:11njnxidorn: I can&#39;t remember the exact reason why now, but printing the filename to about:memory was much harder than doing it to the console, unfortunately, which is why it&#39;s not shown there :/
00:11xidornnjn: oh, it outpus to console... ok
00:12njnxidorn: but on Windows, console output doesn&#39;t seem to show up. At least on my machine it doesn&#39;t. I&#39;ve never worked that one out
00:12njnI think I&#39;ve managed to redirect it to file before, but it doesn&#39;t seem to be reliable
00:18bholleyxidorn: njn: so bz&#39;s theory about memory is a really compelling one
00:18bholleywe should try to validate it
00:18njnbholley: which theory?
00:18* bholley looks for the bug
00:19xidornbholley: I saw the arena thing makes lots of sense
00:19bholleyok I guess he didn&#39;t make it very clear in the bug
00:19njnbholley: https://bugzilla.mozilla.org/show_bug.cgi?id=1367854#c17 ?
00:19firebotBug 1367854 NEW, xidorn+moz@upsuper.org stylo: Memory usage seems to be higher than Gecko
00:19njnGeckoDisplay sharing?
00:19bholleynjn: exactly
00:19bholleynjn: xidorn: Let me write another comment to clarify the issue a bit more
00:19bholleythen we can discuss
00:19xidornk
00:19* njn doesn&#39;t even know what a GeckoDisplay is
00:20bholleynjn: it&#39;s a style struct
00:25njnAh crap. Rust&#39;s HashMap doesn&#39;t have a way to get at the pointer to the elements. Which means I can&#39;t implement MallocSizeOf properly for it.
00:25njnI can computed the size instead of measuring it with a malloc_size_of function, but DMD doesn&#39;t know about that and will think it&#39;s not being reported
00:28njnThis goes back to https://github.com/servo/servo/issues/6908
00:28crowbotIssue #6908: Measure the memory usage of HashMap better - https://github.com/servo/servo/issues/6908
00:29xidornnjn: we should really ask rust team to expose the raw table pointer somehow I guess
00:30njnxidorn: that&#39;s assuming it&#39;s a single pointer! it&#39;s conceivable that a hashtable might have separate blocks for keys and values, for example
00:30xidornnjn: for rust&#39;s hashmap, it seems to have only one for now
00:30njnxidorn: yes, but it&#39;s an API design issue
00:30mbrubeckIs anyone working on a rustup for Servo?
00:30xidornnjn: yeah...
00:30njnxidorn: Vec is easier, there&#39;s an obvious representation
00:31xidornnjn: I guess rust can design a set of new api which reports all pointers a type uses in a vec... somehow
00:32njnxidorn: the Rust folks won&#39;t like that much, I suspect
00:33njnwe&#39;ve already had some discussions about duplicating Rust&#39;s hash maps so that fallible methods can be added. This is another motivator for that...
00:33bholleynjn: we already made our own Arc. Making our own hashmap seems fine
00:33bholleynjn: and would kill two birds with one stone
00:33njnbholley: yay for scope creep
00:34bholleynjn: it&#39;s just copy-paste :-P
00:34xidornbholley: what&#39;s the other bird?
00:34bholleyxidorn: fallible allocations
00:34xidornok...
00:34bholleyxidorn: njn: Just commented in the bug explaining the theory
00:34bholleywriting another comment about next steps
00:34xidorn(and we may be able to optimize it for our own use in the future as well... like we probably don&#39;t really need the randomness)
00:35njnbholley: sewardj was going to do the hashmap duplication, but I might beat him to it now
00:39njnbholley: much of that explanation went over my head, but it sounds promising :)
00:45xidornbholley: we don&#39;t really need changing code for that I suppose... dmd live output should be enough
00:46bholleyxidorn: if bug 1367635 is the solution, it might make more sense to have heycam take it over - your choice
00:46firebothttps://bugzil.la/1367635 NEW, nobody@mozilla.org stylo: consider caching reset structs in the rule tree and shrinking ServoComputedValues
00:46bholleyxidorn: but probably worth at least validating the theory
00:47xidornbholley: yeah, I&#39;ll try analyzing the dmd output and see if there are particularly more non-inherit ones
00:47bholleyxidorn: though they&#39;re not necessarily dark-matter
00:48xidornbholley: yeah, I&#39;ll measure them with live mode
00:48bholleyah ok
00:48xidornbholley: btw, is there env var to disable stylo?
00:49* bholley tries to remember
00:49bholleyxidorn: there is FORCE_STYLO_ENABLED
00:49xidornbholley: but that&#39;s for enabling?
00:49bholleyxidorn: so you could pref it off in the build, and then set that var when you want to enable
00:49xidornok...
00:50jryansSTYLO_FORCE_ENABLED, just to be clear
00:54xidornoh... wait... we would need code change for counting... because we use arena for style structs in gecko :/
00:55bholleyxidorn: yeah I was thinking we could just log in the ctor/dtor
00:55bholleyxidorn: like what that logging patch does for style structs
00:55xidornbholley: yeah, I&#39;ll do that
00:57njnbholley: looks like I need to clone HashMap and FnvHashMap
00:59bholleynjn: how come?
00:59xidornbholley: it seems we have MOZ_COUNT_CTOR/DTOR for style structs already. I guess I can probably reuse that somehow?
01:00njnbholley: because Stylist uses FnvHashMap
01:00bholleyxidorn: yeah, maybe override the macro just for nsStyleStruct.cpp?
01:00bholleyxidorn: (otherwise too spammy)
01:00bholleynjn: but why do we need to clone the stylist?
01:00bholleynjn: oh
01:00bholleynjn: sorry
01:01bholleynjn: I thought you meant Clone trait
01:01njnsorry, no :)
01:01bholleynjn: in general we should just use FnvHashMap for all the stuff in style
01:01njnyep
01:01bholleynjn: or better, FxHashMap
01:01bholleynjn: so no need to make two versions
01:01bholleyIMO
01:01njnRust&#39;s default is ridiculous
01:01* bholley totally agrees
01:02bholleyManishearth: oh crap, looks like the HASR machinery still may depend on the style state of the parent in http://searchfox.org/mozilla-central/rev/bd39b6170f04afeefc751a23bb04e18bbd10352b/servo/components/style/rule_tree/mod.rs#1240
01:07njnwhy does dom/ have all these FooBase typedefs? Just makes things harder to follow, IMO
01:14froydnjnjn: ah, yes, those are very annoying
01:14njnbholley: should I just copy the code from the current Rust tip?
01:17bholleynjn: fine with me - though we&#39;re copying FnvHashMap, right? Isn&#39;t that a separate crate?
01:17njnbholley: we&#39;ll need both
01:17njnFnvHashMap is a very thin wrapper crate
01:17bholleynjn: oh I see, it (and FxHashMap) use HashMap under the hood?
01:18njnor FxHashMap
01:18heycamnjn: it makes it (slightly) easier to explicitly call a parent class&#39;s function. that&#39;s the only reason I can see.
01:18bholleynjn: ok, in that case rust tip is fine. That&#39;s whatI did for Arc
01:18njnyeah, FnvHashMap crate is basically just this: pub type FnvHashMap<K, V> = HashMap<K, V, BuildHasherDefault<FnvHasher>>;
01:18njnplus the definition of FnvHasher
01:18bholleyI see
01:22Manishearthbholley: hmm
01:22Manishearthbholley: so what should we be doing there?
01:22bholleyManishearth: in a perfect world we&#39;d figure out what&#39;s causing us to not be styled here
01:23bholleyManishearth: in the mean time, wallpaper is worth a try
01:27njnManishearth: I&#39;m duplicating HashMap into Stylo, so we can modify it for memory reporting and fallible allocations
01:28njnManishearth: I could do with advice about the mechanics of this; my Rust module-foo is weak
01:28njnManishearth: I&#39;ve copied the five files from src/libstd/collections/hash/ into servo/components/servo_hash/, and renamed mod.rs as lib.rs
01:29njnManishearth: and I copied and modified servo/components/servo_arc/Cargo.toml into servo/components/servo_hash/Cargo.toml
01:34Manishearthbholley: &quot;wallpaper&quot;?
01:35Manishearthnjn: that should probably Just Work? I&#39;m not very fond of doing this though
01:35Manishearthalso hashmap is complex and the impl likely relies on unstable stuff
01:40njnManishearth: I get this https://pastebin.mozilla.org/9029098, even though I have |#[cfg(feature = &quot;servo&quot;)] extern crate servo_hash;| in servo/components/style/lib.rs
01:40bholleyManishearth: &quot;wallpaper&quot; means covering up a bug by handling the aberrant behavior
01:40bholleyManishearth: in this case, just detecting unstyled elements there and returning false
01:40Manishearthbholley: oh ok
01:40bholleyManishearth: meaning &quot;you have a hole in the wall, and you put wallpaper over it&quot;
01:41Manishearthbholley: yeah
01:41njnoh wait, I don&#39;t want the #[cfg(feature = &quot;servo&quot;)]
01:41Manishearthnjn: aren&#39;t you measuring memory for gecko?
01:41njnyeah :)
01:42njnManishearth: now I get &quot;can&#39;t find crate&quot;, which is more sensible...
01:42Manishearthnjn: well it&#39;s not a crate, is it?
01:42Manishearthit&#39;s a module?
01:42Manishearthmod servo_hash probably makes more sense
01:43njnManishearth: I&#39;m cargo-culting servo_arc
01:43njnit&#39;s both a crate and a library, AIUI?
01:43Manishearthyes
01:43Manishearthnjn: oh, you compied servo_hash to components/ ?
01:44Manishearthyeah then you need a servo_hash = {path=../servo_hash} in style&#39;s cargo.toml
01:44Manishearthor something like that
01:44njnManishearth: I have that: servo_hash = {path = &quot;../servo_hash&quot;, optional = true}
01:44njnin servo/components/style/Cargo.toml
01:44Manishearthah ok
01:44Manishearthnjn: does the &quot;gecko&quot; feature enable it?
01:45njnin servo_hash/Cargo.toml?
01:46Manishearthnjn: in style/Cargo.toml
01:46njnI have #[cfg(feature = &quot;gecko&quot;)] extern crate servo_hash; in that file
01:52bradwerthhttps://bugzilla.mozilla.org/show_bug.cgi?id=1385884
01:52firebotBug 1385884 UNCONFIRMED, bwerth@mozilla.com Stylo: With ABP installed (and huge filter list) and Stylo enabled, page continuously uses 100% CPU
02:01njnManishearth: do I need to update toolkit/library/rust/Cargo.lock ?
02:02njnor should that happen automatically?
02:13njnerror: the lock file needs to be updated but --locked was passed to prevent this
02:13njnhuh
02:30cpetersonheycam: your fix for https://github.com/servo/servo/pull/17981 included a println that logs tons of messages like &quot;serialization: Arial&quot; to stdout in Nightly builds.
02:30crowbotPR #17981: style: Preserve font-family identifier sequence when serializing. - https://github.com/servo/servo/pull/17981
02:33heycamcpeterson: yeah, the backout of the println already landed
02:33heycamit should be in today&#39;s nightly I think?
02:38xidornoops, log ctor / dtor doesn&#39;t work with stylo because they don&#39;t like memmoving...
02:40ajeffreygw: thanks!
03:10xidornManishearth: is it guaranteed that we call c++ ctor and dtor for style struct when they are allocated / released?
03:22gwanyone around that can r+ a 1-line fix? ^
03:36njnManishearth: ok, modifying Cargo.lock was required to get it working
03:54njnbholley: argh! as Manishearth said above, HashMap uses lots of unstable Rust features, and so can&#39;t be compiled with a release rustc
03:55njnbholley: so it&#39;s not just copy+pasting the code
04:03xidornbholley: done. it seems that bz&#39;s theory is right. we allocates a lot more style structs than gecko
04:41bholleynjn: so, how sure are we that we&#39;re going to need fallible versions of all these data structures?
04:42njnbholley: not 100% certain, but it seems likely
04:43njnI guess the key question is &quot;how much of Stylo&#39;s data structures are influenced by web content?&quot;
04:43njnwell, that&#39;s an overly general way of saying it, but hopefully you get what I mean
04:44bholleynjn: yeah. I&#39;m just concerned that it&#39;s a bit of a rathole. We&#39;d need to do Vec too, and SmallVec
04:44bholleynjn: and then there&#39;s all the other data structures that we allocate
04:44bholleynjn: like rule nodes
04:44bholleynjn: which we allocate with Box::new
04:44njnbholley: how big are they?
04:44bholleynjn: not big, but there can be many of them
04:45njnBox::new things shouldn&#39;t need to be falliable
04:45njnit&#39;s just the potentially big chunks that are the problem
04:45njnOOM|large, to use crash-stats parlance, not OOM|small
04:45njnif you fail to allocate a 64 byte chunk, you&#39;re hosed anyway
04:45njnif you fail to allocate a 2MB chunk, you might still be able to continue reasonably
04:45bholleynjn: oh I see
04:46bholleynjn: I do worry that we&#39;d have to make a lot of stuff fallible
04:46njnso it&#39;s about which of stylo&#39;s structures can potentially get into the 1 MiB size ranges
04:46bholleynjn: maliciously, or just by a kinda-dumb page?
04:46njneither
04:46njnpeople do weird stuff
04:47bholleynjn: in the malicious case, it seems like there are always ways
04:47njnlike have data: URIs that are 1MiB+
04:47bholleynjn: like, you could just trigger a lot of Box::new
04:47njnas I said above, that&#39;s not the issue
04:47njnhttps://bugzilla.mozilla.org/show_bug.cgi?id=1387903 is an example of a bug I filed yesterday
04:47firebotBug 1387903 NEW, alchen@mozilla.com Crash in OOM | large | NS_ABORT_OOM | CopyASCIItoUTF16 | nsStructuredCloneContainer::GetDataAsBase64
04:48njnstructured clones doing 1.2 MiB allocations
04:48njnthat&#39;s a dead simple fix because we have a fallible variant of CopyASCIItoUTF16()
04:52bholleynjn: ok, so
04:52bholleynjn: in terms of big allocations
04:53bholleynjn: your entry points are probably Servo_StyleSet_FlushStyleSheets (the hashtables) and Servo_Stylesheet_FromUTF8Bytes (parsing)
04:54bholleynjn: can you file a bug about the fallibility stuff so that we can track it?
04:54njnbholley: I can file, sure. What do you mean by entry points?
04:54bholleynjn: like, the entry points into rust code where we&#39;d be doing the large allocations
04:55bholleynjn: also, I might suggest making a fallible version of Vec first, as a warmup
04:55bholleynjn: it&#39;s a simpler API, and all the magic we do to swap it in will be analogous
04:55njnbholley: the right way to do this, which is what jseward and I were planning, is to instrument moz_xmalloc to see where large allocations are happening
04:56njnbecause it is possible that it&#39;s not a problem for Stylo in practice
04:56bholleynjn: hm
04:57bholleynjn: it would be great if it weren&#39;t a problem
04:57bholleynjn: and if it weren&#39;t a problem, I might suggest not forking HashMap over the memory reporter thing
04:57bholleynjn: because of my suggestion that we could reverse-engineer the pointer out of jemalloc
04:57njnbholley: I&#39;m talking about that with glandium in #memshrink, he thinks it&#39;s not possible
04:58glandiumI&#39;m not saying it&#39;s not possible
04:58glandiumI&#39;m saying the code doesn&#39;t exist
04:58bholleyright, we could add the code
04:58njnsloppy_malloc_usable_size(), lol
04:58bholleyexactly
04:59glandiumor a function that returns the base pointer for any allocation
04:59bholleyif it&#39;s just called on the memory reporter path, it shouldn&#39;t matter if it&#39;s a bit slow
04:59bholleyglandium: right
05:01njnglandium: isalloc_validate() is the relevant function in mozjemalloc.cpp, is that right?
05:01bholleynjn: gotta go. But your DMD reports on the allocations in bug 1367854 should give you a reasonable sense of whether any of the hashtables get particularly big, at least on obama
05:01firebothttps://bugzil.la/1367854 NEW, cam@mcc.id.au stylo: Memory usage seems to be higher than Gecko
05:03glandiumnjn: actually, now reading the code, it seems like this wouls work for small and large allocations (< 1MB - 8k)
05:03njnglandium: yeah, http://searchfox.org/mozilla-central/source/memory/mozjemalloc/mozjemalloc.cpp#3451-3462
05:04glandiumit would however blow up if you pass a pointer withing a > 1MB+8k block
05:05glandiumessentially, it assumes that if the pointer is not 1MB-aligned, it&#39;s a small or large allocation
05:05njnglandium: so that&#39;s promising... but DMD currently requires the pointer to the start of the block :/
05:05glandiumnjn: so essentially you&#39;d need a version of that function that does the extent_tree_ad_search first
05:06glandiumwhich also means locking/unlocking huge_mutex a lot of times
05:15Manishearthnjn: yes, also the hashmap impl is *super* complicated
05:16Manishearthnjn: is this for just one hashmap or all hashmaps?
05:16Manishearthwe could use ordermap instead
05:16Manishearth!crate ordermap
05:16rustbotordermap (0.2.10) - A hash table with consistent order and fast iteration. -> https://crates.io/crates/ordermap <https://docs.rs/ordermap>
05:16Manishearthit has different tradeoffs
05:16Manishearthbut we can add the APIs we need
05:16njnManishearth: any hashtable that might get big enough to be worth measuring
05:20Manishearthnjn: can we instead measure it by taking the first element and asking jemalloc about it? since all the elements will share an allocation?
05:20glandiumManishearth: that&#39;s what the whole conversation was about ;)
05:20njnManishearth: glandium and I were just talking about it. The answer is... sorta kinda maybe not really?
05:21Manishearthglandium: (I&#39;ve only seen parts of the conversation)
05:24njnif you look at the implementation, you can see that the storage pointer is sizeof(u64 + u64 + usize + usize) bytes into the HashMap...
05:26glandiumnjn: until it changes ;)
05:26njnyes
05:27glandiumrust doesn&#39;t have the option to output its type layouts yet
05:53xidornheycam: I thought your patches were just a bit of simple dom code and much of style code... actually it is a purely dom, and even dom::security code :/
05:54xidornheycam: I guess I&#39;m not the right person to review this kind of patch... I&#39;d probably redirect it to some security guys instead
05:54heycamxidorn: ok. actually maybe just redirect to bz?
05:54heycamxidorn: christian gave f+ on the approach earlier in the bug
05:54xidornheycam: oh, then probably bz
05:54xidornyeah
05:54heycamthanks
05:54xidornsorry
05:57heycamno worries
06:55xidornis it just me who finds it pretty hard to read code like a.max(XXX).min(XXX)?
06:56xidorn#18007 just changes a piece of code from &quot;weight.min(100.).max(900.)&quot; to &quot;weight.max(100.).min(900.)&quot;... so I suppose it is not just me always gets confusing
06:56crowbotPR #18007: stylo: Fix the computation of the interpolation of FontWeight. - https://github.com/servo/servo/pull/18007
06:57xidornI propose that we rewrite all of them into form like f32::min(a, b) rather than a.min(b)... which is quite confusing...
07:14emiliogw: done, sorry for the lag
07:18emilioheycam: r? #17992
07:18crowbotPR #17992: style: Rework how precomputed pseudo stuff works, to avoid malloc/free churn. - https://github.com/servo/servo/pull/17992
07:18heycamemilio: yep, in my queue to get to before the end of the day...
07:19emilioheycam: also, re. the rule node caching stuff... I suggested a rule node -> ComputedValues thread-local cache instead, did you and Bobby consider that?
07:19emilioheycam: seems like less chances to mess up than tracking cyclic references.
07:20heycamemilio: I didn&#39;t read your comment before I spoke with Bobby. that does seem easier, but also means we&#39;ll not share as much...
07:20emilioheycam: and should be mostly equivalent (modulo that you don&#39;t get sharing across threads, but that may be an issue anyway when two threads race for the same reset data)
07:21heycamemilio: though I think the non-racing lack of sharing will be more significant than loss of sharing due to racing
07:21emilioheycam: yeah, but I suspect it will be good enough, and we could support conditions pretty easily
07:21emilioheycam: instead that just give up when we see a font-relative size
07:21emilioheycam: and we don&#39;t need to care about rebuilding the rule tree on viewport/zoom size changes
07:21heycamemilio: just clearing the cache, right
07:21emilioheycam: instead of just giving up*, I mean
07:22heycamemilio: well we could just explicitly clear the cached data from the rule nodes
07:22heycamwithout throwing away the whole tree
07:22emilioheycam: yeah, that&#39;s fair, I guess... still kinda nasty if you want to do it during the traversal (though not so much if you do it sequentially I guess)
07:23heycamemilio: sequential is probably fine there
07:23heycamI meant not going to be much that much slower than dropping a hashtable probably
07:23emilioheycam: that&#39;s fair
07:23emilioheycam: I think that the cache would still be easier to tweak / improve, and a bit more self-contained than putting more stuff in the rule tree
07:23heycamemilio: what sort of state do we currently preserve in TLS between separate traversals?
07:24emilioheycam: none, I _think_, modulo the device state (default computed values, kinds of units used...)
07:24heycamemilio: would it be easy to preserve these tables?
07:24* heycam hasn&#39;t looked at what TLS stuff we do lately
07:25emilioheycam: oh, we also preserve the bloom filter allocation, though that&#39;s somewhat nasty
07:26emilioheycam: not sure it&#39;d be super-easy, in the sense that other documents could be styled with the same thread pool
07:26heycamemilio: hmm
07:26emilioheycam: we could store them in PerDocStyleData, but if they&#39;re thread-local, chances are that you get a cache with data from other part of the dom anyway
07:27emilioheycam: and thus a bunch of misses
07:27heycamemilio: I&#39;m not sure how much split there is between rule node struct sharing within a given traversal, and between traversals, although I would guess that between traversals is still somewhat important
07:28emilioheycam: blink disables all its sharing mechanism for dynamic stuff like hover/active rules and such. I could believe it still matters to some extent, but I don&#39;t think that much, specially since we restyle less than Gecko
07:29heycamyeah, seems hard for me to tell
07:30heycamemilio: anyway would you like to just briefly reply in the bug, mentioning these tradeoffs?
07:30emilioheycam: yes, will point to the conversation
07:30heycamcheers :-)
07:35cynicaldevilnox: ping
07:36cynicaldevilI found a way to eliminate same_tree, but it is a breaking change.
07:40noxcynicaldevil: pong
07:41cynicaldevilnox: If we pass the insertion point as an extra argument to TreeSink&#39;s associate_with_form method, we can do the check there.
07:43emilioheycam: just left a comment
07:44heycamemilio: thanks
07:44emilioheycam: btw, if you want me to help out with the sharing of cascade data across origins just let me know
07:45heycamemilio: sure. I&#39;m fine for now, thanks :-)
07:46emilioheycam: sure thing, I&#39;ll look into removing more dumb stuff around there then, or the atoms stuff once froydnj replies :)
07:46heycamcool cool
07:46* heycam continues tending his review / needinfo queue and will perhaps get to doing some coding today
07:58paulIs there anyway to run some initialization code for all the rust unit tests?
08:11emilioxidorn: r/ 6
08:11emilioerr, xidorn: r? ^
08:24xidornemilio: actually I don&#39;t want to land any test fix because that would just add extra steps for me to land enabling patch :/
08:25emilioxidorn: feel free to r- for now, I guess, then
08:25xidornemilio: well, that&#39;s fine
08:26xidornemilio: I have to do that for another fix at least
08:26xidornso... it doesn&#39;t really matter
09:25noxSimonSapin: Did we do a rustup with the 40s optimisation?
09:26SimonSapinnox: no, blocked on https://github.com/rust-lang/rust/issues/43567
09:26crowbotIssue #43567: ICE &quot;anonymous bound region BrAnon(0) in return but not args&quot; - https://github.com/rust-lang/rust/issues/43567
09:26noxAlways blocked by ICE.
09:27hirokuoe0: ping
09:50kuoe0pong
09:50kuoe0horo: pong
09:50kuoe0^hiro
09:50hirokuoe0: hi! what is the status of bug 1379516?
09:50firebothttps://bugzil.la/1379516 NEW, kuoe0@mozilla.com stylo: incorrect restyle marker for animations
09:51kuoe0I haven&#39;t look at it
09:51hirokuoe0: if you are busy with other stuffs, I can care of it by myself.
09:51kuoe0ok, feel free to take it
09:51hirokuoe0: OK. thanks!
09:56cynicaldevilnox: What do you think about the idea I mentioned earlier?
09:57noxcynicaldevil: Seems doable.
09:57noxIs it how Gecko does that?
09:57cynicaldevilnox: I couldn&#39;t figure that out.
09:58cynicaldevilremoving &#39;has_parent_node&#39; seems harder to do, though.
10:13noxemilio: Why do we have generated/ files in the repository again?
10:13noxI mean components/style/gecko/generated/pseudo_element_definition.rs
10:13emilionox: so we can build geckolib without merging m-c into servo/
10:14noxI shouldn&#39;t have asked.
10:14emilionox: :)
10:14emilionox: my proposed solution for bug 1381629 would fix this, fwiw
10:14firebothttps://bugzil.la/1381629 NEW, nobody@mozilla.org stylo: Improve the workflow for landing patches that don&#39;t touch style system code like bug 1377993
10:15noxSure but it brings us to a worst place.
10:15emilionox: in which sense?
10:15noxI don&#39;t trust things that don&#39;t go through CI, no exceptions.
10:16emilionox: sure, the point is that Geckolib&#39;s CI would go through m-c when merging style/ into m-c
10:16noxCue larsberg&#39;s past anecdote of someone breaking everything by changing whitespace in a file and not going through CI.
10:17noxThe only future I want to see is style being a normally-vendored dependency, but most people on m-c&#39;s side hate the idea.
10:17noxemilio: Seems like you can land https://github.com/servo/servo/pull/17999
10:17crowbotPR #17999: style: Use an enumerated array for per-pseudo maps. - https://github.com/servo/servo/pull/17999
10:18emilionox: that&#39;s what I&#39;d prefer too, that&#39;s the point
10:18emilionox: yeah, need to write a few servo bits before landing that
10:18noxOh ok.
10:18emilionox: https://bugzilla.mozilla.org/show_bug.cgi?id=1381629#c11
10:21noxemilio: I see the PR to ensure order between custom properties,
10:21noxemilio: did you consider using http://contain-rs.github.io/linked-hash-map/linked_hash_map/?
10:21noxThat&#39;s what is used for serde_json&#39;s objects when preserving order.
10:23emilionox: I... didn&#39;t know about that one, looks reasonable, and seems like it&#39;ll probably perform better than what we do now, mind filing an issue for that? Should be trivial to do (happy to take it if needed)
10:23noxOk.
10:26xidornemilio: do you know at which place do we usually kick off the styling?
10:26noxemilio: With https://github.com/rust-lang/rust/issues/43567, are there other functions in the bindings that make the ICE happen?
10:26crowbotIssue #43567: ICE &quot;anonymous bound region BrAnon(0) in return but not args&quot; - https://github.com/rust-lang/rust/issues/43567
10:27noxCould we exclude it from the generated bindings and define it manually elsewhere with an explicit lifetime or something?
10:27emilioxidorn: are ServoRestyleManager::ProcessPendingRestyles and / or nsCSSFrameConstructor::ConstructDocElementFrame what you&#39;re looking for?
10:27noxNever mind I&#39;m dumb that&#39;s not the Gecko bindings...
10:27emilionox: there&#39;s one that is on the gecko bindings
10:28noxAh.
10:28emilionox: which is the only one that failed for me this morning
10:28nox:(
10:28emilionox: and I think that one is unused entirely
10:29xidornemilio: kinds of ConstructDocElementFrame. are we just kick off just when someone asks for style?
10:29emilioxidorn: not really
10:29emilioxidorn: we&#39;re supposed to have everything computed at once. There are three entry points I know, modulo the random NAC and XBL stuff in the frame ctor
10:30emilioxidorn: (1) ConstructDocElementFrame: This should be the initial styling. (2) ProcessPendingRestyles: This should be the normal restyling process stuff. (3) FlushThrottledAnimations, which is the animation stuff and ends up going through (2)
10:30xidornemilio: at which point do we expect styling start in ConstructDocElementFrame?
10:31emilioxidorn: on the StyleDocument call? http://searchfox.org/mozilla-central/rev/b52285fffc13f36eca6b47de735d4e4403b3859e/layout/base/nsCSSFrameConstructor.cpp#2535
10:32xidornemilio: what about the resolve request before that?
10:32xidornlike ResolveInheritingAnonymousBoxStyle(nsCSSAnonBoxes::viewport, nullptr)
10:32noxSimonSapin: r? https://github.com/servo/servo/pull/17984
10:32crowbotPR #17984: Remove style/testing feature - https://github.com/servo/servo/pull/17984
10:32emilioxidorn: the inheriting anonymous boxes can be resolved before, they update the stylist if needed
10:32xidornand UpdateViewportScrollbarStylesOverride()
10:33emilioxidorn: there are a few LazyComputeBehavior::Allow, that basically go through another path to resolve style just for that element
10:33emilioxidorn: which are the UpdateViewportScrollbarStylesOverride stuff
10:33emilioxidorn: what&#39;s up with that?
10:33xidornemilio: I&#39;m wondering about https://treeherder.mozilla.org/logviewer.html#?job_id=121598839&repo=try&lineNumber=15312
10:35xidornoh, so, it looks like gecko doesn&#39;t think the stylist needs update, while servo thinks so
10:36emilioxidorn: right, because Gecko calls UpdateStylistIfNeeded
10:36xidornotherwise ResolveInheritingAnonymousBoxStyle should have updated stylist
10:36xidornemilio: yep
10:36xidornthat&#39;s why I think gecko and servo have different sense of dirtiness at that point
10:38emilioxidorn: right... so something fishy is going on there, definitely, but someone needs to track it down...
10:38* emilio can take a look, but is swamped with other stuff
10:38xidorngecko starts with ServoStyleSet considering itself not dirty... but servo starts with thinking stylist dirty :/
10:38xidornemilio: you should do other stuff :)
10:39emilioxidorn: if SVG never ever adds a stylesheet before that, it would explain that assertion
10:40emilioxidorn: and it&#39;d be reasonable, I think, to just start off clean on the servo side
10:40xidornemilio: sounds like we should have is_device_dirty default to false, then?
10:41emilioxidorn: yeah, and is_cleared too
10:41noxPSA: If you want to write &quot;SVG&quot; in a type name,
10:41emilioxidorn: but I suspect you&#39;re going to need to feed the UA sheet right after stylist creation in servo
10:41noxplease write &quot;Svg&quot; instead.
10:41noxThanks.
10:41emilioxidorn: because I think it now relies on that is_device_dirty to update it for the first time
10:42xidornemilio: https://searchfox.org/mozilla-central/source/layout/style/ServoStyleSet.cpp#84-85
10:43emilioxidorn: sure, I mean servo&#39;s layout_thread/lib.rs
10:43xidornemilio: it seems we have been inserting sheets when initing ServoStyleSet...
10:43xidornwe probably really should make gecko think it&#39;s dirty?
10:46emilioxidorn: hmm... perhaps, yeah, at least if we appended one stylesheet
10:46xidornemilio: it seems we do fill ua sheets before initing
10:47xidornoh
10:47xidorninteresting
10:48xidornemilio: so we do fill ua sheets into ServoStyleSet before we call ::Init
10:48xidornbut when we fill it with those sheets, we don&#39;t set dirty bit because there is no mRawSet
10:48xidornso we end up having stylist includes stylesheets but gecko think everything is clean
10:49xidornnope... we end up having PerDocumentXXX includes stylesheets, but ServoStyleSet thinks stylist is clean
10:49xidornthat could also explain some of other failures
10:50emilioxidorn: right...
10:50emilioxidorn: initialization order there is supper-messy I think
10:52emilioxidorn: anyway, seems you got it mostly figured out :-)
10:52xidornemilio: yep
10:52xidornemilio: thanks for help :)
10:52emilioxidorn: np, happy to help :)
10:55travis-ciServo failed to build with Rust nightly: https://travis-ci.org/servo/servo-with-rust-nightly/builds/262192197 CC nox, SimonSapin
10:57emilionox or SimonSapin: r? ^
10:57nox&quot;warning: doc comment not used by rustdoc&quot; pff
10:57noxemilio: s/dumb hashing/hashing hashes/
10:58emilionox: fair enough :)
10:58noxemilio: r=me otherwise.
10:58emilionox: thanks!
11:06emilionox: r? ^
11:09xidornnox: do you know what is the general process if I want a new function in rust std?
11:09xidornjust file a pr against rust, or need some rfc things?
11:10noxxidorn: New trait impl can go in directly if it&#39;s not too invasive,
11:10noxnew function you must write a RFC.
11:10noxWhat was the function?
11:10xidornnox: I want a f32/f64::clamp
11:10noxWhat would that do?
11:10xidornthe somevalue.max(XXX).min(XXX) is really hard to read...
11:11noxMeh.
11:11xidornx.clamp(a, b) -> x.max(a).min(b)
11:11noxMake a trait somewhere, no need for it to be in stdlib.
11:11noxOr check if it doesn&#39;t exist already in some crate.
11:12xidornI think it&#39;s worth being in std
11:12xidornbecause min max is really hard to read :/
11:12noxI don&#39;t find it hard to read, YMMV, you will need to write a RFC. :)
11:12xidornI don&#39;t understand why people like a.min(b) over min(a, b)...
11:12noxxidorn: Because cmp::max is for Ord types.
11:13noxWhereas f32::min/max is an intrinsic method on the f32 type with some f32-specific semantics.
11:13xidornand I don&#39;t think I&#39;m the only one usually being confused...
11:13xidornhttps://github.com/servo/servo/pull/18007/files
11:14xidornhmmm, okay, it&#39;s just me
11:14xidornthat line was written by me
11:14noxxidorn: I don&#39;t think that&#39;s related to method vs function.
11:15noxTyOverby: What&#39;s https://crates.io/crates/clamp and why is the repository empty?
11:16* nox goes out for lunch.
12:01SimonSapinnox: alternatively, rustup is blocked on https://github.com/rtbo/rust-xcb/pull/41 but the maintainer hasnt given any response in a week
12:01crowbotPR #41: Use more lifetimes - https://github.com/rtbo/rust-xcb/pull/41
12:01noxUgh.
12:21emilionox: could you stamp #18012? I want to get rid of all the PRs so I can write more stuff on top of them :)
12:21crowbotPR #18012: style: Remove a few unused pseudo-element related functions. - https://github.com/servo/servo/pull/18012
12:22emilio(or SimonSapin)
12:23SimonSapinlooking
12:23SimonSapinpure code removal = instant r+ :)
12:23SimonSapinCI will catch it if its wrong
12:24emilioSimonSapin: :-)
12:24emilioSimonSapin: I also love +0 PRs
12:51xidornemilio: if you r+ bug 1388319, you can autoland that directly. I&#39;m gonna sleep now
12:51firebothttps://bugzil.la/1388319 NEW, xidorn+moz@upsuper.org stylo: Mark ServoStyleSet dirty in Init
12:51emilioxidorn: cool, thanks!
12:52emilioxidorn: yeah, looks good to me
12:56jdmcrowbot: what&#39;s new?
12:56crowbotChrome Mobile improved upon Servo&#39;s concurrent WebBluetooth :<
12:56jgraham*new*
13:07emiliosewardj: ping?
13:07sewardjemilio: hi
13:08emiliosewardj: do you know what&#39;s the reason if I call nsTraceRefcnt::WalkTheStack on a linux debug build, I only get &quot;???&quot; frames back?
13:08emiliosewardj: (s/what&#39;s the reason/how to make it work properly/, I guess)
13:09sewardjemilio: can you pastebin what you have?
13:09emiliosewardj: sure. So I want to see when are these two functions called from, in order to verify an hypothesis about the Servo-equivalent being called more often
13:10emiliohttps://www.irccloud.com/pastebin/G07W9xdi/debug.diff
13:10emiliosewardj: ^ is the patch
13:10sewardjemilio: no I meant, what is the output you get?
13:10emiliosewardj: and this is the output:
13:10sewardjah
13:10emiliohttps://www.irccloud.com/pastebin/XvFMKHU8/output
13:10emiliosewardj: ^
13:11emiliosewardj: which is... not really helpful :-)
13:11sewardjemilio: indeed not. What does ls -l say about your libxul.so ?
13:12emiliolrwxrwxrwx. 1 emilio emilio 31 Aug 8 14:43 /home/emilio/projects/moz/stylo/obj-x86_64-pc-linux-gnu/dist/bin/libxul.so -> ../../toolkit/library/libxul.so
13:12emilioand
13:12emilio-rwxr-xr-x. 1 emilio emilio 1220595448 Aug 8 15:01 /home/emilio/projects/moz/stylo/obj-x86_64-pc-linux-gnu/toolkit/library/libxul.so
13:12sewardjso, 1.22G, so, that&#39;s about the right size for with-debuginfo
13:13emiliosewardj: yeah, I&#39;m pretty sure I have debuginfo. It&#39;s a --disable-debug --enable-optimize build
13:13emiliosewardj: err, --enable-debug --disable-optimize, I mean
13:13sewardjemilio: as a side comment, do you know --enable-optimize=&quot;-g -Og&quot; ?
13:14sewardjthat gives you some optimisation but the compiler still takes care to give a good debugging experience, and it&#39;s way faster than no optimisation
13:14emiliosewardj: oh, interesting, I didn&#39;t!
13:14sewardjemilio: I use it always
13:14sewardjemilio: but sorry .. I don&#39;t know why there is no symbol info
13:14sewardjemilio: one moment ..
13:15emiliosewardj: it&#39;s a clang build, if it matters
13:15emiliosewardj: but I&#39;m pretty sure I had the same stacks on assertions on gcc builds
13:17sewardjemilio: can you pastebin the output of readelf -SW /home/emilio/projects/moz/stylo/obj-x86_64-pc-linux-gnu/dist/bin/libxul.so ?
13:18emiliohttps://www.irccloud.com/pastebin/V7YY69mL/readelf
13:18emiliosewardj: sure, ^
13:22sewardjemilio: well, it defi