mozilla :: #servo

17 May 2017
00:17froydnjManishearth: yes, but I'm surprised it fails to link for you if it's already in automation...
00:18Manishearthfroydnj: automation doesn't run nightly!
00:18Manishearththis is nightly :)
00:18Manishearthfroydnj: there's also a compile error bc of deny(warnings) on nightly
00:19froydnjManishearth: oh, you're compiling with nightly rustc and there are these errors?
00:19Manishearthyes
00:19Manishearthrustc does ask you to -lresolv -- the equivalent code compiles fine for me
00:19froydnjaha, I understand now
00:19Manishearthoutside of firefox
00:19Manishearthi think firefox didn't listen to the -lresolv
00:21froydnjfirefox doesn't pay any attention to what rustc says there
00:21froydnjwonder what added this new requirement
00:21Manishearthfroydnj: oh
00:21Manishearththe getaddrinfo resolv.conf stuff
00:22Manishearthhttps://thesquareplanet.com/blog/the-story-of-a-rust-bug/
00:22Manishearththat's normal
01:04jntrnrlooks like the minimum RAM required to build on Windows went up from 4 gigs?
02:21jntrnron a related note - is there a place to see if the latest nightly build succeeded?
02:39jntrnrtrying to figure out if the perf difference for Windows is specific to me running it in a VM
02:57gwCI says: LLVM ERROR: Invalid data was encountered while parsing the file o.O
02:59jntrnrhrm
03:00jntrnrgw: that sounds not so good
03:01* gw hits retry
03:28bz_awaySimonSapin: did you ever file an issue on the CSS namespaces+selectors specs?
03:54hliebermanIs servo still in a state where it's expected that servo nightly will be relatively unusable at any given download?
03:55hlieberman(I'm unsure whether there's even value in my filing bugs for the stuff that I see as being broken if they're already likely known because the breakage is so wide-spread.)
04:02cbrewsterhlieberman: go ahead and file any issues if you would like, we can close any duplicates
04:02hliebermancbrewster: Sounds good. I want to strike the right balance if I can; too many duplicates, I'm creating more work than providing benefit.
04:05cbrewsterhlieberman: yeah, you can always do a search before filing to see if something similar has been filed, but I wouldn't worry too much.
04:06hliebermanOkie-dokie. Can always move the metaphorical knob later.
04:06cbrewsterIndeed!
05:20waffleshola world!
06:24wafflesnox, YOU BETRAYED ME
07:02heycamemilio: thanks for adding the trace!() lines to spit out each dependency we check, exactly what I need :)
07:04SimonSapinbz_away: not yet because it was night, will do today
07:09emilioheycam: heh, got myself writing the same a few times, so thought I rather leave it in tree
07:35heycamemilio: do you know how I can do the same kind of runtime check that trace!() does to see if trace-level logging is enabled? e.g. if I want to do some computation outside of the trace! statement itself?
07:36emilioheycam: hmm... I'm pretty sure there's a way of doing that, let me check
07:36* heycam wants to add some tracing showing the list of rules that result from match_primary
07:37emilioheycam: https://doc.rust-lang.org/log/log/macro.log_enabled.html
07:37heycamemilio: great, thanks!
07:40emilioheycam: I think the intention of the MaybeDirty stuff is account for bug 1357583
07:40firebothttps://bugzil.la/1357583 NEW, emilio+bugs@crisal.io stylo: Avoid triggering restyle when a stylesheet is added that can't affect the document
07:41emilioheycam: i.e., don't assume that stylesheets have changed == we need a flush
07:41heycamemilio: but what actually checks if we do need the flush?
07:42emilioheycam: the rust code
07:42emilioheycam: well, I guess it would need a flush anyway
07:42emilioheycam: nvm
07:42emilioheycam: { NotDirty, StylesheetsDirty, FullyDirty } sounds fine to me
07:43heycamcool
07:43heycamthanks :)
07:43heycamwell, FullyDirty wasn't one of my suggestions, but I can live with it, with more comments about what it means!
07:44emilioheycam: AllStyleDataDirty is also fine, will use that if you find it cleaner
07:46heycamemilio: better to be more explicit I think
07:46heycamwell
07:46heycamI guess it's not really more explicit
07:46heycamjust longer haha
07:46heycamfine to leave it as FullyDirty
07:52emilioheycam: when you write "When do we want to do this, i.e. clear the non-inheriting style contexts (since they depend on some external data that changed), but not clear the Device on the Servo side, which similarly holds style data?"...
07:52emilioheycam: isn't that what RebuildData does? (well, we recompute it because we can't lazily compute default computed values in parallel, but...)
07:54* heycam looks
07:55heycamemilio: ah, I got confused (again!), and thought Servo_StyleSet_RebuildData didn't clear the Device
07:57heycam(and I think I thought that because I looked at the next function's name, which includes "AndMarkDeviceDirty", and concluded that RebuildData didn't)
07:58emilioheycam: I updated it with your naming suggestions, I'm also not super happy with that function name, so happy to hear other suggestions :)
07:59* heycam looks
08:04heycamemilio: r=me
08:05sewardjheycam: bholley showed me a Talos -derived page which shows Stylo performance over time
08:05sewardjheycam: any idea where that is?
08:05emilioheycam: cool, thanks!
08:05heycamsewardj: oh I've seen that too, but don't know where it is -- maybe shinglyu does?
08:06sewardjheycam: oh well, np, bholley_mobile ^^
08:07emilioheycam: how do theme color changes affect the style data? just curious
08:09heycamemilio: for system colors
08:09heycamemilio: I mean, conceptually the same thing as one of these default computed value dependencies, just might be in some style elsewhere in the tree
08:10emilioheycam: aren't those just a computed value thing? i.e., don't we just preserve the same specified value?
08:10emilioheycam: sure, that means that we need to do a full restyle, but why would a stylesheet flush be needed?
08:11heycamemilio: ugh sorry yeah mixing up stock gecko again. (no stylesheet flush needed there, but throwing away the rule tree is needed)
08:11heycamemilio: (due to cached structs in the rule tree)
08:12emilioheycam: heh, ok, yeah, that makes more sense. I'll avoid mentioning theme changes for now I think
08:12heycamok!
08:35stshineemilio: looking at #16889, do we need to handle the clamping mode in layout side?
08:35crowbotPR #16889: Refactor how calc() clamping is done on computed values (fixes #15296) - https://github.com/servo/servo/pull/16889
08:37emiliostshine: so I thought all the getters clamped, but apparently the fields remain public
08:37emilionox: ^^
08:37emilionox: do you think you could make the fields public and add getters so layout is sane?
08:39stshineemilio: feeling to_computed() is a somewhat confusing name :(
08:43emiliostshine: well, that bit was pre-existing though, right?
08:43stshineemilio: what bit?
08:43emiliostshine: the |fn to_computed(..)|
08:43emiliostshine: I mean, the name is not changing
08:45stshineemilio: I mean the to_computed() method of CalcLengthOrPercentage we added recently :(
08:47stshineemilio: looking at nox's comment in 15296, I think we need to handle it if the container size is negative, but could that happen?
08:59emiliostshine: I don't think so, probably only if it overflows
09:00* stshine agrees
09:03stshineemilio: I can't remember a place in spec where negative container size is required
09:19stshineemilio: oh wait.. I am wrong here. for negative percentage it is possible to computed to a negative absolute length, so we need to clamping it if AllowedLengthType is non-negative
09:20stshineemilio: gah, so we need your getter function anyway :)
09:24sewardjbholley: any way you slice it, I just keep coming back to the theory that there's some kind of wierd spin-wait thing going on in Rayon, which competes for compute resources for "real" work
09:25* sewardj investigates more
09:26bholleysewardj: this is for the 'why only 3x faster' or the 'why tp is slower' question?
09:26sewardjwhy tp is slower
09:26sewardjIIUC this is the more important question
09:27bholleysewardj: that's right
09:27bholleysewardj: it's just weird because when we measure the style traversal it's faster, right?
09:27sewardjbholley: fwiw, I can now reproduce, natively (on real CPU, not valgrind) the slowdowns for the myspace case
09:27bholleysewardj: i.e. have you found a call to Servo_TraverseSubtree that's slower with parallel than with sequential?
09:28sewardjbholley: I'm not looking at that level. There's another wierd but perhaps indicative phenomenon
09:28bholleysewardj: do tell
09:28sewardjwhich is that when running the tests on valgrind/callgrind, with 1,2,3,4 threads, the slowdown is very much more marked
09:29sewardj(5.9 billion insns for 1 thr, 12.4 for 4 thr)
09:29sewardjand when you look at the profiles, the only difference is in rayon_core::registry::Work...
09:29sewardjeverything else looks the same
09:29sewardjSo I wonder really if Rayon is spinning
09:30sewardjbholley: uh .. when the styling is all finished, are we sure that the rayon thread pool is shut down?
09:30bholleysewardj: ah, interesting!
09:30bholleysewardj: that's a good question
09:31bholleysewardj: rayon should be doing zero work outside of Servo_TraverseSubtree
09:31bholleysewardj: if it is, then that would explain what's going on
09:31sewardjlike, if (1) rayon had some kind of spin problem, and (2) it isn't shut down, and just continues to spin
09:31sewardjright
09:31bholleyexactly
09:31sewardjWell, so in my 4 thread case, it burns 7.117 billion insns in rayon_core::registry::Work...
09:31bholleysewardj: which is what would explain Servo_TraverseSubtree being still faster with parallelism, but everything else being slower
09:31sewardjWell, so we need to ask Nico.
09:32sewardjIt should be the case that Rayon is spin-free
09:32sewardjso that even if we don't shut it down, when it runs out of styling work to do, it just falls idle
09:32bholleysewardj: right
09:32bholleysewardj: but if it isn't, that would explain what we're seeing
09:32bholleysewardj: so, some things to measure
09:33bholleysewardj: (1) reproduce my observation that parallel spends less time in Servo_TraverseSubtree than sequential does
09:33bholleysewardj: (2) Can you measure what parts of the timeline the 7.117 billion instructions come at? Or more specifically, how many of them come when the main thread is not doing Servo_TraverseSubtree?
09:33bholleysewardj: (or better, when the main thread is not blocked on the condvar in that function)
09:33sewardjhmm, lemme look at the call graphs
09:34bholleysewardj: (i'm not sure the call graphs would tell us anything about this, FWIW)
09:34bholleyor
09:34bholleywell
09:34sewardjbholley: one thing I didn't mention, but is important, is that (for obscure reasons) valgrind tends to make the effect of any spin loops much worse
09:35bholleyI guess what you'd want is a main-thread callgraph from a sample that is adjacent in time to the sample in ::Work
09:35sewardjwhich might be why we're seeing this.
09:35* sewardj looks
09:36sewardjbholley: so, ok, (2) I can't say
09:37sewardjfor (1), I have 1-thr spending 76M insns in Servo_TraverseSubtree and children, for 4-thr that's only 1.68M
09:37sewardjis that expected?
09:38bholleysewardj: well, in the parallel case Servo_TraverseSubtree just blocks on a condvar and kicks over to the worker threads to do ther work
09:38bholley*do the work
09:39sewardjok
09:39bholleysewardj: so there wouldn't be too many instructions executed there per se
09:39sewardjthere just has to be a spinloop in rayon_core::registry::WorkerThread::wait_until
09:39sewardjnothing else makes sense
09:42noxemilio: I can make them private sure.
09:46bholleysewardj: hm, the gecko profiler does seem to show these threads doing _psynch_cvwait, at least on mac
09:47bholleysewardj: oh interesting
09:47bholleysewardj: on the myspace testcase, we _do_ seem to be significantly slower in Servo_TraverseSubtree with parallelism vs without
09:47bholleysewardj: that doesn't match my recollection...
09:48sewardjbholley: let me try to get the rayon source code annotated by the profiler
09:48sewardjso I have something to show Nico.
09:54noxwaffles: Stuff came up and I couldn't do things this weekend.
09:55SimonSapinbz_away: ah! So I didnt dream this https://www.w3.org/TR/2011/REC-css3-namespace-20110929/#terminology "In CSS Namespaces a namespace name consisting of the empty string is taken to represent the null namespace or lack of a namespace."
09:55crowbotSimonSapin: that's probably not the spec you want. Please read https://github.com/servo/servo/wiki/Relevant-spec-links
09:55SimonSapingo back to sleep, crowbot
09:55SimonSapinbz_away: https://github.com/w3c/csswg-drafts/pull/1387
09:55crowbotPR #1387: [css-namespaces] Restore Terminology section - https://github.com/w3c/csswg-drafts/pull/1387
10:10wafflesnox, ok
10:32bholleysewardj: ok
10:32bholleysewardj: so, I've discovered some things
10:32bholleysewardj: though I guess maybe you're eating lunch right now
10:33sewardjbholley: talking w/ njn right now, back soon
10:34bholleysewardj: k
10:36noxbholley: "So far, we've managed to avoid any intention behavior changes in stylo AFAIK"
10:36bholleys/intention/intentional/
10:36noxWhy is there a "## Need Gecko change" section in stylo-failures.md then?
10:37bholleynox: because presumably we'd be trying to ship those gecko changes in gecko proper sometime before 57
10:38noxI think dholbert just stated the change is easy to do in Gecko proper before 57.
10:38bholleynox: right, in which case we should do it
10:38sewardjbholley: ok. What do you have?
10:39bholleynox: that is why I'm asking dholbert about changing gecko, and not asking you about changing stylo
10:39bholleysewardj: so
10:39bholleysewardj: I bumped up the resolution of my profiler
10:39bholleysewardj: and discovered that we're getting killed on tons of tiny traversals
10:40bholleysewardj: need to verify a bit more
10:40sewardjoh interesting
10:40bholleysewardj: but what appears to be happening is that the "big" traversals are faster with the parallel traversals, but there are ones that are small enough that the overhead is presumably higher than the win
10:40sewardjbholley: can you say how tiny they are, in any meaningful way?
10:40bholleysewardj: like, 1 element
10:41bholleysewardj: I mean, we have callsites that do this intentionally
10:41bholleysewardj: i.e. creating generated ::before content and such
10:41bholleysewardj: this was always a "tune it later" sort of thing
10:41sewardjbholley: ok .. so is it possible to somehow have a threshhold below which it does't even try to create parallel work?
10:41bholleysewardj: because we never had a good sense of the overhead of the parallel traversal
10:41bholleysewardj: exactly
10:41sewardjright .. so "later" is the new "now" :-)
10:41bholleysewardj: theoretically, work stealing is supposed to be good about this
10:42sewardjmaybe it is
10:42bholleysewardj: but there's still the context switch
10:42sewardjbut it just depends on the fixed cost of a work steal vs the average cost of the stolen work
10:42sewardjI think we're agreeing
10:42bholleysewardj: so, we can do various levels of heuristics from the gecko side to say 'do this one in parallel, this one sequentially'
10:43sewardjbholley: is there some easy grain-size adjustment I can try out?
10:43bholleynot really
10:43sewardjJust to see how this affects my numbers
10:43sewardjoh, bummer
10:43bholleysewardj: well, maybe
10:43sewardjbholley: ok .. what about at least, some stats gathering?
10:43bholleysewardj: per se there isn't a way to tell how big a subtree is
10:43sewardjtraversal-size histogram?
10:43sewardjoh
10:43bholleysewardj: though if an element had zero children for example, that would be a good indicator
10:44bholleysewardj: but yes
10:44bholleysewardj: I think it would be good to understand what the threshold is
10:44bholleysewardj: and make sure that threshold makes sense
10:44bholleysewardj: and that it's not something we can make an order of magnitude lower or whatever
10:45noxemilio: Were there expectation changes btw?
10:46sewardjbholley: is there a place in the sources where we can put in a parallel/sequential decision point, based on the tree we're being asked to process?
10:46sewardjand then play around with heuristics at that point?
10:46bholleysewardj: it depends on what you mean by "based on the tree"
10:46bholleysewardj: Servo just gets a subtree
10:47bholleysewardj: so it would have to do some analysis to figure out how big the subtree was
10:47bholleysewardj: or it would have to get a hint from gecko
10:47noxWho should we ask to get servo/servo#PR properly linkified on hg.m.o?
10:48bholleynox: gps
10:48noxbholley: Bugzilla ticket?
10:48sewardjright. but we could do a quick check (for now) based on (eg) is the node I have more than 1 level deep, etc
10:48bholleynox: sure, though I'm not sure what component
10:48sewardjbholley: really I just want to be able to validate your theory somehow
10:48bholleynox: you could also just file in an arbitrary component and then NI him
10:49noxemilio: Do you want to r- that? https://github.com/servo/servo/pull/16889
10:49crowbotPR #16889: Refactor how calc() clamping is done on computed values (fixes #15296) - https://github.com/servo/servo/pull/16889
10:49noxbholley: I see.
10:49bholleysewardj: right
10:49* bholley thinks
10:49bholleysewardj: so, we have the style statistics
10:49bholleysewardj: currently those hide traversals with less than 50 elements because they're too noisy
10:49bholleysewardj: but the data's there
10:50bholleysewardj: so we could at least plot a histogram of the traversal sizes
10:50sewardjbholley: cool. got a source location?
10:50bholleysewardj: and then (the part I'm most interested in) measure the relative costs of the work we do in those traversal vs the context switch
10:51noxmystor: Out of curiosity, does synstructure handle #[cfg] transparently?
10:51bholleysewardj: this stuff: http://searchfox.org/mozilla-central/source/servo/components/style/context.rs#175
10:52sewardjbholley: IIUC, we want to histogram the TraversalStatistics.elements_traversed numbers, yes?
10:52bholleysewardj: well, maybe
10:52bholleysewardj: this is where it gets tricky
10:53bholleysewardj: there's the number of elements traversed, vs the number of elements where we actually did work beyond walking past it
10:53bholleysewardj: which is elements_styled, roughly
10:53bholleysewardj: as a first pass, recording both is probably worthwhie
10:53bholley*worthwhile
10:53bholleysewardj: FWIW you can see all these dumped with DUMP_STYLE_STASTICS=1
10:54sewardjbholley: yes, I've been using that. It only prints once or a few times per page
10:55bholleysewardj: right, that's because of this: http://searchfox.org/mozilla-central/rev/9a7fbdee1d54f99cd548af95b81231d80e5f9ad1/servo/components/style/parallel.rs#89
10:55bholleysewardj: I did that because it was too spammy otherwise :-)
10:56sewardjbholley: let me ask you though .. for the albumart test case
10:56sewardjhow many nodes -- roughly -- do you expect in the tree?
10:56sewardj10k maybe?
10:57sewardjbut not millions
10:57bholleysewardj: less for albumart
10:57bholleysewardj: I can get you a precise number
10:57bholleysewardj: moment
10:57sewardjbholley: no don't bother
10:57bholleysewardj: 1674
10:57sewardjbholley: point is, I'm trying to reconcile this with having burned 7.1 billion insns in rayon's look-for-work machinery
10:58sewardjand failing.
10:58bholleysewardj: I mean, sounds like we should investigate _both_ of these avenues
10:58sewardjbholley: exactly. I was just going to say that.
10:59sewardjbholley: thanks for the pointer. I'll get some lunch and see if Nico is around after lunch.
10:59bholleysewardj: ok great - I'll keep poking at the traversal size thing, though also need to triage some bugs
10:59bholleysewardj: FWIW
11:00bholleysewardj: I just measured
11:00bholleysewardj: for the myspace testcase
11:00bholleysewardj: the "big" traversal is much faster in parallel
11:00bholleysewardj: 12 vs 18ms
11:00sewardjright, ok
11:00bholleysewardj: but then parallel burns another 28 ms in tiny traversals
11:00sewardjok
11:00bholleysewardj: whereas sequential only burs 4
11:00bholley*burns 4
11:01sewardjbholley: are the tiny traversals of the form where, there is 1 tiny traversal to do
11:01bholleysewardj: (this was a very rough analysis, but you get the picture)
11:01sewardjand that generates exactly one more, etc
11:01bholleysewardj: it's not causal
11:01bholleysewardj: it's generally during frame construction, in this profile
11:01bholleysewardj: each time the frame constructor makes a "native anonymous" element
11:02bholleysewardj: i.e. an element that is an implementation detail of layout
11:02bholleysewardj: it needs to style it
11:02bholleysewardj: because this happens after all the author-provided stuff has been styled
11:02bholleysewardj: and so we end up in these one-off traversals
11:02bholleysewardj: I could pretty easily write a patch to fix that
11:02bholleysewardj: but there are lots of other angles to the whole tiny traversal question
11:03bholleysewardj: and it's not obvious to me whether the right answer is to have gecko do the hinting, or have servo try to guess, or both
11:03sewardjbholley: but there can't be more than 1674 tiny traversals in this test case anyway, right?
11:03bholleysewardj: well
11:03sewardjie. what is actually the worst case here?
11:04bholleysewardj: well, it depends on how much each of these tiny traversals costs us
11:04bholleysewardj: they cost 0.2ms in my profile, but that's also my profiler resolution :-)
11:05bholleysewardj: the numbers are noisy, so it's hard to tell
11:05sewardjsure
11:06sewardjbholley: thanks. I will consider. I also have an idea to tell whether Rayon is spinning
11:06* sewardj -> lunch
11:06bholleysewardj: oh hm interesting
11:07bholleysewardj: it's really hard to tell from the noisy profiles, but another angle may be that we have some pessimal behavior when calling into rayon a bunch of times repeatedly
11:08bholleysewardj: i.e. maybe the threads are still a bit agitated from the last traversal or something
11:08sewardjbholley: relatedly .. are we the only Rayon pool in the process?
11:08bholleysewardj: should be
11:08sewardjor could there be a second instance?
11:08bholleysewardj: if there were that'd be a bug
11:08bholleysewardj: but there should be one per process
11:08sewardjok
11:08bholleysewardj: the gecko profiler shows several rayon pools
11:08bholleysewardj: but I think they correspond to the processes
11:09sewardjok, makes sense
11:09bholleysewardj: (profiles now showing 1.2ms for 1-element parallel traversals, which is way too much)
11:09noxSimonSapin: :O
11:09bholleysewardj: but my _last_ profile showed 0.2ms
11:09noxSimonSapin: Animatable too can be derived.
11:09bholleysewardj: so not totally sure what's going on
11:09noxDERIVE ALL THE THINGS
11:09sewardjbholley: I ask because my profiles show rayon sources from two completely different places
11:09bholleysewardj: oh?
11:09bholleysewardj: what are they?
11:10sewardjbholley: (1) third_party/rust/rayon-core/src/registry.rs:rayon_core::registry::WorkerThread::wait_until
11:10bholley(that is the one I'd expect)
11:10SimonSapinnox: whats this, using custom_derive for its intended purpose without any hack? boring
11:10SimonSapin;)
11:10sewardjbholley: (2) /buildslave/rust-buildbot/slave/stable-dist-rustc-linux/build/src/libcore/iter/mod.rs:rayon_core::registry::WorkerThread::wait_until
11:10bholleysewardj: uh
11:10sewardjbholley: I don't know what that signifies.
11:10bholleysewardj: this is a local build?
11:10sewardjyes, local build of Fx/Stylo
11:10sewardjRust installed via rustup
11:11noxSimonSapin: Ah ah.
11:11bholleysewardj: mmm
11:11* bholley looks at that file
11:11sewardjbholley: at this point I don't know if my tools are lying to me or not.
11:12bholleysewardj: so, that's standard library code
11:12bholleysewardj: but your tools might be getting confused by monomorphization
11:13bholleysewardj: i.e. rayon implements the iterator trait
11:13sewardjbholley: maybe, I just don't know
11:13bholleysewardj: anyway, I think that probably isn't worth chasing for now
11:13sewardjI agree
11:13bholleysewardj: that file definitely doesn't do anything with rayon: https://github.com/rust-lang/rust/blob/master/src/libcore/iter/mod.rs
11:13bholleysewardj: so it's likely monomorphization confusing the tools
11:14sewardjbholley: oh, well spotted (that it was mod.rs)
11:14sewardjbholley: ok. /me really -> lunch now
11:14bholleyguten ap
11:14sewardjthx!
11:16noxSimonSapin: Thank god we don't get email notifications for this:
11:16noxhttps://irccloud.mozilla.com/file/SNSPhtmA/Capture%20d%E2%80%99e%CC%81cran%202017-05-17%20a%CC%80%2013.16.16.png
11:17SimonSapinnox: thats why I tell people to not @-mention in PR messages
11:17noxSimonSapin: I know I know.
11:17SimonSapinI get those emails sometimes
11:17noxSimonSapin: But I had not noticed it still gets mentioned in PRs,
11:17noxthough that's obvious in hindsight.
11:17SimonSapinfrom bitbucket too
11:20noxSimonSapin: Is it a problem if I move around an Animatable impl?
11:21SimonSapinprobably not, why do you ask me?
11:24noxSimonSapin: Because you are around hah.
11:28noxSimonSapin: Could you review the hashless color quirk?
11:28noxhttps://github.com/servo/servo/pull/16858
11:28crowbotPR #16858: Implement the hashless color quirk (fixes #15341) - https://github.com/servo/servo/pull/16858
11:28SimonSapinnox: link?
11:28SimonSapinlooking
11:29noxSimonSapin: It's a bit insane.
11:29SimonSapinits quirky?
11:29noxYes.
11:32SimonSapinnox: is it https://quirks.spec.whatwg.org/#the-hashless-hex-color-quirk? Plz add spec link
11:32noxSimonSapin: Oh right. Forgot that.
11:33noxSimonSapin: r=you otherwise?
11:33SimonSapinnox: still looking :)
11:34SimonSapinnox: plz use Type::parse rather than Trait::parse
11:34noxSimonSapin: Can't.
11:34SimonSapinisnt it always RGBA?
11:34noxSimonSapin: There is another parse method, and thus it's ambiguous.
11:34noxSimonSapin: Been there tried that. :)
11:34SimonSapinugh
11:35SimonSapin<Type as Trait>::method
11:35noxSimonSapin: RGBA has an intrinsic parse method IIRC.
11:35noxSimonSapin: That&#39;s not better, is it?
11:35* SimonSapin waves hand
11:35noxSimonSapin: I think it was Trait::parse before my change.
11:35SimonSapinlooking at the new parse_quirky method
11:36noxNope I dreamt that yes.
11:37SimonSapinnox: I dont understand CSSColor::parse_quirky
11:37noxSimonSapin: Why?
11:37SimonSapinnox: isnt the first half the same as Parse for CSSColor?
11:38noxSimonSapin: Where?
11:38SimonSapinvalues/specified/mod.rs
11:39noxSimonSapin: If I don&#39;t repeat some code,
11:39noxSimonSapin: aaa gets parsed as a color with authored == &quot;aaa&quot;.
11:39noxSimonSapin: UAs serialise those as rgb() funs.
11:40* SimonSapin grumbles
11:40noxSimonSapin: Oh.
11:40noxSimonSapin: I guess I could remove that ColorExt thing,
11:40noxSimonSapin: and just put the quirkiness in Color::parse.
11:40noxSimonSapin: That would be cleaner, I think.
11:40noxSimonSapin: Do you want me to do that?
11:41SimonSapinnox: as you like
11:41noxYeah I&#39;ll do that.
11:42noxSimonSapin: I also don&#39;t like that this parse_quirky method is the only one (IIRC) that calls parse,
11:42noxwhereas in every other occurrence, parse calls parse_quirky with AllowQuirks::No.
11:43noxSimonSapin: I now added more lines than I removed some in servo/servo.
11:43* nox is sad.
11:46SimonSapinnox: computing length like you do seems unnecessary
11:46SimonSapindoesnt ito::write return it?
11:46noxSimonSapin: What do you mean?
11:46noxSimonSapin: It returns the number of bytes written.
11:46noxSimonSapin: These bytes might be prefixed by zeroes.
11:46noxAnd followed by the unit.
11:46noxSo at the end, we must have <number of zeroes> + <integer length> + <unit length> == 6
11:47noxSimonSapin: Oh, do you mean I should just write to the array and if it fails I&#39;m done?
11:47noxThat is... less dumb indeed.
11:47SimonSapinsomething like that
11:47noxSimonSapin: Will amend, lol.
11:47noxSimonSapin: Not sure why I did it this way in hindsight.
11:48noxSimonSapin: Oh I know!
11:48noxSimonSapin: I must know the integer length first,
11:48SimonSapinleft align?
11:48noxSimonSapin: otherwise I don&#39;t know how many zeroes I must write, yes.
11:50SimonSapinnox: I took a while to understand &quot;6 - total&quot;, can you add an intermediate variable to name it zeros or something?
11:52SimonSapinnox: r+ with that and the spec link
11:52mystornox: nope, I feel like that probably belongs in a different crate, perhaps which you can configure synstructure to call into
11:52mystornox: (in response to your text from an hour or so ago)
11:53noxmystor: Yes to separate crate, but this crate should be used by default in synstructure probably. :)
11:53mystorS/text/message
11:53noxSimonSapin: Ok.
11:53mystornox: I agree.
11:53mystornox: I would allow it to be turned off though
11:53noxmystor: Why?
11:54noxmystor: If a variant has some #[cfg] attributes, there is no way your generated code can compile in all cases if it doesn&#39;t include them on match arms, can it?
11:54noxmystor: There is probably something to steal about that in serde.
11:55noxmystor: Not sure it requires a separate crate actually, it&#39;s just a matter of copying the attr from the variant to the arm, AFAIK.
11:57mystornox: yeah, true. That shouldn&#39;t be too tricky if cfgs are only used on arms
11:58noxmystor: Oh yeah fields are an issue...
11:58mystornox: I can never remember whether they&#39;re allowed on enum fields
11:58noxI think they are.
11:58mystornox: yeah, that&#39;s the tricksy lne
11:59mystorCfg on statements isn&#39;t stable yet iirc
12:00noxemilio: r? https://github.com/servo/servo/pull/16889
12:00crowbotPR #16889: Refactor how calc() clamping is done on computed values (fixes #15296) - https://github.com/servo/servo/pull/16889
12:05noxemilio: Thanks.
12:05noxeddyb: Drive-by warning silencing. :P
12:06noxErr, emilio*
12:06emilionox: np, thank you :)
12:06emilionox: I&#39;ve been away the whole morning btw, sorry for not having replied
12:06noxemilio: No problem.
12:06noxemilio: I don&#39;t even know in which timezone you are currently.
12:06emilionox: same as you? :P
12:06noxemilio: Oh, back home?
12:07emilionox: I mean, Slovenia and Spain have the same timezone, so it doesn&#39;t actually matter :)
12:07noxOh you in Slovenia.
12:07noxI didn&#39;t gather that.
12:07emilioheh
12:07noxemilio: I&#39;m tz-fluid, woke up at noon today.
12:07emilionox: where did you think I was?
12:07emilionox: lol
12:09noxOh nice: error: unused imports: `Parse`, `ParserContext`
12:09noxSince when warnings from imports from the same statement are coalesced together?
12:10noxstshine: I have absolutely no idea what your comment is about.
12:11stshinenox: no problem, I&#39;ll handle them someday :)
12:11noxstshine: I mean, that&#39;s what I did.
12:11noxThings are accessible only through methods that clamp.
12:11stshinenox: well let me explain
12:11noxstshine: Ah damn forgot to call that clamping method in to_computed.
12:12noxOr did I?
12:13stshinenox: https://github.com/servo/servo/blob/master/components/layout/model.rs#L480
12:13noxstshine: Some(self.clamping_mode.clamp(self.length + len.scale_by(percent)))
12:14noxstshine: Why does this even operate on specified values?
12:14stshinenox: compute used values like this is not correct because it does not take clamping mode into account
12:14noxstshine: What compute value?
12:14stshinenox: how? it is not specified value
12:14stshinenox: CalcLengthOrPercentage
12:14noxThe function you linked is called &#39;specified&#39;.
12:15stshinenox: that is misnamed I believe
12:15noxCalcLengthOrPercentage also exists as a specified value.
12:15stshinenox: it should be something like to_used()
12:15noxstshine: I am immensely triggered by a function called specified but that takes a computed value,
12:15noxso I&#39;ll let you fix that stuff.
12:16stshinenox: all set :)
12:16noxGood. :)
12:16noxemilio: Should I p=1 stylo stuff?
12:16stshinenox: are you going to refactor all the LengthOrOrOr computed value ?
12:16noxstshine: At least the ones with keywords I think.
12:17stshinenox: good, I&#39;ll will for that :)
12:17stshinewait*
12:17noxSimonSapin: Do we have a machinery to declare a specified unit struct type for things similar to None_?
12:19noxhttps://github.com/servo/servo/pulls Is filtering by assignee broken or what?
12:19emilionox: feel free to
12:19* nox emails GitHub.
12:20SimonSapinnox: what are things similar to None_ ?
12:20noxSimonSapin: Auto, Content,
12:20noxthat&#39;s all.
12:20SimonSapinnox: declare_css_keyword! ?
12:21noxSimonSapin: That makes an enum, no?
12:21noxSimonSapin: I want Either<LoP, None_>, Either<LoP, Auto> and Either<LoP, Either<Auto, Content>>.
12:22SimonSapinah, unit structs for a single keyword?
12:22SimonSapinI think there should be a macro for that but last I checked it doesnt exist yet
12:24SimonSapinnox: preferably a rust macro, not mako
12:27noxSimonSapin: That&#39;s obvious. :)
12:27noxemilio: I think I&#39;m going to buy myself a D20 and a D100.
12:27noxAnd then I&#39;ll roll for CI priorities.
12:30noxemilio: Everything related to stylo is p=1&#39;d.
12:47noxemilio: Is there some docs for Gecko prefs?
12:48noxemilio: How do they get propagated into style?
12:57emilionox: so I think there are a few constants in the bindings that tell whether a property has a pref itself
12:57emilionox: we just check that and call into gecko
12:58noxemilio: That&#39;s for property,
12:58emilionox: yes, I don&#39;t think we honor prefs for values
12:58noxemilio: but what about prefs that aren&#39;t about properties, but about some silly quirk in a -moz prefixed fun or whatever?
12:58noxemilio: So we don&#39;t have the machinery for that, do we?
12:59emilionox: I guess we do, wait a second
12:59emilionox: you just have to check manually for that
12:59noxemilio: From where, how?
13:00noxFrom the parsing code in the middle of nowhere?
13:00noxLet me grab my anxiety brown bag to breath into it.
13:01emilionox: lol
13:01emilionox: so we don&#39;t have the machinery indeed, we only have searchfox.org/mozilla-central/source/layout/style/ServoBindings.cpp#1909
13:12SimonSapinplaybot: &quot;ab&quot;.contains(&quot;&quot;)
13:13noxSimonSapin: Great.
13:14SimonSapinplaybot: &quot;&quot;.contains(&quot;&quot;)
13:15emilionox: wanna review a refactor? ^
13:16noxemilio: Not sure, I don&#39;t know a thing about that function.
13:17emilionox: it&#39;s just killing code, but yeah, maybe I can wait for bholley
13:17noxemilio: Thanks for killing some assertions,
13:17noxsometimes I feel like I&#39;m reading code from a language without enums.
13:19emilionox: heh, fair enough
13:39noxjdm spends way too much time reviewing from phone.
13:39noxApparently his device autocorrected build-geckolib into build-cef lol.
13:43SimonSapinugh &quot;When comparing the name part of a CSS attribute selector to the names of namespace-less attributes on HTML elements in HTML documents,&quot;
13:43SimonSapinonly for namespace-less attributes
14:26ajeffreypaul: ping
14:31ajeffreypaul: when you&#39;re around... I&#39;m wondering what the semantics is expected to be of setting the &quot;mozbrowser&quot; attribute of an iframe that&#39;s already attached to a document?
14:32ajeffreysuddenly the nested browsing context becomes a top-level browsing context, and unsurprisingly servo becomes quite confused.
14:32globhrm
14:32ajeffreymy suggestion is to treat this as if the iframe was detached then re-attached, so its document is reloaded.
14:33* ajeffrey is trying to refactor constellation code, and the main problem is mozbrowser support :(
14:34ajeffreygozala: you may have an opinion ^
14:34glob`mach vendor rust` is failing on servo-vcs-sync; investigating
14:35noxI added a dependency to style,
14:35noxthat may or may not be the issue.
14:35noxMy PR just landed, hence me mentioning this.
14:35noxI asked yesterday and was told &quot;revendoring will just work fam&quot;.
14:35globnox: it normally just works, but evidently it also sometimes fails :)
14:35noxglob: Incidentally,
14:36noxmy PR title included a syringe emoji
14:36noxI hope that&#39;s not related this time. :D
14:36globbug 1361005
14:36firebothttps://bugzil.la/1361005 NEW, nobody@mozilla.org `mach vendor rust` failed (failed to select a version for `smallvec`)
14:36noxglob: Ok, smallvec isn&#39;t my thing.
14:36globyeah, sorry, this is from the last time a dep change broke things. give me a moment and i&#39;ll get the real error
14:37glob(because of bug 1361003 i don&#39;t see the errors immediately)
14:37firebothttps://bugzil.la/1361003 NEW, nobody@mozilla.org cargo errors during rust vendoring should be printed
14:38canaltinovaemilio: I changed servo binding signatures in servo side of that patch. That servo side patch should not have build in a stylo build by itself but it built. How is that possible?
14:39emiliocanaltinova: it built because the C++ side had the wrong signatures, but due to how passing arguments in the architecture we use for testing it works
14:40emiliocanaltinova: so I guess they were getting whatever there was in the registry used to pass that argument
14:40emiliocanaltinova: now the fun part: How could that fix tests? :)
14:40canaltinova