mozilla :: #servo

12 Sep 2017
00:03njnmbrubeck: "error: no matching version `^1.0.4` found for package `smallbitvec` (required by `malloc_size_of`)"
00:04mbrubecknjn: That's weird... https://crates.io/crates/smallbitvec
00:04njnmbrubeck: could there be some kind of publishing/sync delay?
00:04mbrubeckI'm never experienced one before, but maybe
00:04mbrubecknjn: What if you run `cargo update -p smallbitvec`
00:05njnmbrubeck: from what directory?
00:05mbrubecknjn: Whatever one you were in when you got that error
00:05njnerror: could not find `Cargo.toml` in `/home/njn/moz/mc1` or any parent directory
00:06njnthat's from the top-level of m-c
00:06mbrubecknjn: Ah, an m-c build...
00:06mbrubecknjn: Try `mach vendor rust`
00:06njnyes, sorry if that wasn't clear
00:09njn"Installing cargo-vendor (this may take a few minutes)..." no shit
00:15njnmbrubeck: no dice: https://pastebin.mozilla.org/9032170
00:17mbrubecknjn: Hmm. Inside "toolkit/library/rust/" you could try running `cargo update -p smallbitvec`
00:25Manishearthstandups: preserve transform variant identities in animation (bug 1391145)
00:25standupsOk, submitted #50763 for https://www.standu.ps/user/Manishearth/
00:25firebothttps://bugzil.la/1391145 NEW, manishearth@gmail.com stylo: translate function is shown as translate3d on animation inspector
00:31njnmbrubeck: now my build is totally hosed: mozpack.errors.ErrorMessage: Symlink target path does not exist: /home/njn/moz/mc1/third_party/rust/encoding_c/include/encoding_rs.h
00:31njnmbrubeck: I'm going to try to go back to 1.0.3
00:34njndammit, this vendoring has completely screwed my patch
01:03gw\o/
01:05njnhow do I run the tests in servo/tests/compiletest/plugin/compile-fail/ ?
01:07jdmnjn: ./mach test-compiletest
01:08njnjdm: thx
01:08njnjdm: converting that closure from a Box to a ref looks like a trail of tears
01:08jdmnjn: on one hand, that's possible. I suspect that lifetime elision might make it easier, but I'm certainly not going to delay for that.
01:09njnjdm: future lifetime elision, you mean?
01:09jdmno, elision today
01:10njnjdm: well, the compiler started asking me to add lifetimes right from the very start, and they were rapidly spreading outwards
01:10jdmnjn: https://play.rust-lang.org/?gist=85084674bb6be3d979c2a8f02a661759&version=stable is what I'm talking about
01:10jdmnjn: and that works with no & on line 6 too
01:11jdmanyways, not really worth exploring at the moment
01:11njnjdm: I'll try it as a follow-up
01:11jdmwe can have an eager volunteer experiment
01:28bz_awayheycam: Do you have a sec?
01:29njnjdm: about that compile-time test -- is it going to work given that MallocSizeOf is "gecko"-only ?
01:30bz!seem heycam
01:31heycambz: hi
01:31heycambz: well, in meeting now :)
01:32bzheycam: OK. I mostly wanted to chat about what scripts you had for downloading/analyzing awsy logs
01:32bzheycam: Maybe ping me when the meeting is over?
01:33heycambz: sure
01:55bholleyheycam: my room
01:58bzbholley, heycam: Still curious what scripts we already have for grabbing stuff, so I don't duplicate work...
01:58bholleybz: jump in my room?
01:58bzum... I can do that; one sec
02:16njnarch: https://pastebin.mozilla.org/9032174
02:16njn*argh
02:18njnthis is after transplanting the Servo part of my m-c patch to servo/
02:19njnwho is around and is good with Servo compile errors?
02:20njnthis occurs when running `./mach test-stylo`, BTW
02:25emilionjn: I can help
02:25njnemilio: thank you!
02:25emilionjn: (don't ask why I'm still awake)
02:25njndon't worry, I won't
02:25njnemilio: my change is in that PR just above ^^
02:25njnall the build/test stuff works, except for `./mach test-stylo`
02:26emilionjn: yeah, I'm trying to remember what was the setup for that abomination
02:26emilionjn: I think adding the extern crate line to `tests/unit/stylo/lib.rs` (with the relevant toml change in `tests/unit/stylo/Cargo.toml`) should just work
02:27njnemilio: cool, I will tyr that
02:27emilionjn: +209 485, nice!
02:28njnemilio: urgh, that's not right
02:28njnit's missing all the added files, sigh
02:28emilionjn: whoops
02:28emilionjn: sorry, got to excited for a second
02:28njnonce more with feeling: this landing procedure sucks
02:28* emilio loves negative diff
02:28emilionjn: you tell me :)
02:29njnmalloc_size_of = {path = "../../../components/malloc_size_of", features = ["gecko"]}
02:29njnemilio: you think I need the features= bit at the end?
02:29njnMallocSizeOf is gecko-only...
02:29emilionjn: have you defined the feature anywhere?
02:30emilionjn: otherwise it'll do nothing
02:30njnnot within that crate, and I got a compile error with it
02:30njntaking it out seems to work better
02:30emilionjn: then no, you don't need it :)
02:30njnk, thx
02:30* emilio goes to sleep
02:41njncan someone please add `@bors-servo r=jdm` to https://github.com/servo/servo/pull/18452 ?
02:41crowbotPR #18452: Overhaul MallocSizeOf and related things. - https://github.com/servo/servo/pull/18452
02:41njngw, maybe?
02:42gwnjn: is there a reference somewhere that says r=jdm?
02:42njngw: https://bugzilla.mozilla.org/show_bug.cgi?id=1398737
02:42firebotBug 1398737 NEW, nobody@mozilla.org Overhaul MallocSizeOf and related things
02:44gwnjn: I'm not really up on how the whole servo/gecko review process works, but if you're certain you have r=jdm, I'll r+ it :)
02:44njngw: lol, I am
02:44njngw: TL;DR: it sucks
02:44gwnjn: ok, r+'ed
02:44njngw: slightly longer version: he gave r+ on the whole thing, and this PR is for the servo side
02:45njnin this case, there are only tiny Cargo.lock changes on the Gecko side
02:45njngw: thank you
02:45gwnjn: np
02:54bholleyheycam: sending along the links now
02:54heycambholley: thanks
02:56bzThanks for the scripts!
02:56gwIs there a recommended method / crate in servo to do static asserts?
03:00heycambz: np!
03:00emiliogw: depends on which kind of asserts
03:01emiliogw: what are you trying to assert? if it's equality between two numbers, then a dead function that transmutes `[0;C1]` to `[0;C2]` should do it
03:01bzheycam: so does awsy take each measurement 3 times or something?
03:02gwemilio: I want to statically assert that the size of a structure matches a constant value
03:02emiliogw: then that should work, let me find a concrete example
03:03gwemilio: weren't you supposed to be asleep!? I can find an example of that, which should work nicely. thanks!
03:03emiliogw: this is kinda full of mako templates, but https://github.com/servo/servo/blob/7fc2c435513feadf1dc666e7873095884dfd6d84/components/style/properties/gecko.mako.rs#L1318 should work
03:03emiliogw: I may have gotten distracted, whoops
03:04gwemilio: perfect, thanks
03:05emiliogw: err, should be something like `transmute::<Struct, [u8; EXPECTED_BYTE_SIZE]>([0; EXPECTED_BYTE_SIZE])`, I&#39;d think
03:06heycambz: yes it runs the whole set of tp5 page loads three times
03:06heycambz: but only the last iteration contributes to the results
03:06heycambz: (for the tabs open / tabs closed checkpoints)
03:07bzI see
03:07bzHrm
03:07bzSo did my push at https://treeherder.mozilla.org/#/jobs?repo=try&revision=8fff2e28b5e234886fa7c18a42ec07858896b39f end up doing stylo somehow?
03:08emiliobz: stylo is now normal linux64 isn&#39;t it?
03:08heycambz: it may well have, due to the recent flip in defaults to linux64 = stylo enabled
03:08bzugh
03:08bzOK
03:09* bz wonders how to turn that off
03:09heycambz: I&#39;ve been just massaging all.js in my try runs :?
03:09heycam:/
03:09* heycam isn&#39;t confident about how all the taskcluster etc. things interact so wanted to be sure
03:10bzyeah
03:10gwemilio: yup, that&#39;s what I&#39;m using - works well :)
03:10bzOK, well, in that case I won&#39;t have any useful numbers until after this next try run finishes...
03:50AareonHi
03:50AareonCloning Servo rn :)
03:58AareonI don&#39;t know the first thing about Rust xD
04:05bholleyheycam: back, everything go ok with the pushes?
04:05heycambholley: no, got pulled into a meeting, and currently rebasing my other patches first
04:06bholleyheycam: ok
04:06heycambholley: will probably push things in half an hour or so
04:06* heycam goes downstairs to grab some lunch
04:09bholleyheycam: sounds good
04:31njnoh, snap
05:12sewardjbholley: around?
05:12bholleysewardj: hi
05:13sewardjbholley: hi. So, inlining Vec/SmallVec/HashMap try_push/try_entry actually seems to recover more perf than we lost with the fallible stuff, at least for stylist building
05:14sewardjbholley: I&#39;ll land that today.
05:14bholleysewardj: that&#39;s great. And the double stuff is marked cold/ool right?
05:14sewardjbholley: yes, double at least in the vec stuff is OOL. For hashglobe I&#39;d have to check
05:15bholleysewardj: yeah just double-check, since otherwise it could negatively impact codesize
05:15sewardjbholley: will do
05:15bholleysewardj: thanks for doing that
05:15sewardjbholley: np
05:15sewardjbholley: what I wanted to ask was
05:16sewardjbholley: after doing that, do you prefer I continue to look at fallible perf stuff, or should I look at the various valgrind related complaints w.r.t. Stylo
05:16sewardj(all of which i suspect are false positives, but not 100% sure)
05:17bholleysewardj: is there other fallible perf stuff to look at? I don&#39;t quite recall
05:18sewardjbholley: https://bugzilla.mozilla.org/show_bug.cgi?id=1398593#c1
05:18firebotBug 1398593 NEW, jseward@acm.org stylo: mitigate performance impact of fallible allocation on stylist rebuilds
05:18sewardjbholley: I only did (1) there so far. Although I suspect (2) .. (4) won&#39;t bring much
05:18sewardjand lose some functionality :-(
05:20bholleysewardj: given that you already have the setup, is it relatively easy for you to profile changes?
05:20bholleysewardj: if so, I&#39;d at least try (2) and (3)
05:20bholleysewardj: it should be just a few minutes to hack up, and would be worth seeing if it moves the needle at all
05:21sewardjbholley: yes
05:21* bholley should have asked for (2) in review, tbh
05:21bholleywe&#39;d OOM way before we hit a capacity overflow
05:22sewardjbholley: I tried (3) already, but the type changes propagated outside hashglobe, so I didn&#39;t get a working patch. Then decided to profile instead.
05:23sewardjbholley: I can try (2), but if it costs zero (given that it&#39;s on the cold path) wouldn&#39;t you prefer to leave it in? Seems like multiply overflow might be some kind of sec- risk?
05:23sewardjoh, well, panic, I guess, so is ok
05:24bholleysewardj: yeah. And keep in mind that you get a capacity overflow when you double a buffer that is 2^31 bytes long
05:24sewardjbholley: from profiling, the main cost of doubling is the realloc/memcpy
05:24bholleywhich almost certainly will have oomed
05:24sewardjtrue
05:24bholleysewardj: but I guess you&#39;re right that it&#39;s on the cold path
05:24bholleysewardj: so actually probably doesn&#39;t matter
05:25sewardjbholley: IMO the main potential win on the double paths is tuning the initial size and up-scaling ratio of the blocks
05:26bholleysewardj: yeah. But any such changes will make it harder to switch back to the standard data structures
05:26heycambholley: qq: my slightly reworked nochrome patch that I put up for review fails, because it relies on calling XRE_IsE10sParentProcess early on (in nsLayoutStatics::Initialize), and it looks like prefs to disable e10s (which chrome mochitests set) haven&#39;t taken effect at that point. but they do later when we mistakenly try to do some servo stuff, so we crash since we never called Servo_Initialize. how much do I need to care about non-e10s?
05:26bholleysewardj: the #[inline(always)] thing will be tricky...
05:26* heycam should just use the old nochrome patch that ends up disabling stylo in non-e10s for his try runs for now
05:27sewardjbholley: true. (was just going to mention the #[inline(always)] thing)
05:27bholleysewardj: maybe open an issue with rustc?
05:28bholleyheycam: so
05:28bholleyheycam: for non-e10s
05:28bholleyheycam: I think we don&#39;t need the memory win of nochrome, but it should still function
05:28sewardjbholley: ok
05:29bholleyheycam: so I think you can just make the nsLayoutStatics::Initialize stuff based on !IsContentProcess
05:29bholleyheycam: and leave everything else as-is
05:29bholleyheycam: but yes, for the try runs do whatever
05:29heycambholley: ok that makes sense, thanks
05:30bholleyheycam: been doing some more testing, and 2tier seems less crucial to 2ndpass on sites that aren&#39;t the html spec
05:30bholleyheycam: so I&#39;m curious to see the AWSY results
05:30heycambholley: ok
05:31bholleyheycam: if 2ndpass and 2tier+2ndpass are about the same, I may just go with 2ndpass
05:31heycambholley: do you want to see {2tier, 2tier+2ndpass} on top of some struct sharing patch too or not bother
05:31bholleyheycam: hm, good question
05:32* heycam sees you have all three combos of that patch for me to apply actually
05:32bholleyyeah
05:33bholleyheycam: is share still performing better than cache3?
05:34heycambholley: cache3 is slightly better than share on 4 threads. like 0.23%
05:35heycambholley: helps more on HTML spec according to bz
05:35bholleyheycam: ok, well I guess it can&#39;t hurt to see what both cache3 and share do in top of 2tier+2ndpass
05:35heycambholley: ok
05:35bholleyheycam: assuming it isn&#39;t a lot more work for you
05:35heycambholley: no. it&#39;s just mq slowness pushing and popping patches :-)
05:46bholleyManishearth: yt?
05:49njnoh man, my autoland didn&#39;t work
05:52* bholley NIs Manishearth about adding a word in bug 1380980
05:52firebothttps://bugzil.la/1380980 ASSIGNED, manishearth@gmail.com stylo: font_size::SpecifiedValue::as_font_ratio looks super-fishy.
05:54bholleyheycam: heading to bed - anything else you need from me before I go?
05:55heycambholley: no I should be fine. I need to do some small rebasing when applying your patches on top of the struct caching one it seems but it should be ok
05:56bholleyheycam: ok sounds good
05:56bholleyheycam: I&#39;m landing a lot of the base stuff in #18453
05:56crowbotPR #18453: Refactor the style cache - https://github.com/servo/servo/pull/18453
05:56bholleyheycam: which should make the patches themselves a bit smaller
05:56bholleyalso the flags refactor in bug 1399011
05:56firebothttps://bugzil.la/1399011 NEW, bobbyholley@gmail.com stylo: Swap RestyleFlags for ElementDataFlags and eliminate RestyleData
05:56heycamok
05:57bholleyheycam: anyway, shoot me with whatever you come up with so I know what to focus on in the morning
05:57heycambholley: will do
05:57bholleyheycam: thanks!
05:57bholleysewardj: (anyway, after opening that rustc issue, I&#39;m fine with you moving onto the stylo valgrind stuff)
05:58* bholley heads to bed
05:58sewardjbholley: ok, fine. (I don&#39;t mind either way -- I was merely asking for prioritisation)
05:58sewardjnight!
05:59bholleysewardj: yeah sure - thanks for all the fixes and careful measurements!
05:59sewardjnp!
06:23Manishearthbholley: i dont know how to solve that problem without adding a word
06:23Manishearthwe can alternativrly choose to not solve it
06:23Manishearthor use a u8 as a half-solution
06:24Manishearthor move that data to nsstylefont. this will bloat nsstylefont
06:24Manishearthmoving it to nsstylefont is the final post-gecko-css-removal state
06:55glandiumhuh, there&#39;s something fishy with the style bindings build script: I had an error in a MOZ_ASSERT (debug only), and that made the style bindings build script fail on an *opt* build.... which means bindings could be terribly wrong
06:55glandiumxidorn: ^
06:56emilior? anyone ^
06:56xidornglandium: don&#39;t understand what do you mean
06:57emilioxidorn: could you stamp ^^ if you&#39;re around?
06:57glandiumxidorn: I had a compile error in some mfbt header in a section that is only built on *debug* builds, and that busted opt builds, in the style bindings build script
06:57glandiumwhich implies the style bindings may be generated for debug builds on opt builds
06:57emilioglandium: we generate both debug and opt bindings
06:57glandiumemilio: because the build is not slow enough?
06:57emilioxidorn: that&#39;s still right, I think, please correct me if I&#39;m wrong
06:57xidornglandium: yeah, as emilio said, we generate both in both builds
06:58xidornglandium: because that makes it easier to update servo&#39;s in-tree binding files
06:59xidornyou would only need to do one build / download from one build
06:59glandiumand the price to pay for a one-person once-in-a-while thing is for every build to be slower?
07:00xidornit... isn&#39;t really once in a while
07:01glandiumit&#39;s still only run by one person at a time, and not on every change
07:01xidornhttps://github.com/servo/servo/commits/master/components/style/gecko/generated this directory is updated multiple times every day especially in early time
07:02xidornbefore we upload those files to treeherder, that means one person build the tree twice
07:02xidornemilio: I wonder, does servo use both in-tree structs?
07:02glandiumthey could be building with a special option. Or a separate command line that generates both bindings, or whatever
07:02xidornglandium: also, why would bindgen triggers a MOZ_ASSERT at all
07:03emilioxidorn: yes. we build and test both debug and release
07:03xidornfor test-stylo?
07:03glandiumxidorn: I had a *compile* error in a MOZ_ASSERT
07:03xidornglandium: oh, okay
07:04xidornemilio: I guess once we can run test-stylo on gecko, we can remove it from servo side
07:04emilioxidorn: yeah, that&#39;d be nice
07:04xidornemilio: I mean, remove the dual struct files situation
07:04emilioxidorn: maybe we can keep the debug one to keep the build happy
07:04xidornand only leave one there for build-geckolib to continue to work
07:04xidornyeah
07:06xidornbut before that&#39;s doable, I&#39;d rather keep generating both for now...
07:07xidornglandium: how long does it take to run bindgen?
07:07xidornglandium: I would expect it to take <1min for each binding file
07:08xidorn(and it runs in parallel, so for local build doesn&#39;t have many other changes, it makes no difference between having 1 and 2)
07:10glandiumxidorn: I don&#39;t remember exactly, but the fact that bindgen/style/geckoservo are essentially built sequentially, any one that is &quot;slow&quot; is a bottleneck. And other things are running in parallel too, which may or may not block them running in parallel (since cargo now respects the make jobserver)
07:10glandium(except if that build script doesn&#39;t, in which case one could argue it should)
07:11xidornglandium: the build script doesn&#39;t respect jobserver, and yeah it probably should
07:11xidornglandium: but if you have lots of other things running in parallel, that difference from the build script shouldn&#39;t be a significant portion anyway
07:11njnwhere is values::None_ defined?
07:11xidornnjn: values/mod.rs presumably?
07:11glandiumxidorn: it is, because of the bottleneck problem
07:12emiliobholley: around?
07:12Manishearthnjn: http://doc.servo.org/geckolib/geckoservo/index.html?search=None_
07:12glandiumxidorn: because bindgen/style/geckoservo is the long tail. C++ is long finished before they are finished
07:12njnthx
07:12Manishearthgah
07:12Manishearthno, that links to a macro
07:13Manishearthnjn: values/mod.rs
07:13njngot it, thanks
07:13xidornglandium: so probably we should make the build script respect the jobserver, and have it run parallel in early stage?
07:13xidornglandium: it would at most add one min...
07:14xidornif C++ takes very long, that one mins doesn&#39;t matter. if C++ doesn&#39;t take long, build script runs in parallel, and there is no difference between the two
07:14xidornif C++ takes long enough to block the build script in parallel, but not long enough to make 1min insignificant...
07:15xidornthat could be a problem then
07:16heycamxidorn: were you going to look at bug 1397626?
07:16firebothttps://bugzil.la/1397626 NEW, nobody@mozilla.org stylo: Consider creating refcounted wrapper object for FontFamilyList and store it in specified valu
07:16xidornheycam: I&#39;m less sure whether we really should do that
07:17heycamxidorn: ok. because you don&#39;t think it will save much? or won&#39;t have much real world impact?
07:19glandiumxidorn: so the fun thing is that cargo doesn&#39;t allow to force crates to go as early as possible
07:20xidornheycam: hmmm...
07:23emilioheycam: ping?
07:23heycamemilio: hello
07:23emilioheycam: good afternoon :)
07:23heycamemilio: I guess you didn&#39;t sleep much? :)
07:23emilioheycam: turns out the talk is today ;_;
07:23emilioheycam: so I kinda had to finish it :)
07:23heycamemilio: oof
07:24emilioheycam: bholley told me that you were going to measure the computed value count when interacting with pages for bug 1370604.
07:24firebothttps://bugzil.la/1370604 NEW, nobody@mozilla.org stylo: Consider preloading the style sharing cache with styled siblings during incremental restyle
07:24heycamemilio: yep
07:24heycamemilio: just to see what happens with after using some sites for a while
07:24heycamin gecko vs current stylo
07:25heycamor maybe stylo + the 2ndpass patch thing bobby has
07:25xidornheycam: what I thought was that probably we should try sharing struct font somehow, because that is even larger than the footprint made by font family list
07:25emilioheycam: If you haven&#39;t got to it yet I guess I can also do it. This should pretty much be stashing an atomic counter in constructor / destructor, right?
07:25heycamemilio: I have just started. (well, building, about to test soon)
07:25heycamemilio: yes I have bobby&#39;s patch for that
07:26xidornheycam: and if we eventually figure out a way to share style font like gecko, boxing font family list would effectively make everything worse
07:26emilioheycam: ok then :)
07:26heycamxidorn: gecko only manages to sharing more font structs because it&#39;s sharing the style contexts
07:26heycamxidorn: I doubt we ever get into the &quot;share a whole Font struct due to it being completely specified case&quot; these days
07:26xidornglandium: probably it makes more sense to ask make to prefer cargo tasks?
07:27emilioheycam: I personally think that bug won&#39;t be a problem in practice... Something more problematic to me seems to be that we loose all sharing when recascading
07:27xidornheycam: so we have problem on sharing the style context?
07:27emilioheycam: but that&#39;s kinda difficult to measure just stashing a counter, and may produce misguided actions :)
07:27emiliobholley: ^
07:27glandiumxidorn: cargo runs rather early already. The problem is you can&#39;t tell cargo &quot;hey, prioritize building that thing&quot;
07:28heycamemilio: well hopefully this testing will reveal one way or the other :)
07:28glandiumand it usually chooses its ordering badly
07:28heycamxidorn: yeah...
07:29heycamxidorn: bobby&#39;s bug 1398658 should help with style context sharing
07:29firebothttps://bugzil.la/1398658 NEW, bobbyholley@gmail.com stylo: Use a two-tiered setup for the style sharing cache
07:29heycamxidorn: so I guess we should see once that&#39;s landed whether we still such a difference in Font struct non-sharing
07:29xidornglandium: but can we have make prioritize cargo tasks rather than having them compete with c++ tasks?
07:29heycamemilio: do we not consult the style sharing cache when recascading?
07:30xidornheycam: yeah, that sounds like a good plan
07:30emilioheycam: well, my point is that getting a higher style context count may be for other reasons, like recascading, which I think we should look at _before_ trying to preload the cache
07:30emilioheycam: we don&#39;t
07:30glandiumxidorn: not really, even if we could, that wouldn&#39;t solve the problem anyways. cargo is dumb
07:31emilioheycam: we may not get significantly higher style context count anyway, idk
07:31emilioheycam: your testing will answer that I guess :)
07:32heycamemilio: yeah... still that is an interesting point about recascading. I wonder what the ratio of re-selector-matched restyles to recascades there are over a document&#39;s lifetime...
07:32heycamprobaly more recascades
07:33emilioheycam: yeah, that sounds likely. Resizing the viewport or zooming may make that counter grow significantly I guess?
07:33heycamyeah..
07:34xidornglandium: anyway, I think we can change to build one fewer file once we have bug 1373878 fixed
07:34firebothttps://bugzil.la/1373878 NEW, nfroyd@mozilla.com Run stylo struct layout tests on automation
07:34heycamemilio: so hitting cmd plus will basically cause us to entirely lose all sharing on the page?
07:34xidornglandium: and before that, I would rather we just keep things as-is so we don&#39;t add confusion
07:34emilioheycam: I think so, yeah... I pretty much just realized about it when bholley told me to think about the preloading
07:35emilioheycam: cool part is that we could use the second pass stuff for that
07:35emilioheycam: I _think_
07:35heycamemilio: true. recascading the entire document shouldn&#39;t need any preloading tho
07:35heycamsince we should visit everything
07:36emilioheycam: right
07:36emilioheycam: but I&#39;m not sure what the intention is about preloading
07:36emilioheycam: IIUC we can only do it safely on the root of the traversal anyway
07:36emilioheycam: (which may be enough)
07:37emilioheycam: but I&#39;m not sure how we&#39;re supposed to do it anywhere else/
07:37emilio*?
07:37heycamemilio: at each node you could poke around the siblings of the children you are about to process
07:37emilioheycam: sure, but you first need to run style invalidation and all that, otherwise you get wrong styles
07:38emilioheycam: so it would need to be the previous siblings
07:38heycamemilio: ok, sure
07:38heycam(or you could look to see what the invalidation ended up doing)
07:38emilioheycam: hmm... I guess it may work, yeah...
07:38heycam(to decide whether the later siblings were safe to look at)
07:38emilioheycam: well, I was assuming we didn&#39;t want to iterate twice
07:39heycamemilio: that would be nice...
07:39heycamemilio: I suppose looking at previous siblings matches more what happens when the sharing cache is populated normally
07:39heycamanyway
07:40* heycam will leave emilio to think about it :-)
07:41emilioheycam: right, but anyway... I&#39;d prefer not to complicate style sharing more unnecessarily... :(
07:41heycamthe &quot;unnecessarily&quot; is the key I guess
07:42emilioheycam: yeah, I guess :)
07:46SimonSapinemilio: could more inlining of thread_local! (from https://github.com/rust-lang/rust/pull/43931) make stack frames larger and cause &quot;assertion failed: distance_to_stack_bottom <= max_allowable_distance&quot; ? https://github.com/servo/servo/pull/18420#issuecomment-328769322
07:46crowbotPR #18420: Get rustc commit hash from channel manifest - https://github.com/servo/servo/pull/18420
07:46crowbotPR #43931: Make the LocalKey facade of thread_local! inlineable cross-crate. - https://github.com/rust-lang/rust/pull/43931
07:46emilioSimonSapin: yeah, was reading that now
07:47emilioSimonSapin: should be easy to verify marking `#[inline(never)]` StyleBloom::new and friends
07:49SimonSapinemilio: would that have negative effects or is it something we could land?
07:50emilioSimonSapin: r=me on that, I don&#39;t think it&#39;s hot enough to _need_ inlining
07:50SimonSapinalright, Ill try that
07:52emilioSimonSapin: I commented with the two functions I think are responsible in that PR
07:53SimonSapinthanks
08:03heycamxidorn: you know the cache of unique style=&quot;&quot; attribute values / declaration blocks, do we use that in stylo?
08:05emilioheycam: we do
08:05xidornheycam: I think we do but not sure
08:05emilioheycam: why&#39;s that?
08:06heycamemilio: just curious. looking at the markup that google docs is creating and wondering why we are creating a number of style contexts in proportion to the number of lines I type
08:06heycamall with the same style
08:06heycamand the same markup
08:06heycambut it&#39;s the case in Gecko too...
08:06heycamand it has a lot of repeated identical style=&quot;&quot; attributes
08:06emilioheycam: they&#39;re probably editor styles? How does gecko handle those?
08:07emilioheycam: (or we for that matter)
08:07heycamemilio: I&#39;m not even sure that google docs uses contenteditable
08:07heycamI think it does it all itself
08:11emilioheycam: yeah, looks like it, huh
08:11emilioheycam: Did you verify the recascade thing? does the style count skyrocket?
08:11heycamemilio: anyway it would be interesting to know why we can&#39;t get more sharing out of so straightforwardly identical markup like this
08:12heycamemilio: I tried that on the google docs page and didn&#39;t actually see it increase... haven&#39;t tested beyond that
08:12emilioheycam: huh
08:13heycam(specifically I pressed cmd+plus)
08:13heycamI&#39;ll try writing a simple page later that should be super-shared, and see what hapepns
08:13emilioheycam: I suspect we&#39;re not sharing because those style attributes are set via CSSOM
08:13emilioheycam: and thus we have already a unique declaration block
08:13heycamemilio: ah good theory
08:14heycamemilio: I wonder if we can look up the style attribute cache after CSSOM modifications without a perf hit
08:14heycamprobably not
08:14heycam:)
08:14emilioheycam: (before it was actually the same, I mean)
08:14emilioheycam: right, tricky :)
08:14emilioheycam: (as usual :P)
08:16emilioheycam: So to test sharing, probably gdocs isn&#39;t the greatest... Doesn&#39;t seem like much sharing going on in there
08:16heycamemilio: yeah. I&#39;ll just write up what I&#39;m doing now and move on to some other site
08:17emilioheycam: do you plan to share your notes / whatever somewhere?
08:17* emilio would be interested to read
08:19heycamemilio: ok
08:20emilioheycam: I guess I&#39;ll go debug the lone addons XBL test failing with my patches in bug 1397983 :(
08:20firebothttps://bugzil.la/1397983 NEW, emilio@crisal.io stylo: Assertion failure: !mServoRestyleRoot || mServoRestyleRoot == aRoot || nsContentUtils::Conten
09:32SimonSapinemilio: r? https://github.com/servo/servo/pull/18420
09:32crowbotPR #18420: Get rustc commit hash from channel manifest - https://github.com/servo/servo/pull/18420
09:33emilioSimonSapin: which commits need review?
09:33SimonSapinemilio: first and last
09:33SimonSapintheyre all small
09:35emilioSimonSapin: cool, looking
09:35emilioSimonSapin: glad the un-inlining worked :)
09:37SimonSapinemilio: good hunch on this one, I would have looked through dozens of thread_local!() uses
09:38emilioSimonSapin: which SNI?
09:38emilioSimonSapin: I know our hacks way too much :/
09:38SimonSapinemilio: TLS server name indication
09:38emilioSimonSapin: err, s/which/what is/
09:39emilioSimonSapin: ok, looks good, feel free to address my comment or not, as you wish :)
09:39SimonSapinclient tells server what domain in wants, server can pick a different cert based on that
09:40SimonSapinold Python versions dont support it
09:40SimonSapinso we get URLError: <urlopen error [Errno 1] _ssl.c:510: error:14077410:SSL routines:SSL23_GET_SERVER_HELLO:sslv3 alert handshake failure>
09:42SimonSapinemilio: done
09:43emilioSimonSapin: Oh, yeah, I saw the backlog from yesterday
09:44SimonSapinemilio: anything else for #18420 ?
09:44crowbotPR #18420: Get rustc commit hash from channel manifest - https://github.com/servo/servo/pull/18420
09:44emilioSimonSapin: nope, land away!
09:45SimonSapinthanks :)
09:45* SimonSapin => office
10:09emilioheycam++, thanks for the numbers :)
10:09heycam:-)
10:56emiliowcpan: ping?
10:59wcpanemilio: yes?
11:00emiliowcpan: should I review #18457? You didn&#39;t flag anybody for review
11:00crowbotPR #18457: Invalidation should also check animation flags. - https://github.com/servo/servo/pull/18457
11:01emiliowcpan: also, quick question re. bug 1363805, does it optimize the general case of calling `getComputedStyle(foo).prop`? or just accesses where you already have a `nsComputedDOMStyle` object?
11:01firebothttps://bugzil.la/1363805 ASSIGNED, wpan@mozilla.com Only flush styles on nsComputedDOMStyle::UpdateCurrentStyleSources if we know that the child or one
11:01wcpanemilio: ah, right, I forgot to r? thank you!
11:01wcpanemilio: it should optimize the general case now
11:02emiliowcpan: it&#39;d be nice to test it :)
11:02emiliowcpan: So, another question
11:02emiliowcpan: So you&#39;re checking individual hints in #18457
11:02crowbotPR #18457: Invalidation should also check animation flags. - https://github.com/servo/servo/pull/18457
11:03emiliowcpan: Why don&#39;t we need to check the `RESTYLE_DESCENDANTS` or `RECASCADE_DESCENDANTS` hints?
11:03emiliowcpan: that looks bogus to me
11:04wcpanemilio: dirty descendants may happens for a different subtree
11:04emiliowcpan: in particular, should we just change it to `!data.restyle.hint.is_empty()`?
11:04emiliowcpan: I&#39;m not talking about the dirty descendant bit
11:04emiliowcpan: I&#39;m talking about descendant restyle hints
11:04wcpanemilio: ah ok
11:06emiliowcpan: Also, I think the patch that just landed means that you need to change `data.restyle.hint` for `data.hint`
11:06emiliowcpan: oh, I lie, that&#39;s another patch that hasn&#39;t landed yet
11:07wcpanemilio: so does that mean we should be able to just check the restyle root&#39;s restyle hint is empty or not, yes?
11:08wcpans/yes//
11:09wcpanemilio: or we don&#39;t propagate all the way to the root?
11:10emiliowcpan: no
11:10emiliowcpan: restyle hints aren&#39;t necessarily propagated
11:10emiliowcpan: otherwise we&#39;d do a lot of work all the time
11:13emilioSimonSapin: yay! ^ :)
11:16emiliowcpan: need to go for lunch, but feel free to drop any question here and I can try to answer afterwards.
11:16wcpanemilio: thx!
11:48travis-ciServo failed to build with Rust nightly: https://travis-ci.org/servo/servo-with-rust-nightly/builds/274556830 CC nox, SimonSapin, jntrnr
12:40travis-ciServo failed to build with Rust nightly: https://travis-ci.org/servo/servo-with-rust-nightly/builds/274556830 CC nox, SimonSapin, jntrnr
12:54njnLookAndFeel_ColorID is defined in structs_{debug,release}.rs. I want it to derive(MallocSizeOf), but I can&#39;t work out how to attain that
13:01jdmemilio: is there any way to have arbitrary derives on some types in bindgen?
13:01* jdm suspects the answer is no
13:01emiliojdm: hmm, there used to, not sure if that was removed
13:01emiliojdm: what do you need this for? You can always impl manually ofc but...
13:02jdmsee previous from njn
13:03emiliojdm: oh, I see
13:04njnjdm: BTW, I found a minor problem with HeapSizeOf
13:04njnhttps://github.com/servo/heapsize/blob/master/src/lib.rs#L29
13:04njnthat&#39;s the is_empty check, which looks at the alignment
13:04njnjdm: but look at this: https://github.com/servo/heapsize/blob/master/src/lib.rs#L90
13:04njnfor box we cast to `*const c_void`
13:04njnwhich is necessary for the ?Sized to work
13:05njnbut means the alignment check is not quite right
13:05jdmhuh
13:06jdmI wonder if we could add the ?Sized as a bound to the heap_size_of function instead
13:06njnjdm: I tried that, you end up propagating it to the align_of::<T> call, which doesn&#39;t like it
13:06jdmmmm
13:06njnjdm: I wonder if making the empty test just check for < 16 might suffice
13:07njnwell, <= 16
13:07SimonSapinplaybot: Box::new([u32; 0]) as Box<[u32]>
13:07SimonSapinplaybot: Box::new([0_u32; 0]) as Box<[u32]>
13:08SimonSapinplaybot: &*(Box::new([0_u32; 0]) as Box<[u32]>) as *const [u32] as *const () as usize
13:08noxSimonSapin or emilio: r? ^
13:08emiliojdm: njn: I don&#39;t think there&#39;s anything left of that feature
13:09njnemilio: ok, I&#39;ll just define it manually
13:09SimonSapinplaybot: ::std::mem::align_of::<[u32]>()
13:09SimonSapinplaybot: ::std::mem::align_of_val(&[] as [u32])
13:10SimonSapinplaybot: ::std::mem::align_of_val(&[] as &[u32])
13:10noxnjn: What&#39;s the issue with Box<T> and !Sized type?
13:10noxCan&#39;t we just implement HeapSizeOf for Box<T> where T: HeapSizeOf,
13:11SimonSapinnox: the issue is in that impl
13:11noxand then separately implement it for Box<[T]> and Box<str>?
13:11njnnox: Box<[T]> and Box<str>
13:11SimonSapinfor detecting empty allocations
13:11noxYeah, so?
13:11noxSorry I don&#39;t follow.
13:11noxOh.
13:12SimonSapinas *const c_void
13:12noxBut anywa,
13:12SimonSapinhttps://github.com/servo/heapsize/blob/master/src/lib.rs#L90
13:12noxthat shouldn&#39;t even exist.
13:12noxI think I filed an issue about that c_void.
13:12SimonSapinwhat shouldnt?
13:12noxIt causes more issues than it solves them.
13:12noxUgh, I didn&#39;t.
13:12SimonSapinnox: looking at your PR
13:13noxThere are some dubious impls too, like the ones for raw pointers.
13:14SimonSapinnox: why remove ref in DeclaredValue::Value(ref specified_value)?
13:14noxSimonSapin: specified_value is a &T,
13:14SimonSapinok
13:14noxand then we get &&T and trait methods can&#39;t be called.
13:15noxSimonSapin: emilio beat you to it.
13:15SimonSapinyup, saw that
13:16noxSimonSapin: Why is it heap_size_of in the first place that checks alignement?
13:16SimonSapinnox: as opposed ot every caller?
13:16noxWhy every caller?
13:16SimonSapinof that function
13:17noxHow can any arbitrary T pointer be empty?
13:17noxIt can only happen if size_of::<T> is 0, no?
13:17noxAnd Vec and boxed slices.
13:18noxAnd if we put that check in derived impls, it will be optimised out all the time anyway so it shouldn&#39;t be a problem.
13:18SimonSapinnox: and anything else that can use std