mozilla :: #gfx

9 Aug 2017
12:57katsumm so layers.acceleration.force-enabled is a boolean pref. how does setting it to "disabled" (presumably a string value) work? http://searchfox.org/mozilla-central/source/testing/mozharness/configs/unittests/linux_unittest.py#230
13:03nicallol.
13:05katsprobably it just skips it, http://searchfox.org/mozilla-central/source/modules/libpref/prefapi.cpp#510
13:05katsfalls back to the default value
13:05katsbut then i suspect that they actually meant to set layers.acceleration.disabled=true anyway to force-disable it
13:05katssigh
14:15nicalDo we support clipping text in webrender these days ?
14:15nical(cf #360)
14:37katskvark: any idea why the assertion at https://github.com/servo/webrender/pull/1547/commits/4f1aa9e103fe204bccf289b0979d94b8096fa2d0#diff-d058594280164bb23d9b1ca25ba9fe79R364 might fail?
14:37katsbacktrace is https://treeherder.mozilla.org/logviewer.html#?job_id=121988470&repo=try&lineNumber=1975
14:38kvarkkats: looking...
14:38katsthanks
14:39kvarkkats: do you call renderer.deinit(); at the end?
14:39katskvark: i don't think so
14:39kvarktake a look at the example boilerplate, it's called at the end
14:39katskvark: thanks, will do
14:45katskvark: that seems to fix it locally, thanks
14:48kvarkawesome
15:13nicaljrmuizel: are you going to publish the news letter here: https://mozillagfx.wordpress.com/ ?
15:19jrmuizelnical: that seems appropriate
15:21Gankrorhunt: so I'm probably going to have to implement this regardless, but: do we ever care about a public type that doesn't actually appear in any function signature?
15:25katsoh man i forgot about that blog
15:25katsmstange: do you know what's the status of the profiler on android?
15:27rhuntGankro: Are you also including types used by those public types appearing in function signatures?
15:28Gankrorhunt: yeah anything contained in an outputted type is outputted
15:29rhuntGankro: then yes, I think that's reasonable.
15:30rhuntI don't know why someone would want to use a rusty type that they can't use with the FFI interface or with types used in the FFI interface
15:32nicaljrmuizel: do you have a wordpress account so that I add you as author on the blog?
15:32jrmuizelnical: let me find out
15:33nicalif anybody else want write access to the blog btw let me know
15:37Gankrobotond: C++ question: I have a struct namespace1::X and a typealias for it, namespace2::Y. If I swap it so Y is the struct, and X is the alias, can anyone possibly know or care?
16:09botondGankro: i don't think so (beyond the fact that the relative order of the two declarations will need to change)
16:10GankroCool thanks!
16:10botondGankro: regarding your question from yesterday, let me see if i understand: you want to wrap a pointer into a struct, and make sure the struct is passed with the same calling convention as if it were a pointer?
16:12Gankrobotond: yes; someone pointed out on irc that an enum class: uintptr_t that you static cast would probably work on any platform we care about
16:13botondGankro: ah, neat
16:16Gankrobotond: alternatively we could introduce a coercion to/from *MyStructRef<T>, and use the in function signatures. This would prevent any accidental coercion issues
16:16GankroAnd also allow us to have non-trivial constructors
16:16katsgo go wr update
16:16katsfinally
16:25Gankrokats:
16:25kats:)
16:57nicalThe brand new GFX team logo has been approved by everyone in the paris office
16:58gw280mstange: https://www.kickstarter.com/projects/anywatt/anywatt-smart-adapter-turn-any-cable-into-a-type-c
17:03mstangegw280: interesting, but I don&#39;t need it
18:00katsjrmuizel: are you ok with disabling webrender if HW acceleration is disabled? anybody who&#39;s running WR on linux will need to force-enable acceleration to keep running WR.
18:00jrmuizelkats: I think that&#39;s fine
18:00katsok
18:03mismithkats: I think I need your r+ to land that patch for bug 1387764 on autoland
18:03firebothttps://bugzil.la/1387764 ASSIGNED, lists@spinda.net Perma widget/headless/tests/test_headless.js | application crashed [@ mozilla::gl::GLContextProvider
18:03katsmismith: yup, done
18:03kats(i should have r+&#39;d before commenting, sorry)
18:13katsoh shoot
18:14katsthis means that all linux64-qr tests are going to be disabling webrender, except where HWA was explicitly enabled
18:14katsi.e. the linux64-qr xpcshell tests are now going to be identical to linux64 xpcshell, both running with WR off
18:14katsargh
18:16katsmismith: jrmuizel: we might need a different approach or additional changes after all, if we don&#39;t want to lose test coverage
18:16jrmuizelkats: argh indeed
18:17katsso i think we want to keep the change that was landed, but force-enable HWA on linux64-qr suites
18:17katsthat will reintroduce the headless xpcshell failure, but maybe we can just skip that test for WR
18:34mismithkats: does a user force-enable override a force-disable? if not, headless mode force-disables HW_COMPOSITING which would in turn switch off WR for that test
18:35katsmismith: where is the code that makes headless mode force-disable HW_COMPOSITING?
18:38mismithkats: http://searchfox.org/mozilla-central/source/gfx/thebes/gfxPlatform.cpp#2413
18:41katsmismith: ok, so headless will override everything and block HWA, even if the pref sets it to force-enabled
18:43katsmismith: feature.ForceDisable calls feature.SetFailed which sets the runtime flag in the gfxFeature, http://searchfox.org/mozilla-central/source/gfx/config/gfxFeature.cpp#137 and that takes priority over everything else, http://searchfox.org/mozilla-central/source/gfx/config/gfxFeature.cpp#29
18:44katsso just force-enabling HWA on linux64-qr by setting the pref should be good enough
18:45katsor the envvar instead of the pref, since that&#39;s easier
18:45mismithkats: perfect, that&#39;s what we want in headless mode (otherwise nsBaseWidget::ComputeShouldAccelerate returns true and takes us down a path which requires a valid window context)
18:46katsmismith: and headless mode is only enabled in the one xpcshell, correct?
18:46katsi.e. the other ones run with HWA enabled?
18:46katserr HWA as whatever the default is
18:48mismithkats: as far as i&#39;m aware, yes
18:48katsmismith: ok, great
19:08lsalzmanmilan: you are on a prioritization rampage!
19:24tobytailordholbert: ping
19:42rhuntwhat is FilterNode used for?
19:44lsalzmansvg and css stuff?
19:44dholberttobytailor: pong
19:44dholbertrhunt: svg filters, yeah
19:45dholbertwhich can have multiple stacked filter primitives, hence &quot;node&quot;, IIRC
19:46rhunthuh didn&#39;t know svg could do that
19:46rhuntthanks
19:48* lsalzman fears the day we have a bug report like &quot;I implemented the game of Life in SVG filters, and it&#39;s slow. But it runs fast in Chrome! Fix it!&quot;
19:51tobytailordholbert: 2 questions
19:52tobytailordholbert: is there a way to test for certain build options in the reftest harness? the printing tests should only run if ff is built with skia-pdf support (--enable-skia-pdf)
19:53tobytailordholbert: and the other questions is, can i pass that option to try builds?
19:53dholberttobytailor: sort-of and yes
19:53dholberttobytailor: for the second question: all of the mozconfigs live in tree, so you can modify them as you like
19:53dholberttobytailor: (to customize the build that gets generated on Try)
19:54tobytailorso i would push my own mozconfig?
19:54tobytailorisnt that file ignored?
19:54tobytailorby hg?
19:54milanlsalzman: enjoying every minute of it too ;) I think the rest of 2017 will be &quot;bugzilla for 8 hours a day&quot; type of life for me...
19:54dholberttobytailor: I believe you edit one of the files in build/mozconfig* or something like that
19:54tobytailoric
19:55katsmstange: do you know the status of gecko profiler on android? (i asked earlier, not sure if you saw)
19:55dholberttobytailor: for the first question: we have a variety of variables like &quot;e10s&quot; that you can use in reftest.list -- e.g. skip-if(e10s), fails-if(acceleratedLayers), etc
19:55mstangekats: I didn&#39;t see
19:55mstangekats: the status is that it&#39;s broken and nobody currently cares about it
19:55dholberttobytailor: I believe those are variables that we assign values to in reftest.js or reftest-content.js or whatever
19:55katsmstange: ok
19:56dholberttobytailor: so you would probably need to create a new variable like that
19:57dholberttobytailor: looks like those variables are all of the &quot;sandbox.[STUFF] = [STUFF]&quot; in reftest.jsm
19:57mstangekats: I just forwarded an email to you that may get you something if you put some work into it, but it may also not work at all
19:57mstangekats: it involves building the pre-webextension gecko profiler add-on and some patches
19:57dholberttobytailor: some of which are exposed via &quot;readGfxInfo&quot; it seems. Perhaps that would be the right place to expose skia-PDF status, not sure
19:58tobytailorait, those are great pointers. thanks.
19:58katsmstange: snorp says he has an addon that works, at https://github.com/snorp/fennec-profiler-addon
19:58dholbertsure!
19:58mstangekats: oh cool! I had no idea
19:58mstangeso this is a fennec add-on
19:59dholberttobytailor: (my &quot;acceleratedLayers&quot; mention above was bogus, but we have e.g. layersOMTC and layersOpenGL and other similar variables)
19:59dholbert(sandbox.*)
20:01tobytailordholbert: compiling with skiapdf would set MOZ_ENABLE_SKIA_PDF, could i also check for that?
20:01tobytailori see #ifdef MOZ_ code in reftest/jsm
20:01dholberttobytailor: I don&#39;t know for sure
20:01dholberttobytailor: but maybe
20:02tobytailorok. thx
20:02dholberttobytailor: (if there are other #ifdefs there, then that file is preprocessed, which means hopefully you&#39;re right that you could just check for that)
20:02katsmstange: for future reference i posted snorp&#39;s instructions to https://bugzilla.mozilla.org/show_bug.cgi?id=1388762#c3
20:02firebotBug 1388762 NEW, nobody@mozilla.org large blank spots while scrolling on mobile.twitter.com
20:02mstangekats: thanks
20:02dholberttobytailor: (i.e. set it to true inside of an #ifdef, else set it to false)
20:05pulsebotCheck-in: https://hg.mozilla.org/projects/graphics/rev/71a1b4ed3da4 - Miko Mynttinen - Move more tasks from EnterPresShell()/LeavePresShell() to EnterFrame()/EndFrame()
20:05pulsebotCheck-in: https://hg.mozilla.org/projects/graphics/rev/8bc47df3fad4 - Miko Mynttinen - Make displaylist retaining reftests run only with the pref enabled
20:05pulsebotCheck-in: https://hg.mozilla.org/projects/graphics/rev/e37f58d49fc0 - Miko Mynttinen - Toggle layout.display-list.retain to true (and false on Android)
20:09Baskats: ping
20:10katsBas: pong
20:11Baskats: When is https://dxr.mozilla.org/mozilla-central/source/layout/painting/FrameLayerBuilder.cpp#5907 supposed to get hit? I can&#39;t get it to ever be hit in a debug build and this code-path is very expensive.. (eliminating that For-loop improves DL building times by over 10%)
20:12katsBas: maybe mattwoodrow can answer. this is deep in FLB code which i&#39;m not that familiar with
20:12BasHmm, I think it&#39;s 8 am for him now, probably a little early :)
20:12Basmstange: ping
20:12mstangeBas: pong
20:13Basmstange: Based on the perf work I&#39;m doing now, and how it&#39;s affecting performance, I wouldn&#39;t be surprised if a significant portion of our poor memory accesses come from upwardly iterating over the frame tree (finding some closest ancestor of a frame for some reason or another) during display list building, could you confirm this?
20:13Basmstange: And any chance you might know the answer to the question I posed to Kats above.
20:14mstangeBas: I can&#39;t confirm it but I&#39;ve kind of paused my investigation into DL perf
20:14mstangeBas: did I show you http://tests.themasta.com/twitter-compact/twitter-main.html ?
20:14mstangeit causes continuous display list constructions
20:15BasNope, interesting though :)
20:16mstangeBas: also see https://bugzilla.mozilla.org/show_bug.cgi?id=1379694#c7
20:16firebotBug 1379694 NEW, mstange@themasta.com Try to understand memory access patterns during display list building
20:16Basmstange: I&#39;ve also found a very big weak spot in our display list stress test when profiling actual websites (and why optimizing it is not -super- representative of real websites), a lot of the cost in real websites I&#39;ve profiled specifically comes from having -deep- frame trees rather than broad ones, the 10000 div stress test has a very broad, but not a very deep one.
20:18mattwoodrowBas: Which for loop is expensive? Seems like all of them in that function should terminate pretty quickly
20:18Basmattwoodrow: The one in https://dxr.mozilla.org/mozilla-central/source/layout/painting/FrameLayerBuilder.cpp#5907.. it seems to always just run through all the parents and ends up using the end of the function PredictScaleForContent call.
20:18mstangeBas: would be good to create a testcase that has a deep frame tree, then :)
20:19Basmstange: Indeed :-)
20:20Basmstange: For things like bug 1363922, deep, and then a broad bottom would sort of expose the worst case (in this case it seems to iterate upwards about 20 frames in the tree to find the nsViewportFrame, a lot of times, before my patch)
20:20firebothttps://bugzil.la/1363922 NEW, nobody@mozilla.org spend too much time in GetStyleDisplay() in nsLayoutUtils::GetReferenceFrame in order to snap scroll
20:20mstangeBas: right
20:21mattwoodrowBas: That is surprising
20:21Basmattwoodrow: Yes, I don&#39;t completely understand it either.. maybe the code is broken in some way? This is the Youtube.com front page, not logged in.
20:21mattwoodrowAnyway, try putting a scrollable div within a div that has a scaling transform
20:22Basmattwoodrow: Well, I want to see real websites that hit this, because it needs to be faster. Removing that for-loop improves display-list building times by over 10%.
20:22Bas(on youtube.com)
20:23mstangeBas: that loop runs during layer building, not during display list building, though
20:23mstangeBas: I&#39;ve only been looking at display list building itself
20:23Basmstange: It does run during display list building... it&#39;s called from ScrollFrameHelper::GetScrolledRect
20:23mattwoodrowmstange: Its called from ScrollFrameHelper::GetScrolledRect()
20:23mstangeoops!
20:24mstangeoh dear
20:24Basmstange: for a -lot- of frames, fwiw.
20:24mstangeok, we should rip it out
20:24mstangemattwoodrow: for scroll area snapping, how about we only look at the presshell resolution and not at the layer scale?
20:25mattwoodrowSeems like we could just record what scale we painted at last time if we wanted
20:25mstangethat also seems ok
20:25Bas(Which would explain why it&#39;s 5-10% of DL building, and from my profiling, removing it offers a 10%+ improvement to DL building times, probably by reduction of cache polution)
20:25mattwoodrowEither way, fine with me
20:25Basmattwoodrow: Note that my efforts currently are aimed at reducing first-time paints.
20:25mattwoodrowBas: Ah, well that function will always fail on first paint
20:26mattwoodrowIts looking for retained data from last paint, so walking the frame tree when theres none is pretty pointless
20:27Basmstange mattwoodrow: If you could give me just a pointer or 2 more in the direction of the solution I&#39;m more than willing to write a patch. Note that cnn.com or nytimes.com also -never- seem to hit the early returns in that for loop.
20:27Bas*return
20:27Bas(Like, it won&#39;t hit it on subsequent paints -either-)
20:30Basmstange: So I basically, inside GetScrolledRect just look at the PresShell and its resolution to determine the value of scale?
20:31BasThe code might also just have a bug, considering the early return is never getting hit anyway.
20:31mattwoodrowBas: The early return only gets hit if theres a ContainerLayer between the scrolled frame and the root
20:32mattwoodrowWhich is probably fairly rare for e10s
20:32Basmattwoodrow: Can you think of a website that should have one?
20:33mattwoodrowBas: Nope :( Easy to make one, but I dont know of anything
20:33mattwoodrowAnything with effects (transform, opacity, filters, blending etc) with scrolling within that
20:34Basmattwoodrow: So a div inside a scrollable div you said? I can pretend I understand CSS for a minute to try and create something :)
20:34mattwoodrowA scrollable div within a transformed div
20:35mattwoodrow<div style=transform: scale(2);> <div style=width:200px; height:200px; overflow:scroll> <div style=height:2000px; width:100px; background-color:red></div></div></div>
20:35mattwoodrowSomething like that
20:35mattwoodrowBas: ^
20:38BasHrm, that doesn&#39;t seem to show anything.
20:39BasAh, it&#39;s being scaled out of the div
20:39Bastransform:scale(0.5) did it.
20:40Basmattwoodrow: Well, it&#39;s showing, but the return still isn&#39;t getting hit./
20:40mattwoodrowFor the inner scroll frame?
20:40mattwoodrow:(
20:41Basmattwoodrow: I dunno, I just put some breakpoints in, it looks like on that page, it never finds DisplayItemData on any frame inside GetPaintedLayerScaleForFrame
20:47mattwoodrowI dont know why that is, it should find the data for the nsDisplayTransform
20:48Basmattwoodrow: Should be trivial for you to reproduce :)
20:49BasLet me see what that frame&#39;s data looks like.
20:49botondrhunt: the change milan is making is manual, not automated :)
20:50botondrhunt: the &quot;mechanical&quot; in the bug title just means it&#39;s only making the sorts of changes an automated refactoring might make
20:50rhuntbotond: ah, I was tripped up by &#39;mechanical&#39; :)
20:52Basmattwoodrow: What kind of frame should that be on?
20:52mattwoodrowItll be nsBlockFrame
20:53mattwoodrowif you do frame->DumpFrameTree() it should print them all to stdout (modulo Windows being annoying)
20:53mattwoodrowand will have transformed printed on the right one
20:54Basmattwoodrow: I see a bunch of nsBoxFrame&#39;s, some nsXULScrollFrames, an nsDeckFrame, but no nsBlockFrame
20:55mattwoodrowBas: Is this e10s?
20:55Basmattwoodrow: Yup
20:55mattwoodrowSounds like youre debugging the parent process
20:55mattwoodrowThose are all XUL frame types, not HTML
20:55Basmattwoodrow: Ah yes, I wasn&#39;t before, but now I am, one sec.
20:56BasForgot to attach to the child for this test case.
20:57Basmattwoodrow: Okay, in the right process now, still no early return, but let me show you the frame tree.
20:58Basmattwoodrow: I have an nsBlockFrame now, but it&#39;s DisplayItem array is empty
20:58Bas(DisplayItemData)
20:58mattwoodrowis IsTransformed() true for it?
21:00Basmattwoodrow: Looks like I can&#39;t invoke that without an argument on an nsIFrame
21:00mattwoodrowBas: pass nullptr/0 for it
21:00Basmattwoodrow: pastebin.mozilla.org/9029287
21:00Basmattwoodrow: It&#39;s false for this block frame.
21:01mattwoodrowBas: 0x2b6b20f0 is the transformed block
21:01mattwoodrowId expect PredictScale to be called for 0x2b6b86a0, and then iterate upwards until the transformed block
21:02Basmattwoodrow: I found your bug.
21:02Basmattwoodrow: Look at FrameLayerBuilder.cpp:5889
21:03mattwoodrowThe popup check?
21:03Basmattwoodrow: Oh? Am I out of date already? :P No, the line just after the popup check
21:03mattwoodrowOh, aFrame
21:03mattwoodrowhahaha
21:03Basmattwoodrow: Yup :-)
21:04mattwoodrowCant be that important if nobody noticed that this broke
21:04mstangewhat&#39;s the bug?
21:05katspresumably it should be f-> and not aFrame->
21:05mattwoodrowindeed
21:05Baskats mattwoodrow: Guess who broke that? :)
21:05Baskats mattwoodrow: *runs into a corner*
21:05mattwoodrowWalking up the frame tree, and then checks the DisplayItemData on aFrame each time
21:05Bashttps://hg.mozilla.org/mozilla-central/diff/a2832968581c/layout/painting/FrameLayerBuilder.cpp
21:06BasSo what shall we do, just kill this codepath? I doubt fixing this bug will do much so fix the perf problem itself.
21:06Bas*to
21:06mattwoodrowoops
21:08mstangeoops
21:08mattwoodrowI think grabbing the resolution from the pres shell is worthwhile
21:08mstangeok
21:08mstangethe main reason that GetScrolledRect wants the scale is that it wants to snap correctly if you zoom on Android
21:08Basmattwoodrow mstange: So just do that in the caller&#39;s locations and kill this function entirely?
21:08mattwoodrowYeah
21:09mstangeyeah
21:09BasAlright, I&#39;ll have a patch up soon. This is going to be a good improvement for DisplayList building times. Thanks for the help guys!
21:09mstangeBas: thank you so much for looking at this
21:10mstangethis is all because of the scrolled rect snapping, which I added, and I never got back around to improve its performance
21:10Basmstange: No problem! Happy that I can make some of this perform better :).
21:10mstangeBas: fixing this may also fix https://bugzilla.mozilla.org/show_bug.cgi?id=1293031
21:10firebotBug 1293031 ASSIGNED, mstange@themasta.com 1.26 - 4.81% a11yr / tart / tp5o / tp5o responsiveness / tscrollx (linux64, osx-10-10, windows7-
21:11mstangewhich was a talos regression from https://bugzilla.mozilla.org/show_bug.cgi?id=1012752
21:11firebotBug 1012752 FIXED, mstange@themasta.com Full-page invalidation when scrolling to the very bottom of a page with fractional height
21:11Basmstange: Fingers crossed!
21:11mattwoodrowmstange: Any chance you could look at the two retained-dl refactoring patches?
21:11mattwoodrowI want to try do a m-c merge before I go on PTO next week
21:11mstangemattwoodrow: hopefully later today
21:11mstangeoh!
21:11mstangeok
21:11mattwoodrowthanks!
21:12mstangewhoa that&#39;s a big patch
21:13mattwoodrowSorry :
21:13mattwoodrow:(
21:13mstangeyeah no problem, it kinda has to
21:13mattwoodrowIts all fairly mechanical
21:38mstangeBas: wrong bug number
21:46Basmstange: fixed.
21:46mstangeBas: thanks
22:02BasHrm, silly mozreview obsoleted the first patch
22:03BasARGH! Darn you mozreview
22:03mstangeBas: yeah, you always need to push all patches when you update mozreview requests
22:04mstangethat means they must be consecutive in your hg commit order
22:04Basmstange: Fixed.
23:02mchangmattwoodrow: do you feel ok reviewing texture client updates with OMTP stuff?
23:02mchangdvander on vacation
23:03mattwoodrowmchang: Yeah, for sure
23:03mchangmattwoodrow: cool thanks
10 Aug 2017
No messages
   
Last message: 12 days and 6 hours ago