mozilla :: #servo

13 Jul 2017
00:05Manishearthemilio: what's up with gmail using invalid CSS? it's like the second time that's happened :|
00:07Manishearthsure, our fault for not rejecting that, but still, weird
00:10emilioManishearth: yeah, I bet they have zillion layers of abstraction over the final CSS
00:10emilioManishearth: so sometimes something slips through
00:13KiChjangjfm: Joshua Frank Matthews
00:22canaltinovaemilio: welp, how we couldn't catch this before :) ^^^
01:21wafflesKiChjang: jfm == "just for messing" the IRC
01:21KiChjangor Just For Me
01:35emiliobholley: around?
01:35emiliocanaltinova: indeed, such a shameful mistake
01:35emiliocanaltinova: but fwiw we had not tests for it, so...
01:35bholleyemilio: yes, but about to head off
01:36emiliobholley: yup, just one general question
01:36emiliobholley: so looking into reducing memory usage of the invalidation stuff (because right now a dependency takes a lot for win32)...
01:36emiliobholley: I thought of
01:37emiliobholley: that will work, _but_, while writing it I realised that the point of all those is to get way less dependencies to scan
01:37emiliobholley: and I'm starting to think that the selector hashes there are a waste of space
01:38emiliobholley: because we usually get around 4/10 selectors
01:38emiliobholley: so, question... Do you think I should do, removing hashes (at that point each dependency is two words, and storing it separately + index would be just needless indirection IMO), or both?
01:39bholleyemilio: I don't have the cycles to give you a good answer right now
01:39bholleyemilio: I do know that the selectormap was a big win under the old model
01:39emiliobholley: I'm _not_ suggesting to remove the SelectorMap
01:39emiliobholley: I'm suggesting to store indices in the SelectorMap
01:40emiliobholley: and the dependencies in an array (right now we duplicate them)
01:41bholleyyeah I can't give you a synchronous answer right now, sorry
01:41emiliobholley: no worries, rest well
02:04emilioheycam: ping?
02:04heycamemilio: hi
02:04emilioheycam: do you have any restyle-hint perf-test by any chance?
02:05heycamemilio: what kind? there are some tests in the style-perf-tests that e.g. we don't restyle a whole subtree when a DOM mutation should just generate RestyleSelf
02:06emilioheycam: well, I guess they don't test what I want... I wanted to measure how much does it hurt to remove the hashes in ^
02:06emilioheycam: but I'll just measure the amount of selectors we get for each element instead, I guess
02:07emilioheycam: you're free to review it if you want
02:08heycamyeah I don't really know how much those hashes help
02:08heycamemilio: if you'd like to write a style-perf-test that would exercise that, it might be good to have...
02:08emilioheycam: yeah, they're a lot (because there are a lot of dependencies)
02:09emilioheycam: but the thing is how much they help in the real world
02:09emilioheycam: I can add 10k selectors like |.foo * .bar| to the page, and toggle .bar somewhere not affected by .foo, and the diff will be massive
02:10heycamemilio: btw they are used to quickly reject a particular invalidation, before really initializing a bloom filter to test them, is that right?
02:10emilioheycam: not really, or, they shouldn't... Maybe they are! Let me check
02:10heycamemilio: then maybe I misunderstand the purpose of those hashes
02:11emilioheycam: oh, sorry, I misread you
02:11emilioheycam: what do you mean with initializing a bloom filter?
02:12emilioheycam: wait, what... we weren't passing the bloom filter down there
02:12emilioheycam: so that's just useless
02:12emilioheycam: is that what you meant?
02:12emilioheycam: I've been worrying for nothing apparently :-)
02:13heycamemilio: when we're checking whether a particular invalidation is one we care about, we have to selector match stuff to the left, is that right?
02:13emilioheycam: right
02:13emilioheycam: and we _could_ use a bloom filter to check
02:13emilioheycam: but _only_ for the matches_now, not for the snapshot
02:14heycamemilio: well we could have another bloom filter with the snapshotted ancestors?
02:14heycamemilio: anyway
02:14emilioheycam: now we can't
02:14emilioheycam: because of
02:14firebotBug 1380198 FIXED, stylo: reuse the bloom filter and style sharing cache across traversals
02:14heycamemilio: so we're not using a bloom filter, and we are relying on these hashes to do some fast rejection of these invalidations, is that right?
02:14emilioheycam: which makes the filter thread-local, as much as I dislike it
02:14emilioheycam: nope
02:14emilioheycam: we're just not using the hashes
02:14emilioheycam: we're just using the hashes because matches_selector required them
02:15emilioheycam: but taht's just moot
02:15emilioheycam: and I change that in my PR
02:15heycamemilio: so we do a heavyweight match (of the LHS part) for every invalidation we check?
02:15emilioheycam: yes
02:15heycamdon't we have tons of invalidations to check?
02:15emilioheycam: nope, because we store class -> dependency and id -> dependency
02:16heycamemilio: and do you think building a bloom filter would generally outweigh the time saved, because we only have a few invalidations to check?
02:16emilioheycam: I don't
02:17emilioheycam: but I wanted to check it, in order to ensure not regressing anything
02:17emilioheycam: but we weren't using it already anyway
02:17emilioheycam: so there's nothing to regress
02:18emilioheycam: ok, sorry, was distracted... so to recap... We were storing (and computing!) a bunch hashes in the InvalidationMap that were effectively useless
02:18emilioheycam: Also in StyleRules
02:19emilioheycam: we _could_ (theoretically), before, take advantage of those hashes
02:19emilioheycam: but now we can't anyway
02:19heycamemilio: why can't we?
02:20emilioheycam: because the selector is thread-local
02:20emilioheycam: err, the filter
02:20emilioheycam: so we can't use two in the same thread
02:20emilioheycam: I guess we could push the ids/etc of the snapshot into the normal filter...
02:20heycamemilio: so we can't just build one?
02:20emilioheycam: but it doesn't seem worth the effort tbh
02:21emilioheycam: well, the point is that I thought I was removing an optimization that never existed... shrug
02:21heycamemilio: sure, ok.
02:22emilioheycam: now the question is... with these patches the dependency takes only two words... is it worth to do the flattening thing that the third commit does?
02:22heycamemilio: and does Servo not use them either?
02:23emilioheycam: nope
02:24heycamemilio: how many dependencies can we have on an average page?
02:24emilioheycam: a lot
02:24emilioheycam: wikipedia were about 3k
02:24heycamemilio: ok. so the flattening thing would save like 24 KB of memory?
02:25emilioheycam: not really
02:25emilioheycam: the flattening thing may actually be worse in some cases
02:25emilioheycam: it only helps when we insert the same dependency in multiple maps multiple
02:25emilioheycam: otherwise it wastes a word
02:26emilioheycam: I'm going to remove it for now
02:26heycamemilio: I see. so depends how many compound selectors have multiple simple selectors in them
02:26emilioheycam: right
02:26emilioheycam: I think the biggest win here is removing the hashes. Those are 4 words in x86
02:26emilioheycam: _per dependency_
02:29heycamemilio: ok. so ~48 KB on wikipedia on x86
02:29emilioheycam: yes, but that user was OOMing on gmail, which who knows how many selector it has
02:30heycamemilio: I'm a little skeptical it would be the cause of the OOM, but it seems worth doing anyway
02:30emilioheycam: yeah, it's a combination, it just happen to be when inserting on that map, and I looked at it with other eyes :)
02:30heycamemilio: ah ok
02:31heycamemilio: anyway, will see if I get some time this afternoon to review that
02:34emilioheycam: let me just remove the flattening thing, it'll make your life way easier :)
02:34heycamok :-)
02:36emilioxidorn: that patch is completely bitrotten on tip :(
02:37xidornemilio: which patch?
02:37emilioxidorn: fieldset
02:37emilioxidorn: also, why doing it on the display cascade instead of on StyleAdjuster?
02:39xidornemilio: just chose whatever seems to be the easiest way
02:39emilioxidorn: also, coordinate with cam, which is adding something per-pseudo too, so maybe he also has some kind of similar patch
02:39emilioxidorn: he's avoiding the display fixup for every pseudo but before / after
02:40xidornemilio: bug?
02:40emilioxidorn: I think I'd prefer the style adjustment, we already tweak display there a in a bunch of places, so it'd be confusing to have multiple special cases scattered around
02:40firebotBug 1379865 NEW, stylo: don't perform parent display-based style fixups on pseudos except ::before/::after
02:41emilioheycam: ammended btw
02:41* emilio goes to sleep for the day
02:41xidornemilio: so you mean heycam is probably adding something to the style adjuster so that it knows whether it is a pseudo?
02:41xidornemilio: good night
02:42emilioxidorn: probably not, but worth checking
02:42emilioxidorn: he's probably just setting the IGNORE_PARENT_AND_ITEM_DISPLAY_FIXUP_FLAG
02:42emilioxidorn: also, one last thing... ::-moz-fieldset-content is an anon-box, right?
02:42xidornemilio: yes
02:43emilioxidorn: maybe a new flag is just easier... Anon-boxes have a pretty concrete entry point in the Stylist
02:43emilioxidorn: that's the _only_ thing that is going to create an anon-box style
02:44emilioxidorn: so I'm not sure how worth it is to pass the pseudo around... though on tip I guess it's easier
02:47stshineemilio: thanks :) How should I pass PropertyDeclarationBlock to style after guard it with write lock?
02:47emilioxidorn: actually, I guess on tip it's quite easy now... you just need to replace a bunch of /* pseudo = */ None, for implemented_pseudo_element().as_ref() on
02:48emiliostshine: just clone the Arc?
02:48emiliostshine: you're doing now Arc::new(lock.wrap(pdb)), right?
02:48emiliostshine: that'd become just pdb.clone()
02:49stshineemilio: I see, awesome!
02:49emiliostshine: np! :)
02:49* emilio really tries to sleep
02:49xidornemilio: hmmm, ok
02:49xidornhas that been merged to m-c?
02:50emilioxidorn: I think so, yeah
02:50xidornemilio: good
02:50xidornthen I'll just rebase and see
02:52stshineemilio: sleep well :)
02:52emiliothanks! Have a nice day :)
03:04emiliogw: gah, you're fast :P
03:04gwemilio: just beat ya :)
03:46WindowsBunnyCould someone please tell cargo to not include testdata in the published crate for
03:46WindowsBunnybecause 84 out of 86 MB of that crate is literally testdata
03:48WindowsBunnyI'd open an issue but y'all disabled issues on that repo
03:48mbrubeckWindowsBunny: will do
03:49WindowsBunnymbrubeck: thanks <3
03:49WindowsBunnyI&#39;ve decided to poke the crates in my cache with the largest size because they&#39;re all including massive piles of test data Q_Q
03:52mbrubeckWindowsBunny: ^
04:00WindowsBunnymbrubeck: appveyor is enabled on that repo but there&#39;s no appveyor.yml?
04:00WindowsBunnywhat&#39;s the point
04:02mbrubeckWindowsBunny: Servo no longer uses this library; we should probably find a new maintainer if people are still using it.
04:03mbrubeckIt&#39;s been 11 months since its last real commit
04:03WindowsBunnyI don&#39;t use it, I just happened to have it in my cache for some reason
04:03mbrubeckOh wait, Servo does use it
04:03mbrubeckI&#39;m just bad at grep
04:30jdmcrowbot: what&#39;s new?
04:30crowbotMicrosoft Edge tied Servo&#39;s parallel 2D canvas API by an underwhelming amount!!
04:31jdmI bet it did
04:31mbrubeck&quot;tied by <some amount>&quot; is my favorite crowbotism
04:31pcwaltonthats actually something I should check
04:31pcwaltononce I have pathfinder hooked up to canvas
04:33jdmcrowbot: what does the web need?
04:34crowbotjdm: I hear the web payments interest group will be the next big thing (
04:34jdmnext, the web loans interest group
04:40bzjdm: The web sharks group
04:40bzjdm: Then the web jets group.
04:40* jdm was going to make that joke
04:41bzjdm: (of course then our layout team has to implement its specs)
04:41bzjdm: ;)
04:41bzDo or do not, there is no &quot;going to&quot;
04:43WindowsBunnyjdm: Actually, I wouldn&#39;t be surprised if Microsoft suddenly pulled a fast parallel 2D canvas API out of thin air :P
04:44jdmDirect 2D 2Furious
04:50bzxidorn: I&#39;m going to get back to you on that inlinization bug first thing in the morning...
04:50* bz really needs to sleep. :(
04:51xidornbz: ok, thanks
04:52bzxidorn: Sorry. :(
04:52* bz is still trying to catch up on stuff, has too many balls in the air.
04:52xidornbz: no worries
05:02mrobinsongw: Thanks for moving the WR update forward!
05:07gwmrobinson: no worries, thanks for the original PR!
05:07gwmrobinson: fingers crossed it merges first time
05:18mrobinsongw: Yeah. :)
05:22stshinemrobinson: I&#39;m wondering, why your wr update PR succeeded in the first try for webgl but failed afterwards?
05:29gwstshine: I haven&#39;t checked, but perhaps those webgl tests did fail but after / on a different CI machine to the first failures, and weren&#39;t reported?
05:33stshinegw: looks like the explanation! the reported failures are css tests
06:20heycamjyc: I&#39;ll click the button if your PR merges
06:21jycheycam: haha awesome, thanks so much!
06:21jycscared after the last try when we had to back out, was worried the auto merge would complete while I was sleeping and it would break things until I was able to wake up and ask someone to land it in the morning
06:22jycso thanks a lot :P
06:47jeremychenheycam: Doesn&#39;t &quot;retry&quot; just work? or, should I use &quot;try&quot; instead?
06:47crowbotPR #17696: Sync Servo / Gecko image-orientation rounding; pass as enum. -
06:48heycamjeremychen: retry was the right thing
06:48heycamit&#39;s just back in the queue
06:48heycamand the bot doesn&#39;t respond until it gets to the head of the queue and starts a build
06:49jeremychenheycam: oh, i c. i thought the bot would reponse once the PR is back in the queue.tThanks. :)
07:01Manishearthemilio: there?
07:02Manishearthemilio: in the post-fuse world this assert won&#39;t hold, yeah?
07:06Tomcat|sheriffdutyheycam: stylo test bustage :)
07:06Tomcat|sheriffdutycould you take a look ?
07:08bz_sleepManishearth: hmm. That assert is trying to assert that inherited_styles is right, if it was passed
07:09bz_sleepManishearth: Pointer identity happened to be easy to assert and true when I added the assert, but if we can assert something better, that&#39;s fine
07:09bz_sleepManishearth: That said, in the post-fuse world why would this assert not hold?
07:09heycamTomcat|sheriffduty: hmm. that&#39;s from the rustup revert from earlier...
07:09heycamTomcat|sheriffduty: I don&#39;t know what the right thing to do there is
07:10Tomcat|sheriffdutyheycam: do you know who could take a look ?
07:10heycamTomcat|sheriffduty: emilio, though he usually wakes up in an hour or two..
07:10Tomcat|sheriffdutyok :)
07:10Tomcat|sheriffdutyemilio: wake up ! :P
07:11bz_sleepManishearth: OK, sleeping for real, but happy to discuss this tomorrow....
07:12Manishearthbz_sleep: well it&#39;s not holding for me
07:12Manishearthbz_sleep: but I&#39;d assume it&#39;s because we clone the ComputedValuesInner at some point (which bumps the refcounts of the style structs)
07:13Manishearthoh, I know, it could be the restyle stuff
07:13Manishearthbz_sleep: in my patch during restyles and for visited styles we create new contexts
07:14Manishearthin the past they would share the ServoComputedValues allocation but not anymore
07:15Manishearthhowever the restyle stuff will go away soon
07:19Manishearthbz_sleep: I was right!
07:20emilioTomcat|sheriffduty: looking, sorry
07:20Tomcat|sheriffdutyheh :)
07:20Tomcat|sheriffdutygood morning emilio :)
07:20Tomcat|sheriffdutyheycam see the wake up ping for emilio worked ;)
07:20Manishearthemilio: removing the style context duplication during the restyle was easy too :)
07:20emilio &#39;morning ;)
07:21Manishearthit&#39;s great when the &quot;hard&quot; bit of a refactor becomes easy because you split it across smaller refactors
07:21* heycam worries that emilio actually has some irc alarm that wakes him up when pings
07:21ManishearthI&#39;ve considered setting one of those up
07:22Manishearthprimarily for things like work weeks, where I actually *do* need realtime pings from irc
07:22Manishearth(currently I get zero notifications from IRC, I actively have to ssh in and check)
07:22emilioManishearth: cool!
07:22emilioManishearth: that assert should still hold
07:22Manishearthemilio: yep
07:23Manishearthemilio: it was broken bc of the style context duplication
07:23Manishearthnow we only clone SCVs on visited style and there&#39;s not much we can do there
07:23ManishearthI do want to eventually clean up the visited style mess but preferably in a followup
07:24Manishearth(it being a mess isn&#39;t jryans&#39; fault, at that time this tight integration of style contexts didn&#39;t exist so we needed two visited style ptrs. with my changes certain avenues open up for making it nicer. will probably followup them)
07:25emilioTomcat|sheriffduty: can you backout
07:25crowbotPR #17701: reuse the bloom filter and style sharing cache across traversals -
07:26Manishearthbholley: btw, what I just pushed up has the fusing bit done. you may want to test perf. It doesn&#39;t yet reduce the FFI, might do that tomorrow morning (depends on the result of this try, and if I figure out the animation stuff)
07:26emilioTomcat|sheriffduty: what it&#39;s trying to do doesn&#39;t hold so far...
07:27emilioheycam: waking up was total luck btw
07:27Manishearthemilio: btw, if you get time, would love it if you could look at the transitions bug. `fuse` branch on my gecko-dev fork. part 9 is when transitions stop working
07:27Manishearththe fuse branch should compile, but may have new bugs. part 12 and previous should be bug free aside from the animation stuff,
07:28emilioManishearth: bug free, just as the rest of the browser (tm)
07:28emilioManishearth: I&#39;ll try to look, let me take breakfast before :)
07:28Manishearthemilio: yes no bugs here
07:29Manishearthemilio: sure sure take your time. im going to bed, and i have other things I can work on in that patchset tomorrow, so no hurry -- only if you have time
07:31emilioManishearth: cool, rest well :)
07:32SimonSapinnox: hours to find this:
07:33SimonSapin- ${id_set(&quot;INTERNAL&quot;, lambda p: p.experimental)}
07:33SimonSapin+ ${id_set(&quot;INTERNAL&quot;, lambda p: p.internal)}
07:33emilioSimonSapin: lol
07:33emilioSimonSapin: classic
07:34Tomcat|sheriffdutyemilio: ok will try to back this out :)
07:35emilioTomcat|sheriffduty: thanks! I can revert manually if needed if you want
07:35Tomcat|sheriffdutyemilio: oh that would be more awesome
07:35emilioTomcat|sheriffduty: cool, will do
07:35Tomcat|sheriffdutysince i work on cleaning up the other trees at the moment also
07:37noxBackouts sound like a daily thing these days.
07:42SimonSapinI cant decide if this is horrible or amazing
07:59emilioheycam: r? ^
07:59emilioSimonSapin: it&#39;s... well :)
08:00SimonSapinemilio: ?
08:00SimonSapinoh, that code
08:00emilioSimonSapin: yup :)
08:01emilioheycam: wdyt about just doing the variable check in nsStyleContext::CalcStyleDifference? If you think it&#39;s too annoying I think I can r+ your patch, but doing it in nsStyleContext feels like the right solution...
08:02heycamemilio: oh I just replied saying sounds ok
08:02emilioheycam: oh, ok, cool :)
08:27xidornemilio: would we restyle an anonymous box via different paths? or we only do that via get_pseudo_style?
08:31heycamafter trying this morning to reproduce that frame-related crash on youtube without success, I just did so accidentally
08:31heycamwell, it was probably that bug, my crash report doesn&#39;t have symbols, so hard to tell
09:08* Tomcat|sheriffduty kicks servobot
09:10Tomcat|sheriffdutyemilio: still no landing :(
09:14heycamit&#39;s still building/testing
09:17Tomcat|sheriffdutyah ok
09:45heycamTomcat|sheriffduty: ^
11:10noxSo many reverts.
11:23travis-ciServo failed to build with Rust nightly: CC nox, SimonSapin
11:23noxSimonSapin: Am on rustup.
11:23SimonSapinI fixed that already
11:24SimonSapinerror[E0425]: cannot find function `allocate` in module `heap`
11:24SimonSapinnox: was my PR reverted?
11:24noxSimonSapin: The previous rustup?
11:24SimonSapinwhat happened?
11:24noxSimonSapin: Panics at runtime.
11:25SimonSapinin alloc().unwrap()?
11:25crowbotPR #17693: Revert &quot;Auto merge of #17633 - servo:rustup, r=nox&quot; -
11:26SimonSapinstack probes, ok
12:30emiliostshine: why closing it?
12:34emilioxidorn: did you land the ruby inilization patches?
12:35xidornemilio: no
12:35emilioxidorn: what&#39;s missing? I suspect a bunch of the crashes on the frame constructor are due to either that or the multiple sources of handwavyness that bug 1379505 fixes
12:35firebot FIXED, stylo: Isolate style resolution to avoid bugs like bug 1379041
12:35xidornemilio: its dependency is still pending for review from bz
12:36xidornemilio: I don&#39;t believe so... that shouldn&#39;t affect much of frame construction
12:37emilioxidorn: a lot of them are when dynamically removing stuff, but maybe you&#39;re right?
12:37emilioxidorn: I missed that dependent bug, thanks!
12:38xidornemilio: and basically, ruby isn&#39;t that common, I believe...
12:38emilioxidorn: I guess... Though we&#39;ve seen typos that made <br> -> <rb> :D
12:38xidornand I didn&#39;t hear that youtube is using ruby anywhere
12:39xidornemilio: that&#39;s... silly :)
13:08stshineemilio: just a hard base ;)
13:13SimonSapinnox: how is rustup going?
13:14SimonSapinmy other thing is waiting on CI
13:15noxSimonSapin: Adding your changes to the commit and making the PR right now.
13:19SimonSapinemilio: I get &quot;Could not open dynamic library `OsMesa`&quot; when running servo with -z, what am I missing?
13:20emilioSimonSapin: are you on linux?
13:20emilioSimonSapin: and, are you running ./target/debug/servo, not using ./mach run, right?
13:20emilioSimonSapin: ./mach run fixes up the LD_LIBRARY_PATH to look at osmesa-src&#39;s directory
13:20SimonSapinno, mach run
13:20SimonSapinis that wrong?
13:20emilioSimonSapin: well... or it should :)
13:21emilioSimonSapin: you should be able to find a somewhere in your directory
13:21SimonSapinThe following NEW packages will be installed:
13:21SimonSapin freeglut3 freeglut3-dev gperf libgles2-mesa-dev libosmesa6 libosmesa6-dev libssl1.0-dev
13:21SimonSapinafter following README
13:21emilioSimonSapin: well, you can also do that I guess
13:21emilioSimonSapin: that installs it system-wide
13:21SimonSapinnow I get &quot;assertion failed: !context.is_null()&quot;
13:22emilioSimonSapin: that&#39;s a too-old mesa right there I think :)
13:22SimonSapinlatest ubuntu though
13:22emilioSimonSapin: what does glxinfo | grep -i version say?
13:22noxSimonSapin: r? ^
13:22SimonSapinError: unable to open display
13:22emilioAlso, SimonSapin: r? #17713
13:22SimonSapinemilio: this is a headless machine
13:22crowbotPR #17713: style: Kill some style sharing code. -
13:23emilioSimonSapin: oh, I see... What about DISPLAY=:0 glxinfo ...
13:23SimonSapinError: couldn&#39;t find RGB GLX visual or fbconfig
13:23SimonSapinwith DISPLAY=:10 where I have xtightvnc running
13:23SimonSapinnot full xorg
13:23SimonSapinemilio: though -z should remove the need for xorg?
13:24emilioSimonSapin: hmm... well, for xorg maybe, but not for mesa afaict
13:28SimonSapinemilio: ok, so theres two different things. When trying to make servo open a window in my fake-X11-for-VNC I get `Xlib: extension &quot;XFree86-VidModeExtension&quot; missing on display &quot;:10&quot;.` I suppose we require a feature that this X11 server doesnt have. I can manage without opening windows, like with -z -o /tmp/a.png
13:29SimonSapinemilio: which used to work, but now I get &quot;assertion failed: !context.is_null()&quot;. So how can I get a more recent mesa on windows?
13:29emilioSimonSapin: I just use mesa master... I guess there are ppas for ubuntu
13:30SimonSapins/on windows/on ubuntu/
13:47emilionox: killing code is so lovely :)
14:01noxemilio: What did you kill?
14:02emilionox: I killed about 1k lines in #17688, and now about 150 in #17713.
14:02crowbotPR #17688: style: Split style resolution and dynamic change computation. -
14:02crowbotPR #17713: style: Kill some style sharing code. -
14:02emilionox: and all this making the code cleaner!
14:05emilionox: in particular, the giant went from 1700 lines to less than 900... Cleaning that file is something that I had wanted to do for a while
14:20SimonSapinlarsberg: do we have corporate access to paywalled papers? nical was asking for gfx stuff
14:24SimonSapinanyone want to review Mako stuff?
14:24crowbotPR #17703: Reduce amount of Mako-generated code in style -
14:24emilioSimonSapin: i was meant to review it, sorry
14:25emilioSimonSapin: will get to it now
14:25SimonSapinthanks emilio
14:33stshineSimonSapin Why HeapSizeOf is not derived for Locked<T>
14:34SimonSapinstshine: because its usually in Arc anyway, which doest implement HeapSizeOf either
14:34stshineSimonSapin: Hmm, ok thanks
14:34SimonSapinugh, it is implement for Arc but it shouldnt be
14:35crowbotIssue #37: Dealing with shared ownership (Rc and Arc) -
14:35SimonSapinnox: any reason we havent removed `HeapSizeOf for Arc<T>` yet?
14:35noxSimonSapin: I didn&#39;t know it had landed.
14:35noxPlease kill it.
14:36SimonSapinnox: we have an incorrect impl since forever
14:36SimonSapinnox: I dont really want to deal with breaking bumps everywhere
14:38stshineSimonSapin: informative read, but if we wrap a PropertyDeclarationBlock in Locked<> then we lose track of it ...
14:38SimonSapinstshine: isnt it already?
14:38SimonSapinstshine: yes, theres a lot of style stuff that is not track by heapsize because of this
14:40* stshine accepts it
14:41emilioSimonSapin: re., was this error visible? is there any test for it? Or is it fixing a bug you introduced in that same pr?
14:41* emilio can track it down, but it&#39;s less work to just ask
14:41SimonSapinstshine: I think the way forward is an attribute to mark an Arc field as the &quot;main&quot; reference. Ideally each object in Arc has exactly one main reference. But its a bunch of work that nobody has prioritized
14:42SimonSapinemilio: introduced in the same PR
14:42SimonSapinin an earlier commit
14:42SimonSapinbut there other changes in between so squashing is annoying
14:42SimonSapin(changes to the same lines)
14:43SimonSapinemilio: that bug did make gecko try orange (but not servo try)
14:43stshineSimonSapin: I guess make Arc as fast as possible is always the No.1 goal, they are so common
14:43emilioSimonSapin: and, btw, I know those mako things were just moving code around... It just makes me sad :)
14:44SimonSapinemilio: so many things in this code base make me sad, I try to not think too much about it :]
14:47larsbergSimonSapin: nical: No, we do not have anything corporate. I&#39;d recommend for &quot;official archival versions.&quot; If it&#39;s something deep behind expensive elsevier/ieee/etc. paywalls where they want a substantial amount per paper, let me know and one of us who has been hanging on to our academic accounts can see if &quot;know a dude.&quot;
14:47larsbergAnd yes this is ridiculous and part of why I&#39;m pushing for all our research grants and conference funding to be officially limited to open access venues :-)
14:50Tomcat|sheriffdutyxidorn: emilio test bustage
14:50Tomcat|sheriffduty from
14:51emilioSimonSapin: r=me
14:52emilioTomcat|sheriffduty: It needs a test expectation update it seems
14:52emilioTomcat|sheriffduty: should be fixed by
14:52Tomcat|sheriffdutyah missed that !
14:52Tomcat|sheriffdutythanks emilio
14:53emilionp :)
15:10cynicaldevilnox: ping
15:12SimonSapin&quot;mozilla-central is closed for lack of mac symbols/crash data&quot; what happens to servo vcs sync when m-c is closed?
15:12cynicaldeviljdm: What does it mean when it says that the Sender has disconnected: ?
15:13cynicaldevildoes it mean when the Sender goes out of scope?
15:13jdmcynicaldevil: yeah, it means that no senders exist that can communicate with that receiver any more
15:16SimonSapinnox: merged
15:18larsbergSimonSapin: basically, there&#39;s a race condition. If there&#39;s no backout in the servo/ directory, we will be fine and stuff will start landing again once the tree reopens. If there&#39;s a backout in servo/, then there will be a conflict and somebody from the gecko side needs to rebase it atop servo master and then use the tools to open the backout commit on
15:19SimonSapinsounds fun
15:19larsbergThe alternative is blocking github on servo (either directly or indirectly by requiring landing to mercurial) every time m-c is closed.
15:19SimonSapinwait, mozilla-central is closed, not integration/autoland?
15:20larsbergOriginally, we were only going to be syncing to a branch (autoland) that was designed to never be closed, but things have changed w.r.t. moving to an autoland-based model in geckoland. It&#39;s a point of active discussion right now; glob is working on other ideas.
15:24globSimonSapin: you&#39;re right, if we go with &quot;close servo if gecko is closed&quot;, it would be when integration/autoland is closed, not mozilla-central
15:24globobviously i&#39;d love to avoid that
15:51* jack still doesn&#39;t understand why anyone ever told us the tree never closes
15:51stshineemilio: ping about #17690
15:51crowbotPR #17690: layout: Stop in-order traversal on children of InlineFlow -
15:54emiliostshine: pong
15:55stshineemilio: Added a comment :)
15:56emiliostshine: thanks!
15:56stshineemilio: thank _you_
16:06* jdm disappears for a while
16:36bholleyManishearth: yt?
16:37Manishearthbholley: yep
16:37bz_sleepManishEarth: Do you have a few mins to talk about those assertions?
16:37Manishearthbz_sleep: which? the ptreq ones? I fixed those
16:37bholleyManishearth: the PodAssign copy constructor seems pretty wrong to me - am I missing something?
16:37bz_sleepYes, those
16:37bzOK, good
16:37Manishearthbholley: what&#39;s wrong with it?
16:37bzWhat was the fix?
16:38bholleyManishearth: it will just blindly copy memory without addreffing anything?
16:38Manishearthbz: I was still cloning style contexts while restyling
16:38Manishearthbholley: it&#39;s doing a move
16:38Manishearththe memory is mem::forgotten on the rust side
16:38Manishearthand then passed on the stack
16:38Manishearthactually, I should make that into a move maybe
16:38Manishearthnow that the rust side doesn&#39;t have a destructor
16:39Manishearther, the C++ side
16:39Manishearthbholley: the problem is that C++ moves are invalidations, not real moves. in the sense that the destructor will still be called after the move
16:39Manishearthso you need to model the drop flag explicitly
16:40bholleyManishearth: I&#39;m fine with passing in a pointer if we need to, but we should have a much more explicit name to indicate that it&#39;s only safe to call if the caller has forgotten the referent
16:40Manishearthbholley: name for what? the argument?
16:40bholleyManishearth: i.e. just having a ctor that takes ServoComputedValues* and assumes the above is a footgun
16:40bholleyManishearth: not the argument
16:40bholleyManishearth: right now it&#39;s just a constructor
16:41bholleyManishearth: you want something like ServoComputedValues::MoveFrom, or something
16:41Manishearthright now I&#39;ve got a weird leak happening -- I&#39;ll fix that first, since the fix may involve changing how we do the destructor dance
16:52bholleyManishearth: I think the best way to do it is to pass the ServoComputedValues as a UniquePtr (to make it clear that the memory is owned), do the PodAssign inside the ServoStyleContext ctor, and then forget the UniquePtr
16:53bholleyManishearth: though hm, I guess that risks accidentally calling free on the buffer from the caller, which isn&#39;t great
16:54bholleyManishearth: maybe a NewType might be the safest, so that the handshake that the caller forgot the memory is clear
17:10* jdm pulls his hair out about error reporting OOMS in stylo
17:21Manishearthbholley: how do you mem::forget in C++?
17:21bholleyManishearth: for what
17:22Manishearthbholley: well if you want to do the uniqueptr thing you have to forget the uniqueptr
17:22Manishearthi guess I could memcpy null into it
17:23bholleyManishearth: per above I don&#39;t think we should do uniqueptr
17:23bholleyManishearth: (see my subsequent messages and review comments)
17:23bholleyManishearth: I think we should just newtype the raw ptr
17:24bholleyManishearth: and maybe do something like we do with already_AddRefed and trigger an assertion if we haven&#39;t called Take() on it
17:26Manishearthbholley: i have to go for a bit, but do you see any reason why this would leak?
17:27ManishearthI&#39;m looking at the stuff and it seems ok
17:27Manishearthbut there are leaks
17:27Manishearthwhat&#39;s weirder is that those leaks only happen for certain tests
17:28Manishearthspecifically, the tests that were failing because of animations before
17:28Manishearthoh, wait
17:28Manishearthi think i know
18:10fitzgenemilio: shinglyu: before the latest attempt to update bindgen, did nsTArray have a generic param in the bindings? I&#39;m going back as far as v0.25.0 and it never does
18:10* fitzgen is trying 0.22.0 now
18:12emiliofitzgen: we&#39;re using 0.26.1 right now, so it should... And before that I&#39;m pretty sure it had too
18:12fitzgenI didn&#39;t try 0.26.1, but did try 0.26.0 and it didn&#39;t :-/
18:26fitzgenemilio: this is enough to run bindgen when building stylo, right?
18:38emiliofitzgen: yes
18:38emiliofitzgen: bindings are at obj/dist/rust_bindings/style, just in case
19:08fitzgenemilio: where do the fixups come from? it looks like from some toml file, but I can&#39;t figure out which
19:09emiliofitzgen: layout/style/ServoBindings.toml
19:12fitzgenemilio: ok, so there is a replaces for nsTArray; for some reason that was not showing up in my .ii file
19:13fitzgenit looks like `-include <header>` on the command line doesn&#39;t result in that header ending up in the .ii file
19:16emiliofitzgen: huh
19:22fitzgenemilio: huh, it looks like -save-temps also strips comments, which I didn&#39;t notice before
19:22fitzgenso all the <div bindgen ...> get removed
19:34jdmfitzgen: what does bindgen generate for ?
19:37fitzgenwe really need to setup a or something
19:37fitzgenI think ted was working on one
19:42jdmooh, that does sound useful
19:50emiliobholley: got time for a few easy reviews?
19:50emiliobholley: one is to reland your patch, the other one is to be able to land a patch that conflicts with yours after it :)
19:51emiliobholley: I didn&#39;t want to wait for heycam|away since they&#39;re mostly trivial
20:09pcwaltonstandups: Reworking the Pathfinder 2 part