mozilla :: #servo

18 May 2017
02:24jntrnris there anyone that can review the PRs for https://github.com/servo/servo/issues/16560 ?
02:24crowbotIssue #16560: Windows nightly uploads are failing - https://github.com/servo/servo/issues/16560
02:24jntrnrthey're currently blocking on larsberg, but he's (I'm guessing) waaaay too busy to be review PRs these days
02:25jntrnrmaybe jack can take a look, or perhaps Manishearth might know of a good person
02:28* gw is scared of saltfs
02:29jntrnrgw: feeling sluggish?
02:29gwjntrnr: huh?
02:30jntrnrgw: bad pun
02:30gwah :)
02:42aneeshusagw: Anything in particular you're scared of in saltfs?
02:47gwaneeshusa: no, I just don't know how it all fits together - I was really meaning the whole CI setup, not just saltfs
02:51aneeshusagw: I can try to add some more docs in the repo. Are you mainly looking to understand how Homu/Buildbot/Travis work together? or something else?
02:53gwaneeshusa: nothing in particular, it was just a flippant comment
02:58* waffles reads the backlog, scrolls through PRs and everything he'd missed over the last week
02:59gwooh, the preserve-3d PR just landed
02:59gw\o/
02:59gwkvark++
03:00waffleshey gw, re #16917, I recall we had a similar tsunami of timeouts a while back when we updated WR, right?
03:00crowbotPR #16917: Update WR (Intel driver workaround, subpixel and other bugfixes). - https://github.com/servo/servo/pull/16917
03:00waffless/recall we had/recall us having
03:02gwwaffles: yea - sometimes it happens when WR has a perf regression, that becomes very apparent when running 10,000 tests. in this case, a PR landed that lets rayon automatically choose the number of worker threads to create. it just occurred to me when preparing the update PR that this probably won't play well with the way we run so many servo processes concurrently on CI. But I figure we can try it and see what happens
03:02gw(the previous code created a fixed number of 4 worker threads)
03:04wafflesgw, ah, interesting
03:04wafflesyep, we can try it :)
03:06wafflesyo aneeshusa, I turned highfive into Github integration :)
03:06wafflesaneeshusa, yeah, I know, I took my time!
03:07wafflesoh, and I'm talking about the thing I'd written from scratch
03:08aneeshusawaffles: https://github.com/servo-automation/highfive that one?
03:08wafflesaneeshusa, yes
03:16aneeshusawaffles: anything noteworthy I should look for while looking through it?
03:41* stshine guess he will copy a webrender api to each SvgFragmentInfo. wtf
03:46wafflesaneeshusa, yes, it's got some new stuff - like assigning issues, tracking them (and pinging assignees), associating PRs, tracking those PRs (and pinging authors or closing them after a timeout)
03:46wafflesaneeshusa, see https://github.com/servo-automation/highfive/blob/master/handlers/issues/easy_assign/__init__.py
03:47wafflesaneeshusa, I'm working on a similar state-maintaining handler which can take care of all PRs in our review queue
03:48wafflesaneeshusa, also, since we're queuing requests, this won't block on rate limits
03:50wafflesso, if you get any ideas for doing more useful/cool stuff, let me know or open an issue
03:51aneeshusawaffles: What do you mean by queuing requests?
03:54wafflesaneeshusa, so, we sleep for a while after making a request :p
03:54wafflesaneeshusa, https://github.com/servo-automation/highfive/blob/master/helpers/runner.py#L104
03:55wafflesthe interval is small, like a few hundred ms, shouln't make much diffference
03:55* waffles cant type while eating (sigh)
03:56aneeshusawaffles: Also, this code https://github.com/servo-automation/highfive/blob/master/handlers/issues/easy_assign/__init__.py#L97 is likely not so helpful anymore,
03:56aneeshusawaffles: given https://groups.google.com/forum/#!topic/mozilla.dev.servo/9jUSv0ZQQE0
03:57wafflesaneeshusa, um, that's for a comment, but yeah, I get your point
03:58aneeshusawaffles: gotcha
03:59* waffles will brb in a few mins
05:16SimonSapinnox: yep, it's notedd
05:16SimonSapinIt's noted in the quirks spec
05:45wafflesnox, what the hell is https://is.gd/yVG6Tk ?
07:08noxSimonSapin: What is?
07:09SimonSapinnox: quirky color vs scientific notation
07:09noxYeah but this isn't scientific notation.
07:28wafflesnox, shouldn't your morning begin with a nice review? :p
08:34noxSimonSapin: Patching bytes, even though we don't need it.
08:34SimonSapin?
08:34SimonSapinla cagette bytes? what patch?
08:36noxSimonSapin: get_mut, into_owned
08:37noxSimonSapin: https://github.com/carllerche/bytes/pull/121
08:37crowbotPR #121: Introduce more bridges between Bytes and BytesMut - https://github.com/carllerche/bytes/pull/121
08:37noxstshine: The unit test failing,
08:37noxstshine: that might be because I run them in release mode.
08:38stshinenox: :/
08:38noxstshine: Well that's good news. :)
08:39noxstshine: Better that than the other direction, and release being bigger. :D
08:39* stshine kudos to rustc
08:39noxSimonSapin: Ugh, forgot the most important part:
08:40noxstshine: https://github.com/carllerche/bytes/pull/121
08:40crowbotPR #121: Introduce more bridges between Bytes and BytesMut - https://github.com/carllerche/bytes/pull/121
08:40nox100% better.
08:42stshinenox: you are going to use that in layout?
08:42noxstshine: Use what? Bytes?
08:42stshineyep
08:42noxNo this is just a patch that I discussed about with SimonSapin,
08:42noxfor h5e and friends.
08:42noxstshine: But he did zbuf meanwhile.
08:43stshinewell still sounds good
08:46SimonSapinnox: since youre patching projects you dont use, do you have time for a review? ;) https://github.com/servo/servo/pull/16915
08:46crowbotPR #16915: Shrink selectors::Component, implement attr selector (in)case-sensitivity - https://github.com/servo/servo/pull/16915
08:46noxSimonSapin: Am taking my breakfast while waiting on m-c to finish compiling...
08:47SimonSapinwell, not necessarily right now
08:47noxSimonSapin: Do I need to read the tickets?
08:47SimonSapinnox: my commit messages should be enough
08:48noxSimonSapin: Think you could kill that boolean in second commit?
08:48SimonSapinnox: its in FFI
08:48noxOk.
08:49SimonSapinnox: a next commit brings an enum right up to the FFI call
08:51noxSimonSapin: Not that I care, but you indent your where clauses the wrong way, neither visual nor block.
08:57noxSimonSapin: phf still doesn't offer a way to build maps with proc macros?
09:06noxSimonSapin: r? ^
09:11sewardjbholley: ping
09:11bholleysewardj: hi
09:11sewardjbholley: moin! (1) you pinged last night. (2) Am hacking Rayon now .. I have a question
09:12bholleysewardj: my ping was just about your rayon issues, so let's jump straight to 2
09:13sewardjbholley: ok. Question was only that any change to my modified Rayon causes ~10 mins of compiling .. any way to avoid that?
09:13bholleysewardj: not really :-(
09:13bholleysewardj: opt builds are slow, servo requires recompiling the entire DAG
09:13bholleysewardj: you can make it a lot faster with a debug build
09:13bholleysewardj: but if you're profiling that isn't too helpful
09:14sewardjbholley: :-(. ok
09:14bholleysewardj: I assume that your round-trip time to compilation failures is fast?
09:14bholleysewardj: you can fix that with cargo check if it's an issue
09:14bholleysewardj: but I'm assuming it's mostly just waiting for codegen after it's clear your code is going to compile
09:15sewardjbholley: RTT to fail is no problem
09:15sewardjbholley: I am trying to understand how rayon's work stealing algorithm interacts with what stylo throws at it
09:15bholleysewardj: a very good thing to understand!
09:16sewardjbholley: rayon seems very complex in this area and I'm not sure why. I need to talk w/ Nico
09:16sewardjbholley: in the meantime: for (eg) the albumart test case, is it possible that in fact stylo is not generating a lot of parallel work?
09:16bholleysewardj: (s/nico/niko/)
09:17sewardjoh, this k/c thing always confuses me
09:17bholleysewardj: it's possible - let me look at the shape of the dom
09:17sewardjbholley: rayon 0.7 has a bunch of spinloopery that the worker threads wind up in before going to sleep
09:18sewardjand I am wondering if that is actually profitable here, or not.
09:20bholleysewardj: so looking at the profile, it does seem like there will be a lot of 1-element work items
09:21sewardjbholley: ah, but that's not the question!
09:21bholleysewardj: because there's a lot of russian-doll stuff with lots of dom elements wrapped around each other
09:21bholleysewardj: but there should definitely be plenty of opportunities for parallelism in this dom
09:21sewardjbholley: ok
09:21sewardjbholley: so (for example) you expect that you would be able to saturate 4 threads most of the time?
09:22bholleysewardj: uh, that is very hard to estimate
09:22bholleysewardj: it would be great if we could visualize what it's doing somehow
09:22sewardjbholley: does the russian-dolling cause sequentialisation in those parts of the tree?
09:22bholleysewardj: no
09:23sewardjI assume this means there are chains of nodes in the tree that just have one child
09:23bholleysewardj: the basic model is that each element gets processed and then its children are appended, as a single work unit, to the queue
09:23bholleysewardj: right
09:23sewardjright, so the chains themselves are (locally) sequential
09:23bholleysewardj: sure
09:24bholleysewardj: until a branch is discovered
09:24sewardjand only if we have multiple chains on the go, do we get parallelism
09:24bholleysewardj: right. But the algorithm should cause us to discover lots of chains quickly
09:24bholleysewardj: because it's breadth-first
09:24bholleysewardj: so even if deep down in some subtree there's a long chain, that shouldn't prevent other threads from working on other parts of the tree
09:25sewardjbholley: yes, I see
09:25bholleysewardj: can we record the number of threads with nothing to do as a function of time?
09:26sewardjbholley: yeah, i was wondering that actually
09:26bholleysewardj: my intuition is still that we should be saturating quickly
09:26bholleysewardj: if we're not, it would be good to dig into exactly what shape causes that
09:30sewardjbholley: yes, I agree
09:30* sewardj continues digging
09:50SimonSapinnox: it could use procedural masquerade I suppose
09:50SimonSapinphf
09:56wafflesnox, should I really store Calc there? :/
09:57wafflesI could parse it with calc
09:57noxwaffles: I don't understand the question.
09:57* avadacatavra thinks about http://searchfox.org/mozilla-central/source/js/src/proxy/Proxy.cpp#80
09:57noxIt's a Number, so you use Number.
09:57noxwaffles: Number stores no calc.
09:57noxwaffles: It stores whether it was calc or not for serialisation.
09:57noxwaffles: Please check that other productions are not incorrect in the same way.
09:58wafflesnox, okay, just Number then (shouldn't be much)
09:58noxwaffles: It's CSSFloat + bool.
09:58wafflesnox, also, see bug 1350069
09:58firebothttps://bugzil.la/1350069 ASSIGNED, mats@mozilla.com [css-grid] calc() function doesn't work inside repeat() function and grid line/span
09:59noxwaffles: Hah.
10:22tedCache hits 279
10:22tedCache misses 1
10:22tedwell *that's* better
10:23tedstill spent a bunch of time compiling mozjs-sys and osmesa
10:30noxWhat is this:
10:30nox /// Set to a pair list value
10:30nox ///
10:30nox /// This is only supported on the main thread.
10:30noxWhy is it allowed only on the main thread,
10:30noxwhy does it seem to not check whether it's on main thread,
10:30noxand if it indeed does not check, why isn't it unsafe?
10:31noxemilio: Do you know? ^
10:32SimonSapinnox: r? two new commits https://github.com/servo/servo/pull/16915
10:32crowbotPR #16915: Shrink selectors::Component, implement attr selector (in)case-sensitivity - https://github.com/servo/servo/pull/16915
10:33noxSimonSapin: Push, maybe?
10:33SimonSapinnox: nsCSSValue has not-thread-safe refcounting
10:33wafflesnox, I just realized that Number is a CSSFloat
10:33noxSimonSapin: But is this checked?
10:33noxwaffles: + bool
10:34wafflesnox, yes, but FLOAT!
10:34noxSo?
10:34waffleswe need Integer
10:34SimonSapinnox: pushes better with -f
10:34noxwaffles: Where is the grammar for this spec again?
10:34wafflesnox, you can't repeat(2.4, auto)
10:34wafflesrepeat(2, auto) computes to "auto auto"
10:34SimonSapinnox: it is checked by carefully adding assertions that were only doing refcounting-free operations
10:34wafflesnox, https://drafts.csswg.org/css-grid/#typedef-track-repeat
10:34noxSimonSapin: I don't understand.
10:35SimonSapinnox: things like https://github.com/servo/servo/blob/master/components/style/gecko_bindings/sugar/ns_css_value.rs#L210
10:35SimonSapindebug_assert_eq!(self.mUnit, nsCSSUnit::eCSSUnit_Null);
10:36noxwaffles: Oh.
10:36noxwaffles: TIL <integer> isn&#39;t part of these things that accept calc().
10:36noxwaffles: r=me
10:36wafflesnox, integer accepts calc though :/
10:36noxwaffles: Where?
10:36nox&quot;It can be used wherever <length>, <frequency>, <angle>, <time>, <percentage>, <number>, or <integer> values are allowed.&quot;
10:36noxOh right there.
10:36noxwaffles: I can&#39;t read.
10:36SimonSapinnox: if the previous unit was not null, it might be a refcounted thing
10:36waffles:D
10:37noxwaffles: Well, you have earned the right to implement that correctly.
10:37SimonSapinand decrementing the refcount would be racy
10:37noxSimonSapin: What I don&#39;t get is,
10:37SimonSapinnox: yes, theres lot of unsafety around this
10:37noxhow does set_pair_list check that it is indeed called on main thread.
10:37SimonSapinit doesnt
10:37sewardjbholley: do you have try push syntax that will get my talos numbers relatively quickly?
10:37sewardjs/my talos/me talos
10:38SimonSapinnox: ah, this one is different
10:38SimonSapinnox: maybe it should be unsafe fn
10:38noxYeah that&#39;s what I meant.
10:38tedneeds more work, but that&#39;s a lot better: Build Completed in 0:03:49
10:38noxted: With what?
10:39SimonSapinnox: pushed https://github.com/servo/servo/pull/16915
10:39crowbotPR #16915: Shrink selectors::Component, implement attr selector (in)case-sensitivity - https://github.com/servo/servo/pull/16915
10:39tednox: that&#39;s a clobber build of servo with sccache on my linux machine
10:39tedwith a full cache
10:39noxAh.
10:39tedhttps://irccloud.mozilla.com/pastebin/nOB34Sm9/
10:39tedit&#39;s still spending some time doing work because of things that sccache doesn&#39;t know how to handle yet
10:39SimonSapinted: whats non-cacheable?
10:39tedall those &quot;Non-cacheable calls&quot;
10:39bholleysewardj: sure
10:40tedSimonSapin: for rust compilation, anything with -l, for sure
10:40bholleysewardj: try -b o -p linux64-stylo -u none -t all
10:40bholleysewardj: I recommend retriggering the tp jobs about 6 times as soon as they start running
10:40bholleysewardj: (you don&#39;t need to wait for them to finish to retrigger, you can just do it when they start)
10:40sewardjbholley: can I add &quot;--rebuild-talos 5&quot; for more iterations?
10:41bholleysewardj: yes, I think that works, though there was some problem with it recently
10:41sewardjbholley: ok. thx
10:41bholleysewardj: certainly give it a try, it&#39;s marginally easier than clicking the retrigger button 5 times instead
10:41tedSimonSapin: some of those are C++ compilations, i&#39;m using sccache-as-ccache as well
10:41tedthere are various quirky things that sccache doesn&#39;t handle for C++
10:41tedmight be some things in the servo build that i could fix or workaround
10:42tedin any event, i think things are working well enough that it&#39;s worthwhile to look at enabling sccache in servo CI
10:42tedgotta track down what those last 2 cache misses are...
10:42SimonSapinted: sounds amazing :)
10:43SimonSapinted: whats the total time like, with a full cache?
10:43bholleysewardj: though I suppose that&#39;s a bit wasteful if you only care about tp
10:43tedSimonSapin: &quot;Build Completed in 0:03:49&quot;
10:43bholleysewardj: because it will rebuild _all_ the jobs
10:43bholleysewardj: and talos jobs are more expensive because they run on real hardware
10:43tedSimonSapin: several-year-old Core i7: Intel(R) Core(TM) i7-3770 CPU @ 3.40GHz
10:43ted32GB, SSD
10:44sewardjbholley: hmm yes good point
10:45bholleysewardj: note that talos has a lot of other things that add noise to our signal, so I wouldn&#39;t primarily focus on that, but it&#39;s good as side-verification
10:47noxSimonSapin: Should I r+ or you want to squash stuff?
10:49sewardjbholley: well, I am trying to extract a signal from noise like this: https://pastebin.mozilla.org/9022074 and so was hoping that Talos would help
10:50bholleysewardj: do those numbers mean that your local measurements show no significant change from twiddling the rayon params?
10:50SimonSapinnox: already squashed what I wanted
10:50noxSimonSapin: r=me then.
10:52sewardjbholley: well, that&#39;s what I am trying to figure out. Is a transition from 1.915/1.917/1.871 to 1.918/1.886/1.883 significant or not?
10:52noxSimonSapin: &quot;let mut raw = &mut [0u8; 4] as &mut [_];&quot;
10:52noxMUT
10:52sewardjanswer is, I have no idea
10:53bholleysewardj: does this help us put a cause behind your numbers showing lots of burned cycles?
10:53bholleysewardj: that&#39;s the part that worries me the most
10:53noxSimonSapin: R
10:53SimonSapinnox: [..] instead of as should work
10:53noxSimonSapin: r+ my PR?
10:53noxSimonSapin: Yes but no.
10:53noxSimonSapin: Oh yeah for the cast.
10:54noxSimonSapin: I tried let mut raw = [0u8; 4],
10:54noxand then calling raw[..].write_u32,
10:54sewardjbholley: I think this is a possible explaination, but that&#39;s all I can reasonably say.
10:54SimonSapinnox: omw to the office rn
10:54noxbut like that it complains that raw[..] is [u8].
10:54sewardjbholley: I need to (1) talk with Niko and (2) try this on Talos
10:54SimonSapinand lunch
10:54bholleysewardj: what does it do locally on myspace?
10:55sewardjbholley: those pastebin numbers are exactly from myspace, locally, quiet machine
10:57wafflesbholley, might need a coordinated landing
10:57wafflesfor #16067
10:57crowbotPR #16067: Stylo: Add support for grid-template-{rows,columns} - https://github.com/servo/servo/pull/16067
10:57noxwaffles: Sorry for the delay, good work.
10:58noxwaffles: Ah you didn&#39;t support calc()?
10:58wafflesnox, it does support calc
10:58noxOh yes you did.
10:58noxNice.
10:58wafflesnox, and, no problem, you were courage enough to review it :p
10:58noxwaffles: Did you store an integer,
10:58noxwaffles: or something like Number?
10:58wafflesnox, Integer
10:58noxwaffles: AFAIK, serialisation needs to preserve calc(),
10:58waffles(which has the bool mechanism just like Number)
10:58noxPerfect!
10:59wafflesbut, calc(2 + 2) will be serialized to calc(4)
10:59noxwaffles: I commented on the Gecko bug that says it&#39;s unsupported.
10:59noxwaffles: So I expect some test failures when this lands.
10:59wafflesnox, do you think gecko has tests for calc?
10:59waffles(for grid, I mean)
11:00noxwaffles: Well Gecko managed to bother me landing calc() in -webkit-gradient.
11:00noxwaffles: So I hope it has some for grid, which sounds more important to me.
11:02noxwaffles: Welp, turns out it doesn&#39;t.
11:02noxwaffles: If it&#39;s not tested, does it even exist? :thinking_face:
11:03wafflesnox, I guessed :p
11:04* waffles will brb
11:06bholleywaffles: lmk if you need me to push something
11:09noxHow can I check that the pref introduced in https://bugzilla.mozilla.org/show_bug.cgi?id=1237720 is still relevant?
11:09firebotBug 1237720 FIXED, dholbert@mozilla.com Give &quot;-webkit-min-device-pixel-ratio&quot;/&quot;-webkit-max-device-pixel-ratio&quot; media query its own pref, & d
11:09noxShould I flip the pref, go on GDocs and see if it&#39;s usable?
11:14noxWelp, still broken, bummer.
11:15noxstshine: r+?
11:16stshinenox: feel free, I don&#39;t have review privileges
11:16noxferjm: Isn&#39;t that what he just reviewed on Bugzilla?
11:17ferjmnox: yes, but I don&#39;t have permissions to r=someone
11:17wafflesbholley, #16067 depends on bug 1363664 and should be landed at the same time
11:17crowbotPR #16067: Stylo: Add support for grid-template-{rows,columns} - https://github.com/servo/servo/pull/16067
11:17ferjmnox, btw thanks for the define_css_keyword_enum! tip :)
11:17firebothttps://bugzil.la/1363664 REOPENED, wafflespeanut@gmail.com Stylo: Add bindings for Servo-side setting of nsStyleGridTemplate
11:17noxferjm: I hate you. I am going to open a PR for font-feature-settings descriptor,
11:17noxand I looked at your diff,
11:17wafflesbholley, also, it might have a lot of &quot;UNEXPECTED PASS&quot;es
11:18noxand all I see is merge conflicts. :P
11:19noxferjm: Oh.
11:19noxferjm: Wait a sec.
11:19noxferjm: add_impls_for_keyword_enum
11:19noxferjm: This does Parse for you.
11:20ferjmnox, oh, really? It didn&#39;t seem so. Let me see again
11:20wafflesit does
11:21noxferjm: Cf. https://github.com/servo/servo/blob/master/components/style/values/specified/position.rs#L41-L51
11:23ferjmah, nice
11:24bholleywaffles: you should prepare the patch against m-c (you can double-check your expectation adjustments with a try run), and then ping somebody when you want them to land it
11:31SimonSapinemilio: r? on the last commit https://github.com/servo/servo/pull/16915
11:31crowbotPR #16915: Shrink selectors::Component, implement attr selector (in)case-sensitivity - https://github.com/servo/servo/pull/16915
11:38noxferjm: Any clue what could be wrong with https://github.com/servo/servo/commit/7b51b72e02c7bb7495a0fbf16cd80e159dbd72fb?
11:38noxOr emilio. ^
11:38nox644 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_font_face_parser.html | @font-face { font-feature-settings: &quot;dlig&quot;; } rule text - got &quot;@font-face { font-feature-settings: ; }&quot;, expected &quot;@font-face { font-feature-settings: \&quot;dlig\&quot;; }&quot;
11:43wafflesis homu stuck on #16897 ?
11:43crowbotPR #16897: Use boolean instead of float to avoid nightly warning - https://github.com/servo/servo/pull/16897
11:44noxemilio: The thread unsafety of nsCSSValue::set_pair_list scares me.
11:44wafflesI don&#39;t think we landed anything after that
11:45noxYou broke it.
11:46wafflesoh nice!
11:47wafflesso, closing PR doesn&#39;t actually stop its build
11:47wafflesshould&#39;ve r- force&#39;d it
11:47noxIn general r- doesn&#39;t work when already started.
11:47SimonSapinbholley: are there plans to use WeakSlice? Looks like its not used atm
11:48wafflesnox, so, it&#39;s only to stop it from landing
11:48SimonSapinbholley: also ArcSlice::slice_to (through ComplexSelector::slice_from) is the only slice* method thats used, so we could make ArcSlice (and so Selector) smaller by one word
11:48noxIsn&#39;t the box to let other people edit your PRs supposed to be checked by default?
11:49bholleySimonSapin: we can remove it, I was just keeping consistency with https://github.com/huonw/shared_slice
11:49wafflesnox, yes, but Github has a bug (I think I emailed about it to them a while back)
11:49bholleySimonSapin: no strong preference
11:49wafflesnox, even though it&#39;s checked when we open a PR, it remains unchecked
11:49SimonSapinbholley: WeakSlice doesnt make ArcSlice bigger so its ok to keep it
11:50bholleySimonSapin: (though it does prevent us from switching this stuff to StyleArc)
11:50noxwaffles: What do you mean?
11:50bholleySimonSapin: which would make it smaller
11:50SimonSapinbholley: StyleArc doesnt do slicing, does it?
11:50bholleySimonSapin: not sure what you mean
11:51wafflesnox, the checkbox is checked by default (when we open a PR), but once it&#39;s opened, it appears to be disabled
11:51noxAnyway, time to go buy lunch.
11:51bholleySimonSapin: ArcSlice does slicing on top of Arc
11:51SimonSapinbholley: the reason for ArcSlice to exist is that it supports slicing without borrowing, right?
11:51noxwaffles: Did you report that?
11:51wafflesnox, yep
11:51bholleySimonSapin: we could make it work on top of StyleArc instead of Arc
11:51noxWhere? Did you get a reply?
11:51SimonSapinah I see
11:51bholleySimonSapin: though might not matter much
11:51wafflesnox, long back, about 2 months back, I think
11:51wafflesnox, that&#39;s the reason for #16102
11:51crowbotPR #16102: Request users to allow edits from maintainers in PRs - https://github.com/servo/servo/pull/16102
11:52noxBut where did you report it?
11:52wafflesnox, mail
11:52noxsupport@gh?
11:52wafflesyep
11:52SimonSapinbholley: probably not, since were not using get_mut and friends with selectors
11:52wafflesnox, and I got a reply that they can&#39;t reproduce it
11:52wafflesnox, I sent the steps again, and got the same reply again
11:52wafflesso, left it
11:53noxOk.
11:53noxwaffles: I&#39;ll try to think about it next time I create a PR, see what happens,
11:53noxand if I can reproduce it I&#39;ll not leave them alone until it gets fixed.
11:53wafflesnox, hah! :D
11:58noxManishearth: I still believe that making FFI functions take Rust references is a big huge mistake btw,
11:58noxI looked at Gecko&#39;s code,
11:58noxand in many many places pointers are checked for null,
11:58noxand it MOZ_ASSERTs out if they are null,
11:59noxmaking glue Rust functions take &T prohibit us entirely from doing such checks,
11:59noxsure you can say &quot;even if you check for null, you can&#39;t check that 0xdeadbeef is an invalid address&quot;,
11:59noxbut that doesn&#39;t mean we shouldn&#39;t check for null.
12:00emilionox: not off-hand... though I&#39;d look into how mFontFeatureSettings is stored
12:00noxemilio: From the parsing code I guess?
12:01emilionox: sure, I mean the format vs. the format that gecko expects
12:01emilionox: do you have a to_ns_css_value method or something like that?
12:01noxhttp://searchfox.org/mozilla-central/source/layout/style/nsCSSParser.cpp#15131-15186
12:01noxemilio: I&#39;m pretty sure I&#39;m following the same format, aren&#39;t I?
12:02emilionox: oh, shitty internet, only saw the first file of the commit ;_;
12:02noxemilio: Hah.
12:08wafflesemilio, is there a clean way to update gecko&#39;s test expectations? there&#39;s a lot of &#39;em! :/
12:09cynicaldevilanyone here used ripgrep?
12:09cynicaldevilIs it faster than the Unix grep?
12:10wafflescynicaldevil, apparently, yeah
12:10cynicaldevilwaffles: good, I&#39;ll install it
12:15tillajeffrey: exciting! \o/
12:15emiliowaffles: nope
12:15wafflesvery encouraging
12:16emiliowaffles: sorry :/
12:16emiliowaffles: I can update them after your grid stuff lands if you want
12:16wafflesemilio, no, thanks, I&#39;ll try :)
12:16wafflesI thought you might know some hack :D
12:17wafflesI&#39;m just copy-pasting the log and regex-replacing stuff
12:20cynicaldevilwow ripgrep is FAST
12:20jgrahamyes
12:22noxemilio: GECKO(2232) | Assertion failure: aString.Length() == 4 && NS_IsAscii(aString.BeginReading()) && isprint(aString[0]) && isprint(aString[1]) && isprint(aString[2]) && isprint(aString[3]), at /Users/nox/src/gecko/layout/style/nsRuleNode.cpp:4080
12:34SimonSapinemilio: does this visitor look right?
12:34SimonSapinhttps://github.com/servo/servo/pull/16915/commits/029bbe00a407329af58b124ab1c6f1469339afed
12:34emilioSimonSapin: yes, that looks fine to me :)
12:35SimonSapincool
12:45noxSimonSapin: Holy hell,
12:45noxSimonSapin: the sadness of my code.
12:46noxSimonSapin: let mut raw = &mut [0u8; 4][..]; raw.write_u32::<BigEndian>(entry.tag).unwrap();
12:46noxstshine: raw is &mut [] afterwards, quite obviously in hindsight...
12:46noxErr, SimonSapin*
12:48SimonSapinplaybot: unsafe { std::mem::transmute::<u32, [u8; 4]>(0x100_u32.to_be()) }
12:49noxNo thanks.
12:49noxlet mut raw = [0u8; 4]; WriteBytesExt::write_u32::<BigEndian>(&mut raw, entry.tag).unwrap();
12:50noxI could have done (&mut raw).write_u32 but that&#39;s even uglier IMO.
12:50wafflesnox, you know where the expectations for mochitests are?
12:50noxwaffles: stylo-failures.md
12:50wafflesok\
12:54wafflesomg
12:54waffles> failure pattern `grid` in this test - expected 548 failures but got 580
12:54wafflesnox ^
12:54noxwaffles: LOL
12:54wafflesand
12:54waffles> failure pattern `&#39;grid` in this test - expected 637 failures but got 853
12:54noxYou didn&#39;t do a geckotry build first?
12:54wafflesnox, I did it last week, but not after the Integer thing
12:55* waffles wonders whether this has to do with Integer
12:56noxDamn it Homu...
12:56noxwaffles: Please force push to your branch.
12:56noxwaffles: So that Homu doesn&#39;t land it.
12:56wafflesnox, because of the failures?
12:57noxwaffles: Yes.
12:57wafflesnox, the other tests still pass (only these 2 throw errors)
12:57noxAh.
12:57noxDo as you wish then.
12:57wafflesnox, see https://treeherder.mozilla.org/#/jobs?repo=try&revision=ec15b198519cf3cf8f233f9ff8297d3c5a39e1ed&selectedJob=100092297
12:59noxSimonSapin: WriteBytesExt::write_u32::<BigEndian>(&mut &mut raw[..], entry.tag) ( ._.)
13:00SimonSapinnox: embrace the (&mut raw).method()
13:00noxSimonSapin: I&#39;m not sure that even works.
13:00noxOh I guess this should trigger auto-deref yeah.
13:01noxauto-ref*
13:01SimonSapinauto-ref
13:02froydnjxidorn: thanks for taking over the build_gecko.rs bug!
13:02jdmnox: https://github.com/servo/heapsize/pull/81 when you have time?
13:02crowbotPR #81: Accommodate change of empty vector representation in nightly libstd. - https://github.com/servo/heapsize/pull/81
13:02xidornfroydnj: np :)
13:03noxjdm: What&#39;s align_of::<()>() now?
13:03jdmplaybot: ::std::mem::align_of::<()>()
13:04noxThis patch worries me.
13:04SimonSapin::<()>()
13:05SimonSapinneeds some more punctuation
13:06noxOh well.
13:07jdmthat&#39;s the spirit!
13:07SimonSapinplaybot: ::std::mem::align_of::<()..()>()
13:07SimonSapinah its an expression, not a type
13:10SimonSapinbholley: when my PR fixes tests I need someone with access to push the expectation file change to autoland, right?
13:11bholleySimonSapin: yes
13:11bholleySimonSapin: if the expectations are trivial it&#39;s probably fine to not worry too much
13:11bholleySimonSapin: if it&#39;s more complex then you should prepare a patch
13:12SimonSapinbholley: lately Ive been including diffs in github comments
13:12emilioSimonSapin: I&#39;ll probably be around
13:12emilioSimonSapin: so I can push it for you :)
13:12SimonSapinemilio: arent you supposed to be in class? :)
13:12emilioSimonSapin: oh well... :P
13:13noxSimonSapin: I link to a geckotry build and mention that it includes said expectation.
13:13SimonSapinemilio: I think youre the only one with access usually in this time zone
13:13emilioSimonSapin: I&#39;ll push it, no worries :)
13:13SimonSapinnox: mention to who?
13:13SimonSapinemilio: thanks
13:13noxSimonSapin: No one?
13:14noxSimonSapin: s/mention/comment/ if you want.
13:14* SimonSapin pictures nox standing on a cliff shouting into the void where no one can hear
13:14wafflesnox, > invalid value &#39;[default] 40px&#39; not accepted for &#39;grid-template-columns&#39; property - got &quot;[default] 40px&quot;, expected &quot;&quot;
13:14wafflespfft
13:14noxwaffles: ToCss impl broken?
13:15noxAh no.
13:15wafflesnox, the line names
13:15wafflesident
13:15wafflesshouldn&#39;t get css keywords
13:15* waffles likes all gecko&#39;s css tests
13:15jdmSimonSapin: access to what?
13:15SimonSapinjdm: autoland
13:16jdmah
13:16* jdm guesses that&#39;s locked down tighter than l3
13:16jgrahamSheriffs + gps more or less I think
13:17jgrahamAnd emilio apparently
13:17noxwaffles: I love/hate them because they aren&#39;t WPT tests.
13:18noxjgraham: You name it, emilio has it.
13:18emiliojgraham: yeah, mostly stylo people to do test expectation updates
13:18SimonSapinjdm, jgraham: and a dozen people who work on stylo, to fix things up when the tree burns because of automatic sync from servo/servo
13:18noxI think he hoards commit rights. :P
13:18jgrahamOh we&#39;re punching holes that big already are we?
13:19* jgraham tries to looks surprised
13:19noxjgraham: Well, &quot;automatic sync&quot;, it&#39;s written in it.
13:19SimonSapinjgraham: our nice plan
13:19SimonSapin
13:20* jgraham starts to wonder why the automatic sync doesn&#39;t go via inbound if it needs so much human intervention, realises he doesn&#39;t want to know
13:20noxjgraham: I think that was the case previously.
13:20SimonSapinjgraham: our nice plan for fully-automatic sync where you can land changes to both at the same time hasnt materialized, and manual sync became an almost full time job for heycam at some point
13:21jgrahamI see
13:23jgrahamI have been arguing that copying the servo model for wpt doesn&#39;t make sense precisely because we don&#39;t want fixing up issues to be critical blockers for anyone to do work
13:23jgrahamSounds like it&#39;s even worse than I thought
13:23SimonSapinjgraham: for wpt on firefox CI?
13:24jgrahamYeah
13:25jdmcrowbot: tell gw could you make time for #16777?
13:25crowbotyou bet!
13:25crowbotIssue #16777: assertion failed: !needs_clipping || batch.key.blend_mode == BlendMode::Alpha || batch.key.blend_mode == BlendMode::PremultipliedAlpha - https://github.com/servo/servo/issues/16777
13:26noxjgraham: What critical blockers?
13:26SimonSapinjgraham: the current stylo situation is that I land a PR on servo/servo (because thats the only way allowed to get changes into the servo/ subdir of m-c), then it gets automatically synced into the autoland repo, then treeherder is orange because stylo has fewer test failures than previously expected, then someone pushes to autoland to update the expectations file
13:27jgrahamnox: Anything that stops a patch in stylo landing also stops any gecko parts of the patch landing
13:27noxjgraham: Did I misunderstand the &quot;for wpt&quot; part?
13:27jgrahamFor stylo it has ot be like that I guess because you need strong consistency guarantees
13:28jgrahamFor wpt we don&#39;t care so much because merge conflicts are rarer and less painful
13:28jgrahamnox: So, I don&#39;t know what I&#39;m not explaining well :)
13:28SimonSapinjgraham: the previous stylo situation was solving merge conflicts all the time, I hear
13:28noxjgraham: I feel like you are referring to the wpt sync stuff in servo, and that it doesn&#39;t exist in gecko,
13:29noxbut then you mentioned stylo so I&#39;m lost.
13:29jgrahamnox: Gecko and servo independently have batch syning of wpt
13:29waffles> &#39;fit-content(-1px)&#39; not accepted for &#39;grid-auto-rows&#39; property - got &quot;fit-content(-1px)&quot;, expected &quot;&quot;
13:29jgrahamnox: The plan for gecko is to move to more regular syncs (might also work for servo, but let&#39;s ignore that for now)
13:29wafflesI don&#39;t understand
13:30wafflesthe spec says nothing about this
13:30jgrahamnox: One suggested approach was to copy the way that stylo works
13:30noxjgraham: More regular syncs? As in more frequent?
13:30jgrahamnox: Yeah, like &quot;almsot as soon as a PR/patch lands&quot;
13:30noxWhat&#39;s wrong with the current wpt syncing?
13:30noxjgraham: Oh, I see.
13:31jgrahamnox: Lots of manual work updating the expectation data, risk of merge conflicts, missing latest tests, etc.
13:31wafflesSimonSapin, I presume expect_ident allows initial, inherit, etc. ?
13:31bholleysewardj: yt?
13:31sewardjbholley: y
13:31bholleysewardj: so, I wrote a patch that lets you set an env var to force stylo to use a threadpool of size 1
13:32SimonSapinwaffles: yes, its <ident-token> as in CSS Syntax. If you want <custom-ident> as in CSS Values theres another rust type
13:32noxjgraham: You are aware of the servo-vcs-sync thread right?
13:32bholleysewardj: instead of optimizing that case to just using the caller thread
13:32wafflesSimonSapin, oh, we use it anywhere?
13:32SimonSapinwaffles: http://doc.servo.org/style/values/struct.CustomIdent.html
13:32jgrahamnox: Possibly not? Where?
13:32wafflesSimonSapin, awesome! thanks
13:32SimonSapinwaffles: we use it for animation names and counter style names
13:32bholleysewardj: I&#39;ve also been profiling a testcase that just styles a single element again and a gain (traversal of 1 element)
13:32sewardjbholley: How does that help us?
13:32sewardjthe patch I mean
13:32jgrahamnox: (the plan right now is not to follow the servo model)
13:33noxjgraham: https://groups.google.com/a/mozilla.com/d/topic/stylo-team/AGrys2jUeBE/discussion
13:33bholleysewardj: because it lets us separate the overhead of the context switching and any spinning from contention issues
13:33jgrahamnox: Ah, I saw the blog post
13:33bholleysewardj: and lets us make a closer comparison to what single-thread does
13:33noxjgraham: Servo model as in sporadic WPT sync, or automatic stylo sync in autoland?
13:33bholleysewardj: i.e. if context switching overhead were zero, it should perform exactly as well as the sequential traversal
13:34sewardjbholley: ok. Does it show a difference?
13:34bholleysewardj: yes
13:34bholleysewardj: but the weird thing is that the numbers from the main thread and the thread pool don&#39;t add up
13:35sewardjbholley: you mean time numbers from the gecko profiler?
13:35bholleysewardj: yes
13:35bholleysewardj: as in, the gecko profiler shows the worker thread doing much less work
13:36jgrahamnox: Servo model as in &quot;what stylo uses&quot;. The current wpt model is the same in gecko/servo
13:37noxOk.
13:37noxSeems like a better fit for wpt than for stylo, if you ask me.
13:37bholleysewardj: so on my testcase, a sequential traversal spends about 120ms in Servo_TraverseSubtree, a parallel-but-single-threaded traversal spends about 180ms in Servo_TraverseSubtree (waiting on the condvar), and the worker thread claims to spend about 26ms doing actual work
13:38bholleysewardj: so the profiler numbers of the worker thread seem suspect
13:38* sewardj contemplates
13:39sewardjbholley: this seems complex enough to warrant filing a bug
13:40sewardjbholley