mozilla :: #servo

11 Sep 2017
01:10njnwho knows about KeyframesStepValue ?
01:10njnspecifically, if the Arc<Locked<PropertyDeclarationBlock>> within it could be considered a &quot;secondary&quot; reference -- is there a &quot;primary&quot; Arc to that PropertyDeclarationBlock somewhere else?
01:13heycamnjn: no, that should be the primary reference. it&#39;s the declaration block inside a keyframe rule, like: @keyframes blah { 12.3% { top: 10px; color: green; } }
01:14njnheycam: do you know where other references might be?
01:14heycamnjn: maybe some CSSOM thing?
01:14njnheycam: ok, I&#39;ll try it and see what DMD says
01:14njnthanks
01:14heycamok
01:16njnheycam: I&#39;m currently changing the MallocSizeOf stuff so it&#39;s more like the existing HeapSizeOf stuff in Servo
01:16njnamong other things, this will allow #[derive(MallocSizeOf)] for straightforward cases
01:17njnheycam: oh, I have a similar primary/secondary question for Rule::style_rule -- any idea?
01:19heycamnjn: nice!
01:19* heycam looks
01:20heycamnjn: where is Rule::style_rule ?
01:20njnin stylist.rs
01:21heycamnjn: that&#39;s not a primary reference
01:21njnok. Usual follow-up question: where is the primary ref?
01:21heycamnjn: it&#39;s the StyleRule under the Stylesheet
01:21njnok, thanks
02:45bholleyheycam: yt?
02:45heycambholleyhi!
02:46bholleyheycam: hey, just thought I&#39;d fill you in with where I got on the sharing cache stuff these past few days
02:46bholleyheycam: and get your thoughts
02:46heycambholley: sure, interested to hear how it&#39;s going
02:47bholleyheycam: so, the tl;dr is that I think we want to do two things: the rulenode-based sharing stuff, but also redesigning the style sharing cache to be two-level
02:47bholleyheycam: i.e. making it a HashMap, keyed by parent CV
02:47heycambholley: so that we can do sharing even if the shareable thing isn&#39;t in our current work unit?
02:48bholleyheycam: well, it&#39;s not so much about work unit as it is about the cache size
02:48bholleyheycam: right now we set the cache size to 31
02:48heycambholley: aha I see
02:48bholleyheycam: which was basically &quot;making this bigger seems to increase sharing, but 31 is already kind of a lot to iterate over each time&quot;
02:48bholleyheycam: the reason the 2ndpass stuff doesn&#39;t hit parity with gecko is due to sharing cache eviction
02:49heycambholley: that makes sense
02:49bholleyheycam: if we use a hashmap, as long as the value bucket is >= 10 entries, we should have parity with gecko&#39;s behavior
02:49bholleyheycam: because gecko uses the style context tree, and tries the most recent 10 contexts
02:49heycambholley: so the style sharing cache now would be able to share (within the current thread) over the entire traversal?
02:50bholleyheycam: it could, though I think we still probably want some limit
02:50bholleyheycam: the point is just to make the limit per-parent rather than shared for everything
02:50heycambholley: yeah, ok
02:51bholleyheycam: so my general feeling is that we should shoot for parity with gecko&#39;s setup in the single-threaded case, and then see where we are for the parallel stuff
02:51bholleyheycam: but the measurements are promising
02:51heycambholley: that sounds like the right way to approach it :)
02:52bholleyheycam: do you see any downsides to the hashmap-as-sharing-cache? I don&#39;t think the current vec-like approach was a very carefully-reasoned design
02:52heycambholley: nothing comes to mind. apart from the hashtable lookup, we&#39;ll still be looking a Vec of entries doing the same work, yeah?
02:53bholleyheycam: right. There _is_ the question of extra heap allocations for each of these things
02:53bholleyheycam: but I think we can probably just reuse them
02:53bholleyheycam: we already store the sharing cache in TLS for reuse
02:53bholleyheycam: so we can just have a vec of free vecs, basically
02:53heycambholley: harder to reuse the hashmap entries I suppose
02:53bholleyheycam: and when we clear the hashmap, we can take ownership of them and store them back in the free pile
02:53heycambholley: when do we clear the hashmap?
02:54bholleyheycam: when we hit a new level in the dom
02:54heycambholley: oh at teh end of the traversal?
02:54heycamoh
02:54heycamok
02:54bholleyheycam: because we&#39;ll never see anything from the old level again
02:54bholleyheycam: and thus no sharing
02:54heycambholley: yeah, fair
02:54bholleyheycam: so anyway, my preliminary measurements from the patches look good, the next step is to make sure they don&#39;t negatively impact perf
02:55heycambholley: sounds good
02:55bholleyheycam: which I&#39;m working on next, but probably won&#39;t finish tonight
02:55heycambholley: we should make some decisions about which patches we&#39;re going to land from the configurations I&#39;ve been testing, too
02:55bholleyheycam: will continue tomorrow, but wanted to see what&#39;s on your plate today and if you&#39;re blocked on anything from my end
02:56bholleyheycam: right
02:56bholleyheycam: I think we&#39;ll probably want to remeasure reset struct sharing on top of the improved CV sharing
02:56heycambholley: well today I need to spend some time on this presentation
02:56bholleyheycam: which is why I feel so much urgency around getting this done
02:56heycambholley: but if there&#39;s any other test runs I can push off or reviews I&#39;ll do them today too
02:57bholleyheycam: ok, sounds good
02:58bholleyheycam: so IIRC, 2ndpass was a significant win, right?
02:58bholleyheycam: 1%?
02:58heycambholley: yes, over stylo base
02:58heycambholley: should the new approach be better?
02:58bholleyheycam: ok. At least on the html spec, hashmapsharing doubles the effectivenss of sharingcache
02:58bholleyheycam: er, of 2ndpass
02:59heycamah great
02:59heycambholley: I do wonder if there&#39;s any way we could increase sharing of anonymous box styles more than we are. but I&#39;d have to remeasure after all bz&#39;s recent patches to see how much we&#39;re losing out from that
02:59bholleyheycam: so I think the next step is to put together patches for hashmapsharing, since that would be something to land first before 2ndpass
03:00heycamok
03:00bholleyheycam: right. I generally think that we probably want to get optimal Element CV sharing first, since everything else is dependent on that
03:00* njn embeds a FnMut in a struct and lives to tell the tale
03:00bholleyheycam: and it will impact the tradeoffs of the various othe rthings
03:00heycamok
03:00heycambholley: meeting, back in a bit
03:00bholleyheycam: ok
03:01heycambholley: if you have any preliminary patches you want me to measure send them my way
03:02bholleyheycam: as long as you weren&#39;t planning to spend your day measuring the tradeoffs between struct caching and struct sharing, there&#39;s probably no particular urgency towards getting AWSY numbers from my patches today. I can spend tomorrow fleshing them out and cleaning them up and then we can measure that
03:04heycambholley: ok cool
03:41edunhamlarsberg: i was offline all day and i see the need for a servo master highstate in my /lastlog. Is this still the case or did that get rolled out?
04:29njnwhere is ElementState defined?
04:30heycamnjn: components/style/element_state.rs
04:30njnheycam: thanks
04:30njnah, it&#39;s bitflags, that&#39;s why tags and my grepping both failed
04:40emiliobholley: I&#39;m around if you&#39;re still around :P
05:21njnis it possible in Rust to say that a particular type T does *not* implement a trait?
05:21njnyou can fake it at runtime by providing an implementation that immediately crashes, but I wonder if it can be done at compile-time
05:23wafflesnjn: I think Cell<T> does something like that
05:23wafflesimpl<T> !Sync for Cell<T>
05:23njnwaffles: I found https://www.reddit.com/r/rust/comments/38bvqw/specify_that_a_type_does_not_implement_a_trait/, which isn&#39;t quite the same, but does mention !sync
05:24wafflesnjn: that *does* sound like what you want
05:26Manishearthemilio: yeah, it already is a struct, there&#39;s some weirdness there
05:26njnwaffles: &quot;error: negative trait bounds are not yet fully implemented; use marker types for now (see issue #13231)&quot;
05:26crowbotIssue #13231: Remove support for UTF-16 in TextEncoder - https://github.com/servo/servo/issues/13231
05:27bholleyemilio: for a few minutes
05:27njnwaffles: are Sync and Send the marker types?
05:28emilioManishearth: it&#39;s not clear what should happen with the font-dependent units
05:28wafflesnjn: yeah they are
05:28bholleyemilio: where are you getting the 9k measurement? Just multiplying out the size of CV?
05:28wafflesnjn: Manishearth might know better
05:28njnwaffles: any suggestion on how I can use them to fake negative trait bounds? I can&#39;t quite imagine it...
05:28emiliobholley: well, from your numbers? Maybe I misread them?
05:28bholleyemilio: those are &quot;thousands of CVs&quot;
05:28bholleyemilio: not kilobytes
05:28emiliobholley: ohh, nvm
05:28Manishearthemilio: anything other than em or % will be computed with the old value
05:28emiliobholley: will comment there
05:29Manishearthemilio: we are ok with this
05:29Manishearthemilio: there is no &quot;right answer&quot; at this level of edge-casery
05:29emiliobholley: clearly too early in the morning for me :)
05:29Manishearthwe won&#39;t match gecko. I don&#39;t care about that in this case.
05:29* bholley would not be sweating a few tens of kilobytes, but is definitely sweating a few tens of thousands of CVs :-)
05:30emilioManishearth: who&#39;s we? Also, mind explaining it either in comments or the commit message? Even if it&#39;s &quot;we don&#39;t really care&quot;
05:31emiliobholley: I still think we shouldn&#39;t be optimizing for the HTML spec fwiw
05:31gwnjn: I think what that&#39;s suggesting is to use a PhantomData marker to get the behaviour you want - e.g. adding _phantom: PhantomData<*mut ()> to a struct would prevent it being Send, without any runtime size/overhead in the struct.
05:32bholleyemilio: sure, but it is a decent example of behavior in the limit
05:32bholleyemilio: if this didn&#39;t move the needly on AWSY, I would care much less
05:34Manishearthemilio: we is &quot;me&quot;, I&#39;m saying we shouldn&#39;t care, sorry
05:34Manishearthemilio: will do
05:35wafflesnjn: we can use negative trait bounds on nightly - do you need it working on stable?
05:35emiliobholley: I think we should be looking into why do CVs cost us much, I suspect most of it is `nsStyleDisplay` structs, that we can shrink ~4words
05:35emiliobholley: that and `nsStylePosition` are absolutely huge
05:37emiliobholley: btw, bye resolvestyle crashes :) https://bugzilla.mozilla.org/show_bug.cgi?id=1395719#c22
05:37firebotBug 1395719 FIXED, emilio@crisal.io stylo: panicked at &#39;Resolving style on <mo> without current styles: ...&#39;
05:38bholleyemilio: yeah, that&#39;s great!
05:38bholleyemilio: if you think you can reasonable shrink nsStyleDisplay and nsStylePosition, please do
05:38bholleyemilio: I&#39;m just not betting stylo on it :-)
05:39Manishearthemilio: still waiting for review on viewport units btw
05:39bholleyemilio: anyway, heading to bed. Put any more comments you have in the bug, will work on it all tomorrow barring compelling arguments not to :-)
05:39emilioManishearth: which patches? I don&#39;t see anything in my queue
05:40bholleyemilio: (and in particular, I&#39;m looking for reasons why the HashMap approach is worse than the existing LRUCache approach)
05:40emilioManishearth: nor in https://github.com/servo/servo/pulls/Manishearth
05:41Manishearthemilio: gah. mozreview carried over r+
05:41Manishearththey&#39;re new patches
05:41Manishearthslightly different strategy
05:41emilioManishearth: where are they?
05:41Manishearthto make the second test pass
05:41Manishearthemilio: bug 1396045
05:41firebothttps://bugzil.la/1396045 ASSIGNED, manishearth@gmail.com stylo: Round down when dealing with relative units
05:41Manishearthyou already r+d
05:41ManishearthI&#39;m asking for rereview
05:42Manishearthi changed the patch ID so i thought mozreview wouldn&#39;t carry over review, but it ... did? idk i don&#39;t understand this
05:42Manishearthsorry, only noticed just now
05:43emilioManishearth: k, thanks :). Sorry for the lag, but there&#39;s no way I could&#39;ve gotten to them :P
05:44Manishearthemilio: yeah sure np
05:44Manishearthemilio: mozreview is confusing
05:44emilioManishearth: why is it still fuzzy-if(stylo)?
05:47njnwaffles: this is for Stylo, so I need it on stable
05:47njngw: alas, I need a negative trait bound on a non-builtin trait
05:47njnwhat I want is this: impl<T> !MallocSizeOf for Arc<T> { }
05:48gwnjn: oh, yea not sure how to do that on stable :/
05:49njnat least I can write that code and comment it out, for exploanation
05:50Manishearthemilio: can&#39;t make the test work both for stylo and gecko
05:50Manishearthemilio: the rounding is still slightly different, just not in an important way
05:51emilioManishearth: oh, the test uses the expected px value, I see
05:51Manishearthyep
05:51emilioManishearth: k, r=me on those
05:51Manishearthcool thanks
05:52Manishearthi think the au update is unnecessary now since it already happened
05:52Manishearthwill land tomorrow, it&#39;s late
05:52* emilio forgot about bug 1395586 ;_;
05:52firebothttps://bugzil.la/1395586 NEW, emilio@crisal.io Add accessors to trivial nsStyleDisplay members, and make them private.
05:53* emilio prepares painful rebase
05:56gwnjn: It&#39;s probably doable if Arc<> in your example above was our own type or a wrapper / newtype, but I guess it&#39;s the stdlib arc you&#39;re referring to?
05:56njngw: it&#39;s actually servo_arc::ARc
05:56njngw: though I&#39;d prefer not to have to modify servo_arc::Arc&#39;s code...
05:57gwnjn: ok - so could you create your own dummy type that doesn&#39;t derive MallocSizeOf, and then have _phantom: PhantomData<NonMallocSizeOfDummy> in that Arc type?
05:57njngw: I see, ok
05:57gwnot sure if that would actually achieve what you want...
06:00njngw: I&#39;m putting it in the too-hard basket, but thanks for the help :)
06:00gwnjn: np :)
06:26xidornstylo-disabled tasks only run in m-c, not autoland or other integration repos?
07:25ferjm!seen waffles
07:25firebotwaffles was last seen 1 hour and 50 minutes ago, saying &#39;njn: we can use negative trait bounds on nightly - do you need it working on stable?&#39; in #servo.
07:27noxnjn: Can&#39;t have negative bounds.
07:27noxemilio: Still killing CVAS
07:28emilionox: did you review my patch?
07:28noxemilio: I&#39;m damn dumb.
07:28emilionox: what happened?
07:29noxemilio: Forgot to review it.
07:29emilionox: lol
07:58noxjntrnr: Y U HANGOUT
07:58noxLet&#39;s use WebRTC next time.
07:59noxI don&#39;t even have Chrome installed ugh
07:59jntrnrnox: should work in firefox nightly
08:00noxIt doesn&#39;t here.
08:00noxIt just told me to install Chrome.
08:00noxAnd we have the best WebRTC impl and that&#39;s free and we should use that anyway.
08:00noxWait maybe I spoofed my UA for some silly stuff earlier.
08:01* nox checks.
08:01noxhttps://irccloud.mozilla.com/file/A6MYFbzp/Capture%20d%E2%80%99e%CC%81cran%202017-09-11%20a%CC%80%2010.01.32.png
08:04noxmic ain&#39;t working, one sec
08:06noxCan we move that to Vidyo
08:18larsbergedunham: nothing needed until Glenn&#39;s saltfs PR lands for changing the home gatin on WR :)
08:26noxlarsberg: Where is the docs for adding new repositories to servo/ org?
08:35noxDamn appear.in is nice.
08:38noxjntrnr, cpearce: Turns out my headset mic volume was set to minimum,
08:38noxthat explains things.
08:40jntrnrnox: heh
08:41noxjntrnr: Apparently some WebRTC services shows a message if they can see the mic but no sound is coming to it, asking to check for volume setting,
08:41noxpadenot is talking about it to the appear.im guy now.
08:41noxWould have avoided me the embarrassment. :p
08:50philnnox: i understood cpearce&#39;s reasons but why were you saying in the chat that license is an issue?
08:51noxAin&#39;t LGPL something we actively avoid?
08:51noxAren&#39;t some codecs even GPL in this thing?
08:51larsbergNox it&#39;s on the wiki, but you will need an owner to set up all the magic parts. If you create and org that&#39;s ok
08:51philnerr, no
08:51noxjntrnr: God damn it, tried all kind of combinations of consonants on GH, failing to autocomplete you,
08:51noxthen realised you used your full name.
08:52philnwell, some are but they are contained in a specific plugin set and are optional
08:52larsbergLgpl and Gpl are out for servo - there is a list approved by legal in our tidy.py license check code
08:52cpearcephiln,nox: We use some LGPL libs in Gecko already; for example ffmpeg. We have a shared library where we stick them all.
08:52noxAh.
08:52noxphiln: What larsberg said.
08:52philnok, i didn&#39;t know about that!
08:53noxlarsberg: &quot;Give write access to the Robots team.&quot;
08:53noxtfw no Robots team anymore
08:54larsbergThere should be but you might not be able to see it. It&#39;s 2am and I just got off a plane in MV - if you open an issue and tag infra I can look in the morning.
08:55philnlarsberg: so, only MPL or Apache, if i read tidy correctly, right?
08:55noxphiln: Or ISC etc.
08:56cynicaldevilnox: Is there a more idiomatic way to write this part https://github.com/servo/html5ever/pull/308/files#diff-500557afae4b1b1befdebc4eedf9cc09R270 ?
08:56larsbergphiln: MIT should be fine too
08:56noxphiln: That tidy.py check AFAIK is for licenses of stuff *in* servo/servo.
08:56cynicaldevili.e, without having parent and has_parent
08:57noxcynicaldevil: Seems ok to me. I would just not make a new variable for has_parent.
08:57philnah :)
08:59cynicaldevilnox: Hmm, why does Rc have a `get_mut` method but not `get`?
08:59philnbut if we import Gecko&#39;s media stack which has ffmpeg, wouldn&#39;t it be an issue?
08:59noxNo idea.
08:59noxI know we avoided using ffmpeg for some reason already.
08:59noxFor the AV metadata stuff.
09:00noxlarsberg: Go to sleep.
09:00noxjntrnr: I thin meetings are a bit more tolerable when not done through miserable Vidyo software.
09:01larsbergnox: as soon as the Lyft gets to good ole Zico :)
09:01noxcpearce: Forked your repos and added the IRC hook to it.
09:02noxI don&#39;t know the sikrits to add Homu though, but it&#39;s not a pressing matter right now, right?
09:02cpearcenox: yeah, it can wait until we&#39;ve got more tests. thanks!
09:03philnnox: do i need to join the servo org?
09:03cpearcenox: Servo would have avoided importing ffmpeg because you were probably looking at it containing an h264 decoder, which is patent encumbered in some jurisdictions. The ffmpeg code that Gecko contains has only the ffvpx decoder, nothing else. ffvpx is a VP8 and VP9 decoder that better utilizes parallelism than than libvpx, Google&#39;s software decoder.
09:04noxphiln: I added you to the repos, so yeah I guess?
09:04cpearceWe can avoid importing ffvpx if necessary by relying on libvpx, which is an Apache style license from memory.
09:04noxcpearce: AACK
09:04noxAck*
09:04cpearceIt&#39;s not as good however.
09:04philnnox: got it, thanks
09:05est31cpearce: doesnt firefox also ship an h264 decoder?
09:05est31for linux?
09:05noxest31: Not from ffmpeg AFAIK.
09:06cpearceest31: we do not bundle an h264 decoder. We&#39;ll use the operating system&#39;s decoder if it&#39;s installed
09:06est31I only can remember that it was gstreamer for some time, but then there was a switch to ffmpeg
09:06noxcpearce: Oh, TIL
09:06cpearceFirefox will use the operating system&#39;s ffmpeg for h264 and AAC on Linux if it&#39;s installed.
09:06est31okay
09:32ferjmcould someone with permissions r,delegate=me on https://github.com/servo/servo/pull/18372 please?
09:32crowbotPR #18372: TEST: fix and add case of po-observe.any.js - https://github.com/servo/servo/pull/18372
09:41wcpanheycam: so I added some conditions that needs flush: 1. the element (or ancestors) is animating 2. the property needs to flush layout 3. the element is out-of-document
09:42heycamwcpan: sounds good. that last one may even not be needed, but I am not sure. not so common, so safe to make it flush...
09:42heycamwcpan: do you think your new patch will avoid the 10% regression?
09:42heycamwcpan: I am a bit worried about that
09:43wcpanwe dont have any snapshot for detached element, so I&#39;m a little bit worry about that
09:44wcpanheycam: maybe I can use raw pointers in that function
09:44heycamwcpan: oh right, I forgot about restyles in the disconnected subtrees
09:44wcpanI&#39;ll do the microbenchmark again anyway
09:44heycamwcpan: where are you currently using smart pointers that you could convert to raw pointers?
09:45wcpanheycam: document, preshell, prescontext
09:45wcpanheycam: i think they wont disappear suddenly
09:45heycamwcpan: yeah, probably fine. (they can disappear after the flush.)
11:48travis-ciServo failed to build with Rust nightly: https://travis-ci.org/servo/servo-with-rust-nightly/builds/274131951 CC nox, SimonSapin, jntrnr
11:51SimonSapin/usr/lib/llvm-3.4/lib/libLLVMSupport.a(Process.o): In function `llvm::sys::Process::FileDescriptorHasColors(int)&#39;:
11:51SimonSapin(.text+0x5b7): undefined reference to `setupterm&#39;
11:51SimonSapinemilio: have you seen errors like this when building osmesa-src? ^
11:51SimonSapinit also says configure: WARNING: Building mesa with statically linked LLVM may cause compilation issues
11:51SimonSapinon Travis
12:19emilioSimonSapin: yes, you need to install terminfo or some package like that
12:22SimonSapinemilio: neither .travis.yml or README.md in the repo seem to say that or list a package name
12:22emilioSimonSapin: ncurses, maybe?
12:34sewardjemilio: ping
12:34emiliosewardj: pong
12:34sewardjemilio: your 1-line patch from yesterda, that inlines the FallibleAllocationError constructor?
12:35emiliosewardj: yes?
12:35sewardjemilio: IIUC, such a value is only constructed in the _double functions
12:35emiliosewardj: yes
12:36sewardjemilio: I ask, because I tried to measure any perf gain and couldn&#39;t find any (actually a small loss, but could be noise)
12:36emiliosewardj: yeah, I don&#39;t expect it to be a measurable perf win
12:36emiliosewardj: just something that would be nice, because paying a function call for that is unfortunate
12:37sewardjemilio: right .. but inlining costs icache misses
12:37sewardjemilio: but anyway, no problem. I just wanted to check I understood the perf effects correctly.
12:37emiliosewardj: that&#39;s actually creating a pointer-sized struct, how could that be worse?
12:37sewardjemilio: are you thinking of d-cache misses?
12:39emiliosewardj: well, like, I&#39;m not sure how inlining that could improve the chances of i-cache misses?
12:43sewardjemilio: well, I dunno. I am just speculating.
12:43* sewardj continues measuring
13:09SimonSapinmbrubeck: r? https://github.com/servo/smallbitvec/pull/1
13:09crowbotPR #1: Make buffer_mut take &mut self - https://github.com/servo/smallbitvec/pull/1
13:10mbrubeckr+
13:10mbrubeckthanks!
13:10SimonSapinmbrubeck: also could you add the repo to https://github.com/orgs/servo/teams/developers/members ?
13:10SimonSapinor https://github.com/orgs/servo/teams/developers/repositories , rather
13:11mbrubeckSimonSapin: Looks like it&#39;s there already
13:12SimonSapinmbrubeck: hmm, https://github.com/servo/smallbitvec/edit/master/src/lib.rs tells me I dont have write access to the repo
13:13mbrubeckHmm. Well, after moving the repo to the servo org, I&#39;m no longer an admin so I can&#39;t seem to edit permissions...
13:13paulmbrubeck: hi - if you have some time today, you think you can help me with #18410?
13:13crowbotPR #18410: harfbuzz-sys update - https://github.com/servo/servo/pull/18410
13:13mbrubeckOh, @servo/developers only has &quot;Read&quot; access
13:14mbrubeckpaul: Yeah, I&#39;ll take a look today.
13:14SimonSapinthat doesnt seem useful, for public repos
13:14paulmbrubeck: thank you!
13:14SimonSapinoh, for that repo you mean
13:48sewardjemilio: ping
13:48emiliosewardj: pong
13:49sewardjemilio: when profiling, I saw a lot of calls to hashglobe::hash_map::HashMap<..>::try_entry
13:49sewardjemilio: so I marked it #[inline] and profiled again
13:49sewardjemilio: but it doesn&#39;t seem to have been inlined.
13:50sewardjWhat should I expect here?
13:50sewardjWill the compiler inline that if I ask it?
13:50emiliosewardj: You can use `#[inline(always)]`
13:50emiliosewardj: `#[inline]` is only a hint
13:50sewardjah i see
13:51sewardjemilio: I try
14:16bz_sleepAnyone have the bist to merge https://github.com/servo/rust-cssparser/pull/195 ?
14:16crowbotPR #195: Avoid array overallocation when parsing background-image property - https://github.com/servo/rust-cssparser/pull/195
14:22jdmyep
14:24jdmoh good, got &quot; Too many open files (os error 24)&quot; on a builder
14:24bzjdm: Thanks!
14:25* bz wonders whether he could have done that himself; didn&#39;t realize rust-cssparser was also under homu control
14:25bzOnce that merges, do I need to do things to update servo and gecko to the new cssparser version?
14:25* bz figures yes
14:26jdmbz: if you update the cssparser lines in the cargo.toml files in servo, that will cause gecko to auto-vendor when it merges
14:27bzAh, excelltn
14:27bzer, excellent
14:27jdmbz: that will require running `./mach build` in a servo directory so that servo&#39;s cargo.lock is updated, though
14:27bzSo just a servo PR to update those
14:27bzok
14:27* bz can do that.
14:27bzThanks!
14:27jdmyou can interrupt the build as soon as the first compiling line appears
14:28jdmbecause at that point the cargo.lock has been updated
14:28bzyep
14:28* bz has done that before
14:28bznothing like build systems that modify the srcdir... ;)
14:29bzemilio: ping
14:29emiliobz: pong
14:30bzemilio: Thank you for picking up that first-letter bug
14:30emiliobz: np, I was curious and rr made it fantastically easy. I&#39;ll land a crashtest asap (should I wait until nightlies are out?)
14:30noxbz: I am contuining to kill CVAS,
14:30bzemilio: I have some questions/comments about the patch, though
14:30emiliobz: sure
14:30noxbz: if you didn&#39;t yet, don&#39;t bother rebasing your PR,
14:30bzemilio: That was the first question. ;)
14:30noxI&#39;ll just carry it.
14:30bznox: my PR is all landed
14:31noxOh.
14:31emiliobz: his PR landed already :D
14:31emilioerr, nox ^
14:31noxI see that you didn&#39;t sat on your hands while I was having some French-lengthed lunch.
14:31noxsit*
14:31emilionox: it landed over the weekend
14:31noxWait what
14:31bznox: You might be able to just nix http://searchfox.org/mozilla-central/source/servo/components/style/values/generics/image.rs#37-76
14:31bznox: Assuming we impl ToComputedValue for Box<T>
14:31noxOH
14:31noxbz: So what happened is that I saw emilio&#39;s PR land,
14:32noxand got a notif about another PR needing a rebase because of emilio&#39;s,
14:32bzAh
14:32noxand I just assumed it was your PR.
14:32nox:)
14:32bzemilio: OK, next question.
14:32noxbz: Yeah we will implement that.
14:32bzemilio: You passed parentStyleIgnoringFirstLine for aNewLayoutParent
14:32bzemilio: Shouldn&#39;t it be parentStyleContext->AsServo() instead?
14:32emiliobz: hmm, good point...
14:32* emilio thinks
14:33emiliobz: I think so, yeah, it has a frame after all
14:33noxemilio: Oh btw,
14:33noxsomething you may enjoy,
14:33bzI should note that the interaction of first-line, first-letter, and display:contents can best be described as &quot;fucked&quot;
14:33noxyou know when we do PRs with ~10 similar commits with a similar message?
14:33bzbut that&#39;s not our problem right now
14:34emiliobz: yeah, you tell me :)
14:34noxemilio: do yourself a favour and run git commit -c @ next time.
14:34noxThis will open your editor with the commit message of HEAD.
14:34bzemilio: Last question. Can we not remove the thing at http://searchfox.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp#12259-12270 ?
14:34bzemilio: er, http://searchfox.org/mozilla-central/rev/00fa5dacedb925022f53d025121f1a919508e7ce/layout/base/nsCSSFrameConstructor.cpp#12259-12270
14:34emilionox: how is that different from `g commit --amend`?
14:34noxemilio: No no,
14:34bzemilio: Because now we make sure to get the right style context &quot;up front&quot;
14:34noxit does a *new* commit,
14:34noxit doesn&#39;t amend HEAD,
14:34emilionox: oh, I see, neat
14:35nox:)
14:35emiliobz: yes, I mentioned that in the bug
14:35* bz looks
14:35emiliobz: the answer is &quot;only that part&quot; :)
14:36* bz is not following
14:36bzI meant specifically just that block I linked to
14:37emiliobz: right, the answer to &quot;I wonder if we could revert bug bug 1385656 after this patch...&quot; is &quot;just that bit&quot;
14:37firebothttps://bugzil.la/1385656 FIXED, bzbarsky@mit.edu stylo: RecoverLetterFrames doesn&#39;t play nicely with ::first-line
14:37bzAh. Yes.
14:37emiliobz: I&#39;ll file a bug for those two followups
14:38emiliobz: if that was the last question, now I got one for you, if you don&#39;t mind :)
14:38bzSure
14:38emiliobz: when does GetXBLBinding differ from GetBindingWithContent? In particular, why do we use the first on UnbindFromTree, and the second on BindToTree?
14:38bzThat was all I had. ;)
14:39bzGetXBLBinding gives you the binding for the element.
14:39bzIF any.
14:39emiliothe other does get you the binding with anon content, but that may differ from the XBL binding for the element
14:39bzGetBindingWithContent gives you the most-derived binding for the element tht has an <xbl:content>
14:39bzBecause bindings can inherit
14:40bzSo you can have a binding for an element that inherits from another binding that inherits from another binding, etc
14:40emiliobz: sounds fun :)
14:40bzThey can all define behaviors for the element
14:40bzadd methods, etc
14:40bzBut only the most-derived binding with <xbl:content> produces anonymous content
14:41bzbecause xbl doesn&#39;t have the multiple shadow tree stuff shadow DOM tried to do
14:41bzOK
14:41bzSo that answers the &quot;when does it differ?&quot; question
14:41bzNow for the other one... ;)
14:41* bz looks at the code
14:41emiliobz: hmm, never mind, the second one ends up doing `mNextBinding->ChangeDocument` as well
14:42bzSo in BindToTree we use GetBindingWithContent because we actually want to get our hands on the anon content
14:42bzAnd that&#39;s where it hangs off of
14:42emiliobz: so for my purpose I want GetBindingWithContent. I just needed to understand what all the `mNextBinding` thing was about
14:42* emilio revises his patch
14:42bzAh, ok
14:42bzGlad to be of service.
14:43bzSo yeah, it&#39;s for things like http://searchfox.org/mozilla-central/rev/00fa5dacedb925022f53d025121f1a919508e7ce/browser/components/places/content/menu.xml#14
14:43bzThough in this case the more-derived places-popup-base binding has content itself, of course.
14:44bzBut nothing prevents one from extending a binding with content and not having any anon content of your own, just behavior
14:44bzmmm
14:44bzcycle time for cssparser is so fast... ;)
14:45jdmyep
14:45* jdm tries to remember the next step for his url sharing patch
14:45emiliobz: ok, that&#39;s all I think. You may want to take a look at bug 1398448, too bad we&#39;re not getting rid of the recursive change hint stuff yet :(
14:45firebothttps://bugzil.la/1398448 NEW, nobody@mozilla.org page loading is to slow if scrolling during page loading (not always reproduce, but 50/50)
14:46bzyes, that&#39;s next on my list to look
14:46* bz wants to start homu on this cssparser update first
14:47bzhuh
14:47bzwe never updated to 0.20.1 apparently?
14:52bzhmm
14:52bzSo the 0.20.2 version of cssparser landed
14:52bzI guess now we need to republish it on crates.io?
14:52bzSo servo can pick it up...
14:52bzhow do I do that?
14:53* jdm will do that
14:53bzjdm: Thank you!
14:54jdmbz: done
14:54tromeyit&#39;d be nice if merging a version bump PR did that automatically
14:54bzyep, that&#39;s perfect.
14:54* jdm agrees
14:54* bz thirds
14:55bzautomate all the things
14:56bzAnyone want to give that PR a quick once-over?
14:58bzemilio: I thought I had asked you to not make WipeContainingBlock sync?
14:59bzAh, I had asked you to do it as a separate changeset
14:59bzOK, I see
14:59bzWell, I&#39;m glad my pessimism was justified... ;)
15:06emiliobz: yup, you were right ;)
15:07bzemilio: r=me in the bug
15:07emiliobz: thanks!
15:07bzemilii: care to stamp the above cssparser version bump pr?
15:07bzer, emilio
15:07emiliobz: SimonSapin already did it
15:07* bz ponders the concept of multiple emilii; could be useful for getting work done
15:07emilioheh
15:07bzOh, so he did. Sweet. ;)
15:09noxemilio: mfw I forgot an important detail.
15:09bzThough &quot;emilii&quot; could also be just referring collectively to the https://en.wikipedia.org/wiki/Aemilia_(gens) family, with a bit of mis-spelling
15:10noxWait no, everything is fine.
15:10* bz tries to figure out what contiuum nox lives in where Friday comes before Wednesday
15:10SimonSapinnox: famous last words
15:10bzBut after Monday
15:10bzhrm
15:10noxbz: wat
15:10emilionox: which detail?
15:10bznox: mfw vs mwf
15:10noxbz: my face when
15:10emiliolol
15:11bznox: And yes, I did the obigatory urban dictionary lookup, and then pretended to misunderstand anyway. ;)
15:11noxemilio: There are some manual impls where a variant Calc(Box<T>) computes to Calc(U)
15:11noxbz: Ah ah.
15:11bzWe still have unreported stylesheet memory
15:11noxemilio: So these needs s/self/(**self)/
15:11* bz pokes at memory reporters
15:11nox41 of them, to be precise.
15:11emilionox: that shouldn&#39;t be a huge deal though, right?
15:11emilionox: 41!
15:12noxNo.
15:12noxemilio: But I was greeted with a wall of errors,
15:12nox41 errors from a proc macro without spanning info,
15:12emilionox: I thought only `CalcLengthOrPercentage`
15:12noxthat gets me every time.
15:12noxemilio: Well I looked only at this one, maybe the 40 others are different and SimonSapin is right.
15:12bzDoes anyone know what the ETA is on enum flattening?
15:12SimonSapinnox: (I have no idea what the context is)
15:12noxeddyb: ^
15:12noxSimonSapin: You don&#39;t need to.
15:13bzOur current story where Gradient has something like 56 bytes of discriminants is ... a bit sad
15:13noxbz: Clearly it&#39;s not enough bytes.
15:13bz(Note that it&#39;s got a mix of structs and enums, so flattening it completely might be touch)
15:13bzer, tough
15:13bznox: heh
15:13noxIt won&#39;t ever get flattened completely AFAIK.
15:13bzyeah, seems like a tough problem
15:13noxEspecially because it&#39;s not sound to put outer discriminants in inner padding space, IIRC.
15:14eddybbz: I&#39;m literally in the middle of nuking the field path representation
15:14bzIt just bothers me that we have this big struct
15:14bz~200 bytes
15:14eddybI went back to the branch nox knows
15:14bzBut 56 bytes of that is representing about 7 bits
15:15eddybit&#39;s p cool that I can get away with an arbitrary offset for the discriminant
15:15bzAnd this is more or less a consequence of it just modeling the CSS spec in the most obvious way....
15:15noxbz: Yeah it&#39;s rightly a bother.
15:15* jdm disembarks
15:15noxNot necessarily in the most obvious way,
15:15noxbut in the most typesafe way.
15:15* bz likes it when clean, clear code also happens to be the most efficient code. ;)
15:15bznox: well, fair.
15:15noxThere are plenty of Gecko representations that allow for values that doesn&#39;t make sense.
15:15eddybI have like three-four more commits
15:16eddybsoooo maybe a week? depends how much attention my interns demand :P
15:16noxI wouldn&#39;t call Gecko reprs clean and clear.
15:16bzAh, that&#39;s much sooner than I expected.
15:16bznox: I never claimed they were!
15:16noxbz: Yeah I didn&#39;t say you did. :)
15:16bznox: But if we&#39;re trying to do better here...
15:16bznox: It sure would be nice if we could have our cleanliness and clarity and eat our memory usage cake too. ;)
15:17bzeddyb: Ah, that&#39;s way closer than I was thinking.
15:17bzeddyb: So might actually make 57?
15:17noxIMO if we want to do better, maybe at some point Rust should allow us to say &quot;forbid mutable references to this field and do whatever packing you want with that&quot;
15:17* bz is not sure how the schedules line up
15:17eddybbz: I will try to get it in this release cycle
15:17bzThe other question I have is whether there is a tool that will dump out the layout of a Rust enum or struct for me
15:18* bz recalls there was a tool for C++ that he used for this
15:18tromeypahole perhaps though the output may look weird for rust enums
15:18bzaha
15:18bzpahole is what I was thinking of
15:18bzHmm
15:19bzI guess it could in theory work on Rust stuff...
15:19bzI should try
15:19bzwould eliminate a lot of the guesswork I&#39;ve been doing
15:19tromeyI should just add this to my gdb pahole
15:20* bz points to &quot;automate all the things&quot; ;)
15:21noxDid you automate the pointing?
15:23bzNot yet
15:23* bz waiting on dmd to run
15:23bzHence time to type... ;)
15:23noxHeh.
15:25bzhrm
15:25bzIt occurs to me that I left http://searchfox.org/mozilla-central/rev/00fa5dacedb925022f53d025121f1a919508e7ce/servo/components/style/gecko/conversions.rs#154-156 like that
15:25bzI wonder how many copies of the gradient it ends up doing...
15:26bzWhat is QualifiedRuleParser ?
15:28SimonSapinbz: a trait to provide &quot;callbacks&quot; to cssparser