mozilla :: #servo

13 Aug 2017
02:32heycamemilio: thanks for the fast reviews, and profiling the facebook stuff with my patches :-)
02:32* heycam found no improvement on talos, unfortunately
09:59noxSimonSapin: "edited by SimonSapin"
09:59noxIs that because of Reviewable somehow?
10:03Manishearthnox: iirc reviewable uses lars' account
10:03noxAll PRs are currently marked as being edited by Simon.
10:56bojanIs this an issue that should be reported? Servo doesn't know my screen resolution
10:57travis-ciServo failed to build with Rust nightly: CC nox, SimonSapin
10:57bojanAlso browser window size is incorrect
11:40noxbojan: Always nice to report issues, yes.
11:44eaglenox: :
12:11SimonSapinnox: I guess because I'm the one who reenabled reviewable avec we switched the permission model
12:11noxThat's what I assumed too.
12:11noxSimonSapin: You can just call yourself a triage expert now.
12:37larsbergEmily and I were talking about re-signing up reviewable as bors-servo instead
13:24bojannox: I know bugs need to be reported, but I don't know if everything should be working correctly or if some features are missing or something
13:25bojanfor example Spheres and Transparent demos don't work, but I assume you guys know about it
13:25bojanok apparently demos work now, they didn't two days ago :D
13:26bojanand now it doesn't
14:39bojanAny way to view javascript errors in servo? From nighly build, not building it locally
14:50emilionox: r? ^
14:50emiliobojan: they should appear on the console prefixed with |ERROR: |
15:02bojanemilio: I'm running developer preview on windows 10
15:16bojanI hope there is enough info ^^
15:21emiliobojan: nice report, thanks!
15:36bojanwould help more if I knew how :)
15:38emiliobojan: so it seems that Screen::Width/Height aren't implemented at all
15:38bojanI also noticed I had to use textContent instead of innerText to make it work on servo
19:55bholleyemilio: yt?
19:55emiliobholley: kinda, yeah (on my phone)
19:55bholleyemilio: I applied the patches from bug 1382925, but I still see roughly the same overhead
19:55firebot ASSIGNED, stylo: Keep the UA parts of the stylist across changes to document sheets
19:56emiliobholley: where, on facebook you mean? I only see ~4ms
19:56bholleyemilio: this is, logged out?
19:56emiliobholley: yes
19:56bholleyemilio: looking just to first paint, or after?
19:57emiliobholley: looking to four consecutive page loads, on average
19:57* emilio sees that the profile link he posted is broken
19:57bholleyemilio: but in terms of what you're measuring, are you restricting the profile to first non-blank paint, or measuring the entire thing?
19:58bholleyemilio: if this would be easier to get to the bottom of sometime when you're back at your computer, we can wait :-)
20:01emiliobholley: I'm doing the entire thing. Basically, the procedure was: Open, stop profiler, start profiler, shift reload ~4 times
20:01emiliobholley: then see how the stacks looked like
20:01emiliobholley: let me go to my computer for a second
20:03emiliobholley: ok, back, let me take a profile real quick
20:03bholleyemilio: and to be clear, I built central, turned LTO back on, and cherry-pick and the 4 previous commits (5 total)
20:05emiliobholley: so, here's what I see:
20:06emiliobholley: this is cam's patches as-reviewed (without the fixups) on top of "Backed out changeset c73631b194bb (bug 1389740) for failing Firefox-UI's and r=backout"
20:06firebot ASSIGNED, Consolidate URL bar history dropmarker styling
20:07emiliobholley: I see 7ms under Servo_StyleSet_FlushStyleSets per load, 24ms total
20:08emiliobholley: well, ~7ms (otherwise the sum wouldn't be 24)
20:08emiliobholley: let's say 6ms
20:08bholleyemilio: well, I'm measuring Servo_Styleset_*
20:08bholleyemilio: I see 14ms of that before the first non-blank paint
20:10emiliobholley: on a single page load? I see a few ResolveForDeclarations calls (wtf is FB's front page doing with Canvas2d?)
20:10bholleyemilio: so, go to 'markers'
20:10bholleythen type in 'blank' into the filter box
20:10bholleyemilio: you'll see markers for first non-blank paint after XXX ms
20:10emiliobholley: should I remove the callstack filter?
20:10* emilio was filtering Servo_StyleSet_
20:10bholleyemilio: callstack filter doesn't matter - it's a separate filter
20:11bholleyemilio: take the first non-blank paint (the one that says "after 337 ms"), right click, and say "set selection end time here"
20:11bholleythen type "nav" in the filter box, and select the Navigation::Start
20:11bholleyemilio: and set the start time there. then click the plus on the magnifying glass
20:12bholleyemilio: the resulting intervals (displayed at the top) should be ~337ms (I get 338)
20:12bholleyemilio: this is the interval that Tp6 measures
20:12bholleyemilio: and it's also a relatively comparable interval between servo and gecko
20:13bholleyemilio: now go back to the call tree, and type Servo_StyleSet_ into the filter box, you should see 14ms
20:14bholleyemilio: if you measure the same interval in gecko, you'll see a time about 3x less spent in RefreshRuleCascade, which is consistent with other measurements that I've taken
20:14bholleyemilio: (lmk if any of that doesn't repro for you)
20:15emiliobholley: so, I do get a 338ms interval
20:15emiliobholley: now, from there: 5ms are Servo_StyleSet_FlushStyleSheets
20:15bholleyemilio: on the profile you linked? I see 7
20:16emiliobholley: I was looking at one call, how do you combine them? But basically, there's one big 5ms flush, then a few smaller flush presumably during styleset creation
20:17bholleyemilio: you just type the thing in the filter box, and then look at the number at the root of the callstack
20:17bholleyemilio: It's called "(root)"
20:17bholleyemilio: the number on the left is the cumulative run time under that segment of the callgraph
20:17emiliobholley: ok, then I get 7
20:18bholleyemilio: ok good. And then if you just filter for Servo_StyleSet_ on its own, you should see 14
20:18emiliobholley: yup
20:18bholleyemilio: which includes the clears we do on append
20:18bholleyemilio: so my measurements across the sites in Tp6 pretty consistently show us spending about 3x as much time in Servo_StyleSet_* as gecko spends in RefreshRuleCascade
20:19bholleyemilio: so unless there's some other part of Gecko I'm not measuring but should be, that suggests we're still about 3x slower in stylist handling
20:19emiliobholley: well, Servo_StyleSet_* isn't just RefreshRuleCascade
20:19bholleyemilio: what other work should I measure on the gecko side?
20:19emiliobholley: RefreshRuleCascade is FlushStyleSheets
20:19bholleyemilio: sure, but gecko does no measurable work in append
20:19emiliobholley: presumably nsStyleSet::*
20:20bholleyemilio: (can't measure that, because that includes ResolveStyleFor)
20:20bholleyemilio: I just measure the equivalent callsites that cause stylo to do the flush
20:20bholleyemilio: for example, nsDocument::AddStyleSheetsToStyleSets
20:20emiliobholley: I can try making clears to keep allocated data, and see if it helps, but I had a patch for that and didn't measurably help on the microbenchmark
20:20bholleyerr, AddStyleSheetToStyleSets
20:21bholley(which doesn't even register on Gecko)
20:21bholleyemilio: well, so
20:21bholleyemilio: the question is whether the allocs are hurting us, or the drops
20:21bholleyemilio: measurements indicate the drops
20:21bholleyemilio: which makes me wonder if it's the atomic arc ops that hurt us
20:22emiliobholley: I'd doubt that, they should be mostly uncontended
20:22bholleyemilio: an uncontended atomic op is still 5ns
20:22emiliobholley: let me look at SmallVec::drop in case we're doing something stupid
20:22bholleyemilio: does gecko clear the hashtables as well in AddStyleSheetToStyleSets?
20:24bholleyemilio: because if it does, it manages to be pretty darn fast about it
20:24emiliobholley: Hmm... It does not, because it keeps a cache of media query effective results. But it needs when stylesheets are added anyway because that would mean that the cached values will no longer match...
20:26bholleyemilio: yeah it seems to call DirtyRuleProcessors, which AFAICT just defers the work to the next RefreshRuleCascade
20:26emiliobholley: there's ClearRuleCascades
20:26emiliobholley: which is what should drop most of the stuff
20:27bholleyemilio: ok - that doesn't register in my profiles either
20:27bholleyemilio: so it seems like we're either adding vastly more data to our hashmaps, or the data is vastly more expensive to manipulate, or the hashmaps are slower, or some collection of all three
20:28bholleyemilio: would be good to understand which
20:28emiliobholley: there's also the RuleProcessorCache, btw
20:28emiliobholley: so I'm pretty sure the InvalidationMap stuff makes us do more work than needed. I could look at optimizing that.
20:29emiliobholley: our hash functions should be non-existent now (because of the precomputed hash), so if something is slower, it'd be the hashmap impl itself
20:29bholleyyeah, which seems unlikely
20:30emiliobholley: and we don't add vastly more data to our hashmaps, but we do refcount a bunch more stuff (in Gecko the selectors are owned by the stylesheet, and there are only weak refs and clears when they go away)
20:30emiliobholley: well, probably we do a bit of extra work due to the invalidation map, that's likely to be a measurable part of the rebuild stuff
20:31bholleyemilio: so I see 6.5ms (~half the total time) in invalidation_map::*
20:32bholleyemilio: so that's definitely part of the explanation. Which is unfortunate, because gecko doesn't have that to worry about
20:35emiliobholley: and that's pretty much everything either hashtable operations, atom refcounting, and a bunch of drop stuff... I can try to see how do the invalidations look like and see if it's worth it to change how we store them in a few cases. I have a few ideas about that... But I suspect that if they look as I think they do it's not going to help that much...
20:36* emilio wonders if it'd be more productive to look at the perf of SelectorMap itself
20:37bholleyemilio: one thing I&#39;m trying now is to add a wrapper struct around the Selector<Impl> in Rule and Dependency so that it addrefs/releases the arc twice
20:37bholleyemilio: and see how much that impacts perf
20:39bholleyemilio: or maybe just forcing non-atomic reference counting directly and seeing how far that gets us
20:40bholleyemilio: I can just run with stylo_threads=1 :-)
20:42emiliobholley: heh
21:05emiliobholley: first hacks falling as part of bug 1389743 :)
21:05firebot NEW, Never do sync frame construction when removing content from the DOM.
21:21noxemilio: I can&#39;t really derive the impl for ExtremumLength because the macro that defines it is in style_traits.
21:22noxemilio: Do you mind if I just let the FIXME for now? It&#39;s not a regression.
21:22emilionox: sounds ok to me
21:23emilionox: maybe file a bug so hiro or boris look at it?
21:23emilionox: and / or to the other FIXMEs that you left
21:23noxWill do.
21:23emilionox: and seriously, _thanks_ for taking care of cleaning all the stuff up, it&#39;s awesome :)
21:24noxThanks, I&#39;m happy to remove code.
21:42mystornox: Awesome!
21:43mystornox: I am thinking of releasing the new API soon but I&#39;d like to test it on some real-world derives - do you have any suggestions about which ones in servo might be good for me to make sure work nicely with the new API?
21:43noxmystor: Does your PR introduces a way to get the full variant name prefixed by the type name if it&#39;s an enum?
21:44noxmystor: :)
21:44mystornox: If you call `variant.pat()` in the new one the pattern includes the type name prefix if its an enum
21:44mystornox: Umm - I should expose a method for like &quot;full_variant_name()&quot; or something
21:44noxmystor: What&#39;s `variant` there?
21:44noxWhere do the variable names in the pattern come from?
21:46mystornox: Basically synstructure builds a full `Structure` object with a `BindingInfo` object for each binding and a `VariantInfo` object for each variant which holds references into the original AST, and then it lets you mutate that tree however you want with some helper methods. So you can do things like set the binding&#39;s name (it&#39;s a public field) or use the
21:46mystor`binding_name(|bi, idx| ...)` method to compute the name for each binding quickly.
21:46noxOh ok, I thought this was syn::Variant so I was confused.
21:47mystornox: Nah, synstructure uses a parallel `TInfo` types
21:47mystornox: so we can store extra information like the binding names
21:47noxOk, makes sense.
21:48mystornox: A BindingInfo contains a &syn::Field member, a VariantInfo contains a VariantAst<&#39;a> field (which is a syn::Variant except each field is a reference so we can cheaply create them for structs which don&#39;t have syn::Variants)
21:48mystorand the Structure object contains a &syn::DeriveInput
21:49mystornox: I&#39;ll throw the rustdocs up somewhere if you want?
21:49noxThat would be nice yeah. :) I almost fetched the PR to build it locally but then got distracted as usual, probably by my cats.
21:52mystornox: I really need to submit my patch for fixing the highlighting of the `#` character to rustc to fix that ugly highlighting
21:53noxmystor: Nice trick with the braces, but wouldn&#39;t that consume any used &mut argument?
21:53mystornox: hmm
21:53mystornox: I was trying to make it so you didn&#39;t have to worry about your outputs interfering with eachother
21:54mystornox: Didn&#39;t really think of that :-/
21:54noxI&#39;m not sure it&#39;s actually a problem.
21:54mystormay not be
21:54nox.fold is neat. :)
21:55mystoryeah, that one was inspired by you
21:56noxThere are only 2 remaining traits in style that are not derived yet.
21:56mystornox: noice!
21:56mystornox: Which ones?
21:57nox and
21:57noxThe first one I introduced just now ComputeSquaredDistance to remove the compute_distance methods that shouldn&#39;t be here,
21:57noxand I have an idea to merge the remaining methods into a single one and derive just that.
21:58noxThere are some impls that are humongous and with the multiple methods that doesn&#39;t help.
22:00noxmystor: Cf. :D
22:00firebotBug 1377594 NEW, stylo: 374k in animated_properties functions
22:26xidornemilio: why do you think gecko doesn&#39;t handle consistently with @font-face?
22:26xidornemilio: IIRC I almost follow the handling of @font-face when I developed @counter-style
22:27emilioxidorn: see the test-case that fails in Gecko... _maybe_ it&#39;s just that it gets the font faces for document.fonts in a different order it does the lookup
22:30xidornemilio: sounds like gecko&#39;s order is correct
22:30Aryxemilio: hi, still here? 1382925 also triggers an unexpected pass
22:31emilioxidorn: hmm... well, it depends what order font-face rules are returned, which is what I&#39;m saying... We return it from most specific (author) to less (user-agent), but seems Gecko is doing the opposite.
22:31xidornemilio: the iteration order from fonts should be the document order, per font-loading
22:31emilioAryx: looking, will update
22:32Aryxthank you
22:32xidornwhich indicates that author should really be the last
22:32emilioxidorn: link?
22:32emilioAryx: thank _you_, as always :)
22:33Aryxbtw, what&#39;s the ideal way to inform stylo people of new stylo related test failures which are not related to the tests but hit randomly?
22:34emilioAryx: like, intermittents?
22:35emilioxidorn: ok, seems like that&#39;s a stylo bug then, I agree... :)
22:35emilioAryx: probably, if none of us isn&#39;t around, filing a bug blocking bug 1341102 or similar
22:35firebot NEW, stylo: Bustage metabug
22:35Aryxyes, e.g.
22:35Aryxok, will block that one
22:36emilioAryx: huh, that sounds pretty much non-related at all to stylo... does that happen with non-stylo builds too?
22:36Aryxi have to check, might be the 4th hit in the last two days
22:37emilioAryx: anyway, feel free to file against that one, and maybe ni? any of us, and we&#39;ll just remove the blocking field if we&#39;re confident that it&#39;s not a stylo bug per se
22:44xidornemilio: new try push for rust-bindgen:
23:15xidornemilio: I just realized that we include the 1.3MB example png image in every bindgen package...
23:17JayfluxWhy does servo currently take up so much CPU?
23:23noxemilio: Still around?
23:23emilionox: for a bit
23:26noxemilio: Am starting to work on this:
23:26noxBranch name because it&#39;s a code exodus.
23:29emilionox: is its attempt to just move away as much as possible from ``?
23:29emilionox: if so, neat
23:29noxemilio: Yes, given I created a values::animated module, might as well put stuff where they belong.
23:50noxemilio: Am creating &#39;percentage&#39; submodules too.
23:50noxI&#39;m tired of the long files.
23:59njnseriously, downloading rustc again?
14 Aug 2017
No messages
Last message: 5 days and 13 hours ago