mozilla :: #servo

13 Sep 2017
00:13bholleyjryans: still fails: https://github.com/bholley/gecko/commits/2ndpass_only
00:14bholleyjryans: if you have a moment to take a look at some point, I'd appreciate it
00:14bholleyjryans: specifically, the 3 failures at https://treeherder.mozilla.org/#/jobs?repo=try&revision=670e99d80bc81641e37ea6debcba7d5cc76eb4cb&selectedJob=130245624
00:14bholleyjryans: (the rev is old, use the github branch above, but the failure mode is the same)
00:14njnManishearth: hmm, ignore_heap_size_of and ignore_malloc_size_of work for structs, but not for enums
00:17bholleyheycam: can you look at getting emilio's reset sharing patch in a landable state today?
00:17bholleyheycam: I think the numbers suggest we should land that + 2ndpass and see how far that gets us
00:17bholleyheycam: I'm working on 2ndpass
00:17* bholley heads off for 30 minutes
00:18heycambholley: sure
00:18heycambholley: ok
00:20njnManishearth: oh, you have to put them on the fields within each variant, not before the tag; cool, that works
00:29hiroxidorn: do you know inDOMUtils.getCSSStyleRules is supposed to work with generated content element?
00:29KWiersojdm: still around maybe?
00:29hiroxidorn: the function has an optional argument |const nsAString& aPseudo|, but
00:30hiroxidorn: we are calling the function with ::before or ::after element without specifying the |aPseudo| in devtools tests.
00:31hiroxidorn: From here. https://hg.mozilla.org/mozilla-central/file/a73cc4e08bf5/devtools/client/inspector/computed/test/browser_computed_pseudo-element_01.js#l26
00:31xidornhiro: I think it is supposed to work with ::before / ::after
00:31heycambholley: looks like I copy/pasted the wrong values into the stylo+cache3 column on the threads=4 sheet. (still, landing emilio's patch seems like the right thing.)
00:32hiroxidorn: yeah, what I am wondering is we don't need to specify |aPseudo| for such cases?
00:33xidornhiro: my guess is that, ::before / ::after has a pseudo "Element" for them, which can be used the same way as using a (element, pseudo) tuple
00:33xidornhiro: well, that test doesn't say anything about getCSSStyleRules at all, does it?
00:33hiroxidorn: OK, thanks.
00:34hiroxidorn: yeah, as far as I looked the stack trace, they calls getCSSStyleRules.
00:34xidornhiro: call that without pseudo?
00:35xidornhiro: is there any specific bug you're debugging?
00:35hiroxidorn: yes, without |aPseudo|. but the |aElement| is a generated content,
00:35hiroxidorn: this, https://bugzilla.mozilla.org/show_bug.cgi?id=1398661#c7
00:35firebotBug 1398661 ASSIGNED, hikezoe@mozilla.com stylo: content property animation does not work without explicit style flush
00:37njnjdm: ping
00:48gwjack: ugh - switching to python3 allows the servo-tidy script to be installed, but servo-tidy isn't python3 compatible :/
00:49njnjdm: I added this to MallocSizeOfOps: pub fn malloc_size_of_non_empty<T: ?Sized>(&self, ptr: *const T) -> usize
00:49jdmnjn: pong
00:49njnthis lets me do `impl<T: MallocSizeOf + ?Sized> MallocSizeOf for Box<T> {`
00:49jdmreasonable
00:50njnbecause the malloc_size_of_non_empty() function doesn&#39;t do the empty check, and so doesn&#39;t have the ?Sized problem
00:50jdm(nox will say that it needs to be unsafe fn
00:50njnwhy?
00:50jdmKWierso: what&#39;s up?
00:50jdmnjn: because you can pass any pointer to it
00:50jdmwhat does jemalloc do if you give it a meaningless pointer?
00:50njnjdm: potentially crashes
00:50jdmhence unafe
00:51njnjdm: it&#39;s not clear to me where the safety boundary should lie here
00:52jdmnjn: actually, what is the difference between self and ptr in the API?
00:52KWiersojdm: I hope I did https://hg.mozilla.org/integration/autoland/rev/f4b0f3105c72dd650ed8f7728cc5602fe2d34f5e correctly for the non-stylo failures like https://treeherder.mozilla.org/logviewer.html#?job_id=130490583&repo=autoland and https://treeherder.mozilla.org/logviewer.html#?job_id=130491371&repo=autoland
00:52njnjdm: sorry, which self and which ptr?
00:52jdmKWierso: good call
00:52jdmnjn: pub fn malloc_size_of_non_empty<T: ?Sized>(&self, ptr: *const T)
00:52jdmwhat is the relationship between the two?
00:53njnjdm: self is MallocSizeOfOps, which is just a struct holding some operations.
00:53njnptr is the pointer you&#39;re measuring
00:53jdmgot it
00:53jdmso yeah, the safety boundary has to lie in the callers here
00:53jdmthey are the ones that have to provide meaningful pointers
00:54njnjdm: so I should make malloc_size_of, malloc_size_of_non_empty, and malloc_enclosing_size_of all unsafe?
00:54jdmyep
00:54njn(have_seen_ptr is safe,though)
00:54njnok
00:55njnis_empty is safe too, I htink?
00:55jdmagreed
01:02njnjdm: it was suggested above that MallocSizeOfOps::new is unsafe, which I don&#39;t udnerstand
01:02jdmnjn: that opinion was retracted
01:02njnoh, ok
01:04bholleyheycam: back
01:04bholleyheycam: yeah bz and I noticed that
01:05heycambholley: I put the right numbers in there
01:05bholleyheycam: ok - so looks like cache3 still doesn&#39;t win on 4 threads
01:06heycambholley: so I&#39;m reviewing emilio&#39;s patch and will make a couple of fixes to it, and I&#39;ll do one more awsy try push once I&#39;m done
01:06bholleyheycam: which would be one of the two main reasons to prefer it (the other being persistence across restyles)
01:06bholleyheycam: sounds good
01:06bholleyheycam: my impression is that emilio&#39;s patch is also less risky, which matters at this point in the cycle
01:06heycambholley: definitely
01:06bholleyheycam: also, we should put the buffer into SharingCache, so that it gets persisted in TLS
01:07heycambholley: so even if I find some bugs and the sharing comes out a bit less than my patch, we should start off by landing emilio&#39;s
01:07bholleyheycam: agreed
01:08bholleyheycam: (you may need to cherry-pick my patch from #18453 from autoland
01:08crowbotPR #18453: Refactor the style cache - https://github.com/servo/servo/pull/18453
01:08bholley)
01:08heycambholley: his patch just uses a HashMap. should clear()ing it preserve its buffer?
01:08* bholley heycam: yes: https://doc.rust-lang.org/std/collections/struct.HashMap.html#method.clear
01:09heycamok
01:10heycambholley: any thoughts on what to do with the style sharing cache priming?
01:10bholleyheycam: (https://hg.mozilla.org/integration/autoland/file/tip/servo/components/style/sharing/mod.rs#l373 is the place to put the map to make it reusable)
01:10bholleyheycam: I&#39;m hoping I can finish the patch to solve the recascade thing today
01:10heycambholley: great
01:10bholleyheycam: and then bz_away will remeasure
01:11jryansbholley: was away for dinner, still need visited help?
01:11bholleyheycam: I asked bz to take the lead on the priming stuff at least for today, since we need to get the reset sharing stuff ready to go and you&#39;re the right person to do that
01:11bholleyjryans: yeah, my most recent ping still applies
01:12heycambholley: sure, was just curious
01:12bholleyheycam: yeah just filling you in
01:12bholleyheycam: if the stars align, bz may be around later, just in time for my recascade patches
01:12jryansok, checking...
01:12bholleyheycam: and we can all pow wow
01:12heycamok
01:14* bholley wonders if that usage of &quot;pow wow&quot; is now frowned upon
01:14bholleyshrug
01:14bholleyjryans: good news is that the patches I linked you to are otherwise green. I expect M-9 to come back orange: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5e01de4113bbdd2c32c226770ef189d197095695
01:15jryansbholley: ah ah, that&#39;s good
01:17njnheycam: I want to measure SpecifiedUrl, but I&#39;m not sure how to handle the Arc and RefPtrs
01:17heycamemilio: (oh, I did find my CascadeData sharing experiment patch...)
01:18njnheycam: actually, the RefPtrs might be more than I can handle now, but the Arc<String> might be nice
01:18* heycam looks
01:19bholleyheycam: also, for the nochrome stuff - wouldn&#39;t it be easier to just encapsulate it in nsLayoutUtils::StyloEnabled ?
01:19* njn also wonders about the Arc<UnparsedValues> fields in PropertyDeclaration and DeclaredValueOwned
01:20heycambholley: yes, I suppose so!
01:23heycamnjn: for the Arc<String> inside the SpecifiedUrl... I guess the owning copy of the string is in the StyleRule / PropertyDeclaration that parsed the url() value
01:24heycamnjn: and each of those should be unique
01:24njnheycam: what/where is that owning field?
01:24jryansbholley: okay, looks like is related to nested links... the inner link of visited inside visited is getting unvisited styles
01:24heycamnjn: it&#39;s just whatever specified value happens to contain a SpecifiedUrl value
01:24jryansbholley: now just trying to parse why...
01:25bholleyjryans: does the general approach in the patch make sense? Happy to explain if that&#39;s helpful
01:25heycamnjn: so inside list_style_image::SpecifiedValue, ...
01:32jryansbholley: i think so! just trying to track down the missing piece. visited is tricky to page back in :(
01:33bholleyjryans: ok great - thanks for helping on this. I&#39;m sprinting on a bunch of stuff so it&#39;s helpful not to page it in myself :-)
01:33jryanshaha :)
01:34* bholley has worked every day straight since August 20th
01:35bz_awaybholley: including weekends? :(
01:35bholleybz_away: yeah
01:35bholley:-(
01:35bz_awaybholley: I hope you plan to take some vacation in a week or two here...
01:35bholleybz_away: I plan to alter my work habits drastically in about 9 days :-)
01:36bz_awaybholley: good
01:37bholleybz_away: heycam: so 2ndpass is green modulo the visited issue that jryans is looking at. I&#39;ve reproduced the recascade blowup, and I think it actually corresponds to something I&#39;d seen before but wasn&#39;t able to isolate. I&#39;m writing up the recascade sharing now
01:38bz_awaybholley: Awesome
01:42njnif the only Gecko-side code changed by a patch is in toolkit/library/rust/Cargo.lock and toolkit/library/gtest/rust/Cargo.lock, can I do a normal Servo PR and then the Cargo.lock changes will be fixed up automatically by the Servo VCS sync service?
01:43njnthat&#39;s what seemed to happen with one of my patches yesterday
01:43njnI went to autoland the gecko-side (Cargo.lock-only) changes, and they&#39;d already been done
01:44heycamone of the best parts about stylo has been how much easier it has been to implement these style system optimizations that we need, because Rust
01:44heycamcan you imagine if we needed to implement this all in C++ in the timeframe we have
01:45bholleyheycam: yeah srsly
01:46bholleyheycam: it&#39;s so rare that we get fuzz bugs in rust code
01:46bholleyheycam: considering all the complex stuff we&#39;re doing
01:46* heycam remembers getting a bunch of fuzzer bugs from all kinds of style system stuff in gecko
01:47bholleyheycam: think about how much time we could save if each one of those annoying compiler errors today was swapped for a fuzz bug tomorrow :-)
01:48heycamheh
01:52njnyou guys sound like an ad for Rust
01:52* jdm records this conversation for his Rust Belt Rust talk
01:53* bholley launches the browser, and his optimization doesn&#39;t work
01:53* bholley angrily withdraws his compliments from Rust
01:53jdmnjn: yes. mozilla-central has auto-revendoring
01:53njnjdm: cool, thanks
01:53njnthat&#39;ll be useful for this patch I&#39;m writing right now
01:55gwjack: around to r? https://github.com/servo/webrender/pull/1697 (or anyone else who knows travis / python things)
01:55crowbotPR #1697: Fix SSL errors on travis CI. - https://github.com/servo/webrender/pull/1697
01:56bholleyoh wait, it does work
01:56bholleyhm
01:59bholleybz_away: so it appears to be working. Still a bit more CVs than initial, I&#39;m guessing it&#39;s because I&#39;m not handling pseudos
01:59* bholley adds that
01:59jryansbholley: going to try a few theories locally... do you know for sure it&#39;s the last patch in the branch affecting visited?
02:00bholleyjryans: 95% certainty
02:00bholleyjryans: in that I&#39;ve been seeing that failure for a while, with various things underneath
02:00jryansok
02:00bholleyjryans: i.e. this patch has gone through a lot of iterations, and I&#39;ve seen that failure across several try pushes, which makes me think it&#39;s something fundamental about the check I&#39;m doing
02:01bholleyjryans: but I haven&#39;t verified directly, so that&#39;s probably worth doing
02:15njnhmm, I&#39;m getting some calls to malloc_size_of_non_empty where the ptr is 0x8
02:15njnjdm: ^^
02:15njnonly place I&#39;m using it is in the MallocShallowSizeOf impl for Box<T>
02:16njnseems weird, I don&#39;t think Box can contain an empty poitner?
02:17bz_awaybtw, I was somewhat surprised the other day that we run memory-reporting code as part of normal operation
02:17bz_awayin case that comes as a surprise to anyone else...
02:19njnbz_away: there&#39;s some image cache sizing that happens, including at startup
02:19bz_awayyeah
02:19bz_awayThat was exactly it
02:19bz_awaySVG image sizing
02:19bz_awayI was just surprised to see my printfs in stylesheet sizeof code trigger at startup. ;)
02:19njnI had the same experience a couple of weeks ago :)
02:33jryansbholley: pretty sure i see the issue, waiting for compile to finish so i can verify...
02:42* bholley -> dinner
03:04jryansbholley: this should fix it, gotta run for now, happy to discuss more tomorrow if needed! https://github.com/jryans/gecko-dev/commit/904d016c20063d1bfcbb54a8d35dacb5e4aabd62
03:18njnheycam: sorry, I didn&#39;t end up understanding what you said about SpecifiedUrl. Is it possible within SpecifiedUrl::size_of() to know whether it&#39;s safe to measure the fields? Or does it depend on where the SpecifiedUrl is stored?
03:18heycamnjn: no, it depends where it&#39;s stored
03:18njndrat
03:19heycamnjn: to be honest I&#39;m not sure where else we end up storing SpecifiedUrl values
03:19heycamnjn: other than in the specified values of properties like list-style-image
03:19heycamnjn: so I don&#39;t know where we are utilizing that sharing of the serialization string
03:20heycamnjn: (do we use the same serialization string in the computed value?)
03:20njnheycam: grep suggests...
03:20njnscript/stylesheet_loader.rs: url: SpecifiedUrl,
03:20njnstyle/properties/longhand/pointing.mako.rs: pub url: SpecifiedUrl,
03:20njnstyle/font_face.rs: pub url: SpecifiedUrl,
03:20njnstyle/stylesheets/loader.rs: url: SpecifiedUrl,
03:20njnstyle/stylesheets/import_rule.rs: pub url: SpecifiedUrl,
03:21heycamnjn: ok, so one case of sharing at least seems to be @import url(). the @import rule will have a SpecifiedUrl, and it might be that the loaded style sheet then stores that same url in it
03:21heycambut I&#39;m not sure
03:21heycamxidorn: may remember the state of what we&#39;re doing with urls better...
03:22xidornheycam: I... don&#39;t :/
03:23xidornheycam: I guess we may share some objects referenced from SpecifiedUrl value with style structs
03:23xidornheycam: and I don&#39;t think we share anything between specified values
03:23xidornheycam: so I suggested that specified value may be the primary reference of those objects
03:24xidornand we should probably skip them when counting memory in style structs
03:24heycamxidorn: ah, I see pub type ComputedUrl = specified::url::SpecifiedUrl
03:25xidornheycam: ComputedUrl isn&#39;t a real problem, the problem is how we convert them to style structs, from gecko&#39;s point of view
03:26heycammmm
03:26heycamxidorn: in any case that Arc<String> won&#39;t be used for the same gecko string object
03:26heycamit&#39;s a Rust String, I mean
03:27xidornthat sounds like something bug 1397971 may be changing
03:27firebothttps://bugzil.la/1397971 NEW, josh@joshmatthews.net stylo: lots of memory used for SpecifiedURLs relating to images for gmail
03:28xidornheycam: but anyway, I think specified value should still be considered to be the primary reference
03:28heycamxidorn: yeah
03:29xidornas far as there is no sharing between specified values
03:29xidornwe should be counting those values from specified value but not anywhere else
03:29heycamxidorn: I think that should be true
03:30xidornyeah, I&#39;m not aware of any sharing between specified values... they should be directly derived from parsing
03:44jackgw: not sure why it&#39;s failing in the osmesa build now, but that python error is gone
03:46njnheycam: I measured SpecifiedUrl from gmail. I see no sharing for `serialization`, huge sharing for `extra_data`, and no sharing for `image_value`
03:47heycamnjn: ok. I think that makes sense for serialization and extra_data.
03:47heycamnjn: I don&#39;t know how image_value is used
03:49njnthere&#39;s build_image_value()
03:50gwjack: yea - I found that if I updated the osmesa version to what Servo is using, it seems to compile...
03:51gwjack: which is super weird - I have no idea what would have changed to somehow make g++ error out on an unknown warning command line parameter! O.O
03:51gwjack: but I think those two changes should get things merging again anyway ...
03:55jdmnjn: could they be Box<[T]>?
03:55jdmbecause that slice can be empty
03:56njnjdm: the stack trace suggested a Box within a Vec, but there may have been elided frames
03:56njnjdm: but it means I&#39;m circling back to the <=256 idea for is_empty
03:57jdmheycam: my patch in bug 1397971 is making us share the rust string with the gecko object
03:57firebothttps://bugzil.la/1397971 NEW, josh@joshmatthews.net stylo: lots of memory used for SpecifiedURLs relating to images for gmail
03:57jdmoh right, xidorn mentioned that
03:58heycamjdm: he did. sounds great :)
03:58njnjdm: that&#39;s the `serialization` field you&#39;re talking about?
03:58jdmnjn: yes
03:59xidornnjn: extra_data is shared between stylesheetinner and all rules inside, so that&#39;s expected
04:00njnjdm: what&#39;s the bug number for your patch?
04:00jdm^
04:00jdmsee last firebot
04:00gwjdm: I vaguely recall that you fixed an issue with zlib on mac builders related to osmesa? Is my recollection correct?
04:00jdmgw: on the servo CI builders, yes
04:01gwjdm: I can see that we have env PKG_CONFIG_PATH=/usr/local/opt/zlib/lib/pkgconfig added to the CI builders. Do you recall if that was the entire fix? Did we also need to breq update zlib or anything like that?
04:01jdmgw: yes, that was the whole fix besides installing zlib from homebrew
04:02gwjdm: ok, thanks
04:02gwjdm: do you know where the change was made that installs zlib?
04:03jdmgw: it was in salt, not travis
04:03jdmhttps://github.com/servo/saltfs/commit/b2a9891ad9f4b138d101933794bd896274ea813d
04:03gwjdm: ahhhh, thanks
04:32njnjdm: the empty pointer might have been Vec<Box<[CustomIdent]>>
04:34jdmthat would make sense
04:42* jdm -> bed
04:43hliebermanHas anyone reported Twitter causing nightly w/Servo to stall out?
04:51hliebermanMmm, actually, appears to be a bad interaction between nightly, stylo, and firejail.
04:51hliebermanWill try to get an mwe
04:58larsberggw: sorry we were offline - Emily and I were at a offsite thing kicking off the social mixed reality stuff. Events just ended :)
04:59gwlarsberg: no worries - I have finally managed to randomly add the correct lines to travis.yml to make it pass again on all platforms :)
05:00larsberggw: lol :)
05:02gwlarsberg: no idea why the default travis python version suddenly started getting SSL errors when it wasn&#39;t previously, and the previously compiling osmesa c++ code started getting internal compiler errors :/ but https://github.com/servo/webrender/pull/1697/files seems to fix them anyway
05:03larsbergTravis did recently do an image upgrade on Linux - it broke rust and they are pinned back right now
05:03gwmight be related to that
05:29bholleyhiro: you there?
05:29hirobholley: yes
05:29bholleyhiro: do you have cycles to take bug 1399349?
05:29firebothttps://bugzil.la/1399349 NEW, nobody@mozilla.org stylo: thread &#39;<unnamed>&#39; panicked at &#39;<html...> has still dirty bit true or animation-only dirty b
05:29hirobholley: sure, I just opened the site.
05:30bholleyhiro: awesome, thanks
05:30hirobholley: np.
05:39gw\o/
05:45njnerror: failed to parse lock file at: /home/njn/moz/mc1/toolkit/library/rust/Cargo.lock
05:46njnthat&#39;s not a very helpful error message :(
05:47* larsberg claps for gw
05:49heycambholley: it feels a little weird to stick the struct cache inside SharingCacheBase. is there a downside/trickiness to just having a separate TLS key for that cache?
05:50bholleyheycam: just because of the name?
05:51heycambholley: yeah, and it&#39;s got a method like clear() on it that should now only clear the shared styles and not shared structs when the depth changes
05:51heycammaybe it&#39;s not a big deal
05:52bholleyheycam: fair. Could use a separate TLS key, but maybe better would be to generalize the type we store into something that could store both
05:52bholleyheycam: either way, doesn&#39;t really matter
05:52heycamok
05:52bholleyheycam: but I guess the clear thing is a good point
05:52bholleyheycam: separate TLS key is also fine, really
05:52bholleyheycam: whatever feels cleanest
05:52bholleyheycam: gotta run
05:52heycambholley: k thx
06:31hiroAnyone in Taipei, can you see a panic when you open https://www.weebly.com/editor/main.php on debug build?
06:31hiroboris or jeremychen?
06:37hirohiro: nvm, I can see the panic locally with a whole sites by Bob Clary.
07:25noxxidorn: https://github.com/dropbox/rust-brotli/issues/8 lol
07:25crowbot1Issue #8: Size of published crate could be smaller - https://github.com/dropbox/rust-brotli/issues/8
07:28noxThe brotli crate is scary.
07:34wcpanheycam: checking ancestors dirty bits does not seems always work for gecko
07:34heycamwcpan: for animated things? or in other cases?
07:35wcpanheycam: :empty + <selector> case
07:36glandiumheycam: would you expect a difference in stylo memory usage between TabsOpen and TabsOpenForceGC?
07:36heycamwcpan: I guess we really need to propagate the LaterSiblings hints, at the same point that we would be doing the snapshot expansion for Servo...
07:37heycamglandium: potentially. maybe a document gets GCed, and drops its ElementData
07:38wcpanheycam: for now propagation for eRestyle_LaterSiblings and/or eRestyle_Subtree happens later right?
07:38heycamwcpan: yes LaterSiblings is expanded during the processing of restyles, as soon as we pick it out of the table of restyles
07:39glandiumheycam: ok, so it&#39;s also not unexpected that it doesn&#39;t change
07:40heycamglandium: yeah
07:40wcpanheycam: i wonder if i can do some lightweight propagation for gecko too ...
07:47heycamwcpan: I wonder if it&#39;s possible to just propagate dirty bits and not actually create new RestyleDatas for the Subtree hints that the LaterSiblings would expand to
07:47heycamwcpan: (that would be a big lighter)
07:48heycamwcpan: or if you&#39;re happy to accept less accuracy, whenever we post a LaterSiblings, we could set some bit on the parent, to say some children might need to be restyled
07:48heycam(if we have a free bit for that)
07:48heycamwcpan: the Gecko style system is only going to be around for another release or two, I guess
07:49heycamwcpan: so it might not be worth trying to get something as good working in Gecko
07:49heycamwcpan: (or at all)
07:50wcpanheycam: if so, maybe we can just check if there is anything remain in RestyleTracker? (i.e. mPendingRestyles.Count() == 0)
07:50heycamwcpan: I think that&#39;s not very different from just flushing styles
07:50heycamsince flushing styles will be cheap if that table is empty
07:52wcpanheycam: ok, so if I add a dirty bit to parent when there is a LaterSiblings hint (in GeckoRestyleManager::PostRestyleEvent maybe?), will it confuse restyle process?
07:53heycamwcpan: if it&#39;s one of the existing dirty bits, then yes it might do
07:53heycamwcpan: I was thinking some different bit
07:53heycamwcpan: but then you&#39;d have to remember to clear it too
08:16emilioheycam: just woke up, but quick question... How can UA sheets be mutated? Aren&#39;t they global + static-ish?
08:18heycamemilio: add-ons can modify any sheets they can get their hands on. although in the WebExtensions world I&#39;m not sure if that&#39;s possible...
08:18emilioheycam: oh I see... That&#39;s annoying
08:19heycamemilio: you could do a try run with a MOZ_CRASH in a few places to check maybe?
08:19heycam(to see if Gecko code is doing so?)
08:19* heycam thought he remembered some case of that
08:19emilioheycam: yup, can do that (I need to have breakfast first though :P)
08:19heycamemilio: so I&#39;m getting your style sharing cache ready for landing, got a few fixes on top of it which I&#39;ll send to you for review
08:20emilioheycam: sounds good. Is it using the hacky refcell? Or your derive? (I think I&#39;d prefer your derive, but nbd)
08:20heycamemilio: decided just to leave your refcell
08:21heycamemilio: can fix later if we hate it too much :)
08:21emilioheycam: hmmm.... Ok
08:22heycam(could probably even just change the uncacheable / font-size / writing-mode fields to atomics)
08:22heycam(which I assume is faster than refcell dynamic checking...?)
08:37xidornnox: yeah, so I really think cargo should do something there...
08:59emilioheycam: if they use relaxed loads it will be faster, but seems a bit overkill, and you need a value to represent none :)
09:00heycamyeah..
09:00heycamanyway, I will push patches in a min
09:00noxemilio: Just noticed we are both above 1k commits now.
09:02emilionox: lol
09:47noxajeffrey: Does https://github.com/servo/servo/blob/master/components/script/microtask.rs#L82 ever return None?
09:53avadacatavraManishearth: you around?
10:14heycamemilio: thanks for the quick reviews. gtg now. try run is still a bit orange in places tho, so will have to fix tomorrow
10:14emilioheycam: np, sounds good, thanks!
10:16emilioheycam: sounds like text-decoration, ::first-line/letter, which I think are mishandled per my comment on the pseudo restrictions commit, and some grid stuff?
10:17emilioheycam: I hate this &quot;reset but not really&quot; properties, like text-decoration or justify-items
10:18heycamemilio: not sure about the grid stuff one. anyway, will take a look tomorrow. these feel like things I already fixed once in my other patch :)
10:18emilioheycam|away: have a nice evening :)
10:26noxcrowbot1: tell pcwalton about https://github.com/sourcelair/xterm.js/pull/938 I just want to hear him rant about how this is due to bad architecture in modern browsers.
10:26crowbot1*sigh*
10:28* nox goes out to lunch.
10:44est31OMG
10:45est31hahaha
10:45est31so vscode uses electron
10:45est31and it just says &quot;screw electron I render using canvas!&quot;
10:45est31lololol
10:57sheldon_knuthwho said that?
11:02est31sheldon_knuth: thats what the link that nox shared said
11:02sheldon_knuthoh I get it
11:02noxest31: Code is surprisingly fast though.
11:03est31its basically creating a font atlas
11:04est31and then draws little images itself
11:04est31using canvas
11:04est31https://github.com/sourcelair/xterm.js/pull/938/files#diff-4ebbb8638fcaf458ea69db6a83f0fef4R230
11:04est31for each single character
11:07est31nox: that&#39;s cool, but its still funny
11:07est31font rendering is literally the first thing that web browsers could do
11:08est31http://info.cern.ch/hypertext/WWW/TheProject.html
11:08noxBut they do tho.
11:11est31either way... this made my day
11:25noxest31: Hah.
11:27emilionox: maybe you can take a look at #18482? I know you don&#39;t know that particular code, but it should be trivial to prove that it does the same :)
11:27crowbot1PR #18482: Share more code for primary style resolution. - https://github.com/servo/servo/pull/18482
11:27noxemilio: Sure.
11:41noxemilio: https://github.com/servo/servo/pull/18482/commits/fbb283af9fe78b3097a0a175e731ce42d124e9e6#diff-3df0058f42702ff630ef857b1dbc65ebR491 What?
11:42noxemilio: map_or(true, no?
12:01emilionox: err, yes
12:01emilionox: should stop writing debug assertions on release builds
12:08emilionox: I ammended it fwiw
12:27noxjdm: Went to bed yesterday thinking &quot;wait, I could just put it in a Rc&quot;.
12:28noxjdm: r? :)
12:54emilionox: do you know if impl Trait is stable already?
12:55noxIt&#39;s not.
13:03noxjdm: r=you?
13:09jdmnox: yes
13:10noxajeffrey: Worklets cannot ever use the file reading task source, right?
13:11jdmaka: can worklets use blob urls
13:11noxjdm: Trying to see if we can put moar stuff in GlobalScope.
13:15avadacatavraif i wanted to analyze stylo code, where would i dl it from
13:15avadacatavrais components/style enough
13:17noxavadacatavra: Analyse how?
13:17avadacatavranox: the unsafe analysis stuff i&#39;ve been doing
13:17avadacatavraneed to run a count lines of code like thing on it
13:17noxavadacatavra: ports/geckolib too.
13:18avadacatavrawill those be sufficient
13:18noxNo, there are also many dependencies, but these two folders are where most of the unsafe is.
13:18avadacatavranox: if dependencies are specified in cargo.toml that&#39;s fine
13:18avadacatavrais that all of the safe code as well
13:20jdmavadacatavra: the other important biggest pieces are components/selectors and the rust-cssparser crate
13:20noxOh right, these.
13:20avadacatavranox: lol
13:20jdmavadacatavra: oh, and components/style_traits
13:21avadacatavrajdm: components/layout?
13:21avadacatavraor just style
13:21jdmavadacatavra: no
13:21jdmjgraham: would it be reasonable to include the screenshot data urls in the WPT error summary
13:21jdmjgraham: that would be so much nicer than having to download a 150mb+ log every time we want to investigate reftest failures
13:27wafflesjdm: we could run another snippet in buildbot that gets the screenshot-related stuff from the log and store it in a JSON
13:27avadacatavrajdm: style_derive?
13:29jdmavadacatavra: that&#39;s a compile-time plugin to generate code for the style crate; I&#39;m not really sure at what point your analysis hooks in
13:29* avadacatavra isn&#39;t sure yet either
13:29avadacatavrai guess if that&#39;s where the code is generated, it wouldn&#39;t fit
13:30avadacatavracode generation problems seem out of scope
13:31jdmavadacatavra: a significant proportion of stylo&#39;s code lives in the generated properties.rs, by the way
13:32jdmbut that&#39;s generated by the crate&#39;s build script (from python templates), rather than by the rust compiler via plugins
13:32avadacatavrajdm: what&#39;s the source code for that?
13:32avadacatavrai do think that would be in scope
13:33jdmavadacatavra: honestly, if you take the result of ./mach build and get properties.rs from the build directory, that&#39;s probably your best bet
13:33avadacatavrajdm: to clarify, just look at properties.rs and then rust-cssparser?
13:33* avadacatavra doesn&#39;t want to get confused
13:34jdmavadacatavra: er, no. I was just pointing out that just looking at the source code in components/style will probably not give you a representative result
13:34avadacatavrajdm: ok thanks
13:34avadacatavraso just append it to the list
13:34jdmright
13:35jdmand maybe exclude the source files that go into it
13:35avadacatavranot sure if you can tell that i know 0 about styling :p
13:35jdmcomponents/style/properties/{longhand,shorthand,helpers,helpers.mako.rs,properties.mako.rs}
13:36jgrahamjdm: The only concern I have is that treeherder is using that data, and including some huge screenshots might be a perf concern
13:36jgrahamjdm: Maybe generate the tbpl-style logs and use those?
13:36jdmjgraham: got a link to one of those logs?
13:36jdmjgraham: and/or what would it take to make an extended summary that includes it?
13:39jgrahamjdm: Extended summary would be pretty easy I think, if you want that. tbpl-style logs are the ones that treeherder uses so e.g.
13:39jgrahamhttps://treeherder.mozilla.org/logviewer.html#?job_id=130619857&repo=try&lineNumber=28713
13:43jdmjgraham: 600kb for that log seems much more reasonable
13:44jgrahamjdm: Well you&#39;re not comparing like-with-like probably
13:45jgrahamBut at some point all these logs were under 50Mb decompressed for gecko because buildbot enforced that. But chunking was needed to make it happen. If you&#39;re downlowding the log for all tests it&#39;s going to be larger, but smaller than the raw log for sure
13:54emilioSimonSapin: r? ^
13:58noxjdm: Can I rename RunnableWrapper to TaskCanceller?
13:58jdmyeah, makes sense
13:58ajeffreynox: hmm, paint worklets can use arbitrary URLs for images, this might include file or blob. But these are loaded by the layout thread, not by the worklet itself, so we may be okay.
13:58noxajeffrey: Saw my earlier PR?
13:58ajeffreyDon&#39;t know about other worklets I&#39;m afraid.
13:59ajeffreynox: only just got into the office!
13:59* ajeffrey goes and looks...
13:59noxajeffrey: TL;DR microtask queues are in GlobalScope now.
13:59ajeffreynox: good!
14:00noxajeffrey: What&#39;s TestWorkletGlobalScope?
14:03jdmnox: it&#39;s a worklet for automated tests
14:03ajeffreyIt&#39;s a test worklet :) https://github.com/servo/servo/blob/master/tests/wpt/mozilla/tests/mozilla/worklets/test_worklet.html
14:03noxAh.
14:03ajeffreynox: hidden behind a pref.
14:03noxYeah saw that.
14:04noxUnfortunate.
14:04noxCan&#39;t we use the paint worklet for tests?
14:04ajeffreynox: tricky because their only effect is visual,
14:04ajeffreyso you need to write them as reftests.
14:05noxajeffrey: So? We can do reftest.
14:05ajeffreyyes, but it&#39;s very brittle,
14:05noxIt&#39;s already brittle.
14:05noxTestWorklet makes WorkletGlobalScope&#39;s code brittle.
14:05ajeffreyyou can easily end up with tests failing for reasons nothing to do with the test.
14:05noxajeffrey: Not if you do a trivial test.
14:05noxLike, painting a red square.
14:06noxAnd I interpret what you are saying as the paint worklet not being tested at all,
14:06ajeffreynox: that&#39;s what the current tests do, but it&#39;s not great.
14:06noxwhich isn&#39;t good either.
14:06jgrahamajeffrey: Is this reftests in general, or something specific to paint worklets?
14:06noxajeffrey: Having additional code just to test that code is worse than brittle.
14:06ajeffreyjgraham: specific to paint worklets
14:06jgrahamIn general reftests are pretty reliable, although with caveats
14:06ajeffreythere&#39;s a lot of machinery sitting behind one of those paints :)
14:06jgraham(mostly around antialiasing, GPUs, etc.)
14:07noxLike this: https://github.com/servo/servo/blob/rc-microtask-queue/components/script/dom/workletglobalscope.rs#L131:L142
14:07jgrahamajeffrey: I am interested to hear more :)
14:07noxWhy are we even warning instead of just panicking?
14:07ajeffreyjgraham: yes, what I&#39;m trying to avoid is having a bug with (e.g.) display list generation causing a test for (e.g.) worklet loading to fail.
14:08jgrahamajeffrey: Like, is it something particular to the servo implementaion or are there fundamental issues here. TestWorkletGlobalScope doesn&#39;t seem like it&#39;s going to work for cross-browser testsing
14:08ajeffreylife is much easier when tests are as self-contained as we can manage.
14:08ajeffreyjgraham: no, this is servo-specific.
14:08noxLife is much easier when we don&#39;t have code that exists only to be tested.
14:08ajeffreyIIRC chrome has its own test worklet.
14:09jgrahamajeffrey: If everyone is converging on a similar solution is there any prospect of standardising it?
14:09jgraham(I agree with nox in general that test-only code is suboptimal)
14:10ajeffreyjgraham: don&#39;t know, this is all a bit wild wild west atm.
14:10jgraham(but I don&#39;t understand the details well enough in this situation to know if this is something that will go away in time or if it&#39;s really necessary. &quot;Can we standardise it&quot; seems like a reasonable test)
14:11ajeffreynox: yes, https://github.com/servo/servo/blob/rc-microtask-queue/components/script/dom/workletglobalscope.rs#L131:L142 is not great, but I can&#39;t see a way round it short of giving every worklet class it&#39;s own thread pool.
14:12noxajeffrey: I&#39;m not sure how that relates.
14:12ajeffreythat code should eventually have branches for animation worklets, etc.
14:12ajeffreynox: the code is there to route a task to its executor,
14:12noxajeffrey: Did you ever document how the code flows?
14:12noxWhy can&#39;t the task contain its executor itself?
14:12ajeffreyand the thread-pool is shared.
14:13ajeffreynox: I&#39;m not a huge fan of passing around Runnables,
14:13noxI am.
14:13noxLet&#39;s change that.
14:13noxhttps://github.com/servo/servo/blob/master/components/script/dom/worklet.rs#L619:L624 this shouldn&#39;t have to be done.
14:13ajeffreymainly for security reasons.
14:13noxWhat.
14:14noxThat sounds like FUD.
14:14noxWhat&#39;s the security issue with Runnable?
14:14ajeffreynox: doing a security audit of code that runs a Runnable is tricky,
14:14noxHow?
14:14ajeffreyyou need to audit every instance of Runnable,
14:14noxYes, why is that an issue?
14:14ajeffreywhich might be in completely separate bits of the code base.
14:14noxThat doesn&#39;t answer my question.
14:15ajeffreyI&#39;m trying to make the worklet code as self-contained as possible,
14:15noxThe fact that tasks are all in the same place makes code comprehension absolutely horrible.
14:15noxajeffrey: Using enums means it&#39;s exactly the opposite.
14:15noxIf I have a task, I want the code for that task at the very place where I queue it.
14:16noxAnd enums means we have the kind of unreachable!() expressions