David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Description • 7 years ago

Advanced Layers is a new D3D11 compositor based on Bas Schouten's "scene graph" idea that he has proposed for some time. Bas did the prototype of Advanced Layers and I've put some work into pushing it over the finish line.

Branch: https://github.com/dvander/gecko-dev/commits/al

What's wrong with the existing compositor?

The existing compositor performs multiple passes over the layer tree to guess occluded and visible regions, and then issues one draw call per layer. Each draw call entirely reconstructs the rendering pipeline. On large layer trees, it is a huge waste of CPU *and* GPU resources. Even on small layer trees, it has technical and design problems. It does not use the GPU to perform occlusion culling, so it can overdraw, which on laptops is bad for memory bandwidth. Design-wise the rendering functionality is pushed too far into device-specific territory, and adding small features means changing three different compositors. Some features (like DEAA) are entirely missing on D3D11. Others (like active layers experiments) are just infeasible due to performance.

Advanced Layers instead performs a single pass over the layer tree, and attempts to group as many rendering operations into a single draw call as possible. Occlusion culling is done on the GPU via a depth buffer. This is all done entirely within the existing layers system. No content side code is affected or changed, and we performed minimal surgery on the texture system. Most of Advanced Layers is isolated to the new LayerManagerMLGPU which implements HostLayerManager.

What are the benefits?

AL shows small wins on modest webpages, mostly since pixel culling happens on the GPU and it issues smaller draw calls. On insane webpages, like the 3D nojs demo, AL shines. CPU time (on my machine) is 10ms and GPU time is 1ms - that's compared to 30ms and 15ms on the existing D3D11 compositor. That's exciting since it means we can experiment with layerizing more primitives and pretty quickly see what the performance characteristics are.

This comes with a caveat: it's not so easy to layerize more primitives. To go down that road we will have to start tackling parts of the codebase that do not scale to large layer trees: LayerTreeInvalidation, APZCTreeManager, AsyncCompositionManager, FrameLayerBuilder, and PLayerTransaction. I believe all of that is possible though, some of it is already on file, and it might also be possible to balance the tradeoff in FrameLayerBuilder.

I am hoping either way that Advanced Layers can eventually replace LayerManagerComposite. An OpenGL port would be straightforward and AL has been designed with that possibility in mind. A software implementation should be possible too: we can lower the burden of complexity by limiting layerization for software. At the very least we should be able to replace the D3D11 compositor.

What's the status?

Advanced Layers is almost green on try - there is a single bc2 crash I'm trying to narrow down, and otherwise it seems to not have any obvious regressions. It passes tests and faithfully works with all of the required features in layers, including APZ, 3d transforms, and plane splitting. It does not use TextLayers, BorderLayers, etc yet - new layerization is considered future work.

What's next?

I'd like to land AL in a few stages:
 1. Precursor Gecko changes that are needed to include AL in the build. Mostly, these are tiny additions to compositor-side classes to expose information AL needs.
 2. The import of the AL code, which is a handful of new .cpp/.h/.hlsl files.
 3. Integration of AL into Gecko, including telemetry, about:support, and memory reporting.
 4. Enable in Nightly. Separate bug, so we can track the TreeHerder/Talos regressions needed to land the pref change.

After a final audit & commit message clean-up I'll start posting patches tomorrow.

	Mayank Bansal
	Comment 1 • 7 years ago

How does this work with WR? Or is it an interim/short-term/quick win, while waiting for WR to get enabled?

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 2 • 7 years ago

TextureSourceProvider doesn't automatically flush pending texture notifications. The compositor does it manually. It's easier to just make it automatic.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 3 • 7 years ago

Advanced Layers has a critical path that tests whether an item can be treated as opaque, and it's better if we can test it directly on the effective transform instead of converting to a 2D matrix first.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 4 • 7 years ago

The compositor has a bunch of extremely useful helpers, like computing the blend backdrop rect, and generating textured triangles, that AL would like to access. This just exposes them in a new header + .cpp file. I also added another helper for generating uv coordinates which is a pretty common operation.

Most of this is code motion and some changes to fix unified build bustage.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 5 • 7 years ago

This exposes some simple ContentTextureHost properties that are needed to composite painted layers.

The existing compositor has a callback on each Layer and Compositable type, effectively making rendering recursive. AL doesn't do this, so it needs more information exposed from types like this.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 6 • 7 years ago

This lets us use safe accessors instead of static_casts. I'm actually not sure why ContentHostTexture has an extra base class, but it does.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 8 • 7 years ago

TextureHosts come from CompositableHosts, so we need lockless methods on those too. This handles the ContentHost case which is easy.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 9 • 7 years ago

This version is for ImageHosts. It's a bit more complicated because ImageHosts select the right image given the current timestamp, and send a notification after composition is done.

ImageHost now exposes a "RenderInfo" object, acquired via ImageHost::PrepareToRender, that returns everything needed to render the current frame. When rendering is done, this object is given back to the ImageHost and it can advance the frame and notify the LayerManager as needed.

ImageHost::Composite (the old compositor's path) now uses this code as well. The image notification stuff has been hoisted into HostLayerManager.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 10 • 7 years ago

BigImageIterators are supposed to hide the fact that they tile over textures, but this is not a useful abstraction for Advanced Layers. AL handles this case by skipping over tiled layers and generating temporary one-off layers to feed as inputs into the rendering algorithm. These one-off layers need a TextureSource.

This patch exposes a new method on BigImageIterator that returns a TextureSource of the current tile.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 11 • 7 years ago

D3D11 will crash if you composite into an iconic window. It's easiest to put this check on CompositorWidget so we don't need some #ifdef XP_WIN grossness.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 12 • 7 years ago

Advanced Layers requires an intermediate surface for every mix-blend container. The alternative was duplicating or bloating all the existing shaders. As we layerize more this decision will probably need to be revisited, especially since AL does not use a pre-allocated texture shared across all temporary render targets.

Anyway, this patch lets LayerManagers override whether blending requires an intermediate surface.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 13 • 7 years ago

I had to audit places that expect HostLayerManager to have a Compositor. Ideally we should just delete this method after these precursor patches are okay'd - I'll see if that's possible.

In this case, we were using the Compositor to get a LayersBackend.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 14 • 7 years ago

We associate a CompositorBridgeParent, via ID, with async compositables. For some reason this ID is stored on the compositor, so Advanced Layers couldn't use it.

This patch moves the ID to the HostLayerManager.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 15 • 7 years ago

Same thing, this moves diagnostic information from Compositor to HostLayerManager. AL does not support DiagnosticTypes so it just ignores the call.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 16 • 7 years ago

This is such a critical part of the compositing process, it deserves an RAII guard.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 17 • 7 years ago

(In reply to David Anderson [:dvander] from comment #7)
> Created attachment 8869200 [details] [diff] [review]
> part 6, lockless TextureSource acquisition

I forgot to post a comment explaining this patch. The compositor's draw calls are scoped to visit a layer, so it made sense to lock, draw, unlock in each Composite method. For AL that doesn't make sense, so we need lockless versions.

AL still has to lock the texture but it does so automatically (whereas the existing TextureHost implementations do it manually). IDXGIKeyedMutexs are automatically queried when textures are bound to the pipeline. If one exists, and isn't already in the locked-textures-set, it's locked and cached. At the end of the frame all locked textures are unlocked. Locked textures are very rare as video and content use a global synchronization object.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 18 • 7 years ago

The existing compositor reallocates and redraws intermediate surfaces when any child changes. We have precise invalid regions for these surfaces, we just throw them away. This patch allows composite layers to retain those regions.

I didn't just store this in the existing mInvalidRegion member since LTI clears that immediately after. Also AL just takes the bounds of the region for simplicity/performance anyway.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 19 • 7 years ago

This should really be named "ScheduleComposite" so this patch renames it. It's in  this queue since LayerManagerMLGPU has an internal Composite method, and static analysis got angry that it wasn't marked as "override".

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 20 • 7 years ago

This just fixes some unified build bustage in WebRender.

	Matt Woodrow (:mattwoodrow)
	Comment 21 • 7 years ago

Comment on attachment 8869195 [details] [diff] [review]
part 3, extract compositor helpers

Review of attachment 8869195 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/wr/WebRenderBridgeChild.cpp
@@ -220,5 @@
>    MOZ_ASSERT((aScaledFont->GetType() == gfx::FontType::DWRITE) ||
>               (aScaledFont->GetType() == gfx::FontType::MAC) ||
>               (aScaledFont->GetType() == gfx::FontType::FONTCONFIG));
>  
> -  RefPtr<UnscaledFont> unscaled = aScaledFont->GetUnscaledFont();

Could probably put these last few changes into the last patch?

	Matt Woodrow (:mattwoodrow)
	Comment 22 • 7 years ago

Comment on attachment 8869212 [details] [diff] [review]
part 9, TextureSources from BigImageIterators

Review of attachment 8869212 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/composite/TextureHost.h
@@ +167,5 @@
>  
> +  // When iterating as a BigImage, this creates temporary TextureSources wrapping
> +  // individual tiles.
> +  virtual RefPtr<TextureSource> ExtractCurrentTile() {
> +    return nullptr;

Could (soft?) assert here, so that we figure out why rendering is broken on new AL platforms quicker.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 23 • 7 years ago

There's some discussion on this in bug 1366618. I'm anticipating that no one will care about this particular case, so this patch lets LayerManagers disable component-alpha when it would require readback. If we decide otherwise it would not be difficult for AL to support this feature.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 24 • 7 years ago

I delayed posting the rest of this bug since I wanted to do a thorough investigation of depth buffer performance. On my Thinkpad and the reference Acer machine, using a depth buffer is slow. Even on desktop it has a small overhead, but it's significant on integrated GPUs. In general it appears to increase GPU draw time by 20-30%, which roughly matches with how many extra writes are occurring.

Granted, that time is asynchronous, and it's very small (2ms on a fullscreen video), but it's sort of the opposite of what we're going for. I tried doing away with ClearDepthStencilView calls, but it didn't help enough - it only about halved the damage.

As a result I've reverted AL to use CPU-based occlusion culling. It turns out to play quite nicely given that AL visits layers front-to-back, and regions work naturally with batched rendering. The depth buffer can still be enabled via a pref, but I'm less convinced it'll be useful now. In most cases we have invalidation to save us. And in very complex cases (like nojs), we were rendering back-to-front anyway in anticipation of adding DEAA, which does not work with z-buffering. We'd have to split all transformed layers into two draw calls, and who knows how well that'll perform.

So tl;dr I think the baseline framework for AL is complete with that matter settled.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 25 • 7 years ago

Bas, this is a current snapshot of Advanced Layers. There's a gfx/doc/AdvancedLayers.md file that describes the current state of things. A few big changes have occurred since the original AL:

 - Tiling is gone. Handling transforms (especially geometry) got too complicated. Instead, batches have a region to track their affected area. For experimenting with huge layer trees, there is also a pref to enable the z-buffer, and that bypasses most of the CPU-bound ordering/occlusion code.
 - Vertex shaders have unit-quad and arbitrary-geometry flavors. That lets us use instancing for most quads, but still support fast plane-splitting.
 - SharedBufferMLGPU now works on Direct3D 11.0, by just allocating one-off buffers. It's quite expensive to do that, so there is a simple buffer cache.
 - Instancing now uses constant buffers pretty aggressively. I'd like to walk back on this, though. It'd be much simpler to shove everything into a separate vertex buffer, and we already do that for complex geometry.
 - Details from the existing compositor have been ported: invalidation, double buffering, wait-queries, etc.
 - Pixel shaders are now much more generic - they all support masking and clipping, for example. I plan to add some ultra-lightweight shaders for video/painted layers since there is a small measurable difference on integrated GPUs.

The biggest thing that's missing is support for "right-sizing" intermediate surfaces. Layout sometimes computes wildly large bounding boxes for containers, and the existing compositor happens to fix it during occlusion analysis. But there is a more complete TODO list in the .md file.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 26 • 7 years ago

These are the bits to make it so you can turn AL on. gfxConfig, DeviceManagerDx, CompositorBridge, GPU process support, etc.

	Bas Schouten (:bas.schouten)
	Comment 33 • 7 years ago

David, do you think turning this on my default for some non-insignificant subset of our users within the 57 timeframe is realistic? (Soft codefreeze for 57 is August 7th I believe)

	Bas Schouten (:bas.schouten)
	Comment 34 • 7 years ago

Comment on attachment 8872539 [details] [diff] [review]
part 20, initial import

Review of attachment 8872539 [details] [diff] [review]:
-----------------------------------------------------------------

Starting off with reviewing the interface. Overall I'm not happy about the re-usal of TextureSource, although I can understand why it might initially make things easier, I think we should plan for its removal though? I would like a low-level wrapper to still exists, (something like MLGTexture2D), historically we have incrementally been needing to lower the level of our wrapper classes, and it's been a waste of time. Can we use an MLGTexture like class internally for TextureSourceMLGPU and progressively kill it later then?

::: gfx/layers/d3d11/MLGDeviceD3D11.h
@@ +2,5 @@
> +* This Source Code Form is subject to the terms of the Mozilla Public
> +* License, v. 2.0. If a copy of the MPL was not distributed with this
> +* file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef mozilla_gfx_layers_d3d11_MLGDeviceD3D11_h 

nit: whitespace

::: gfx/layers/mlgpu/MLGDevice.h
@@ +52,5 @@
> +  virtual gfx::IntSize GetSize() const = 0;
> +  virtual MLGRenderTargetD3D11* AsD3D11() { return nullptr; }
> +
> +  // Create a new texture as a copy of an area in this render target.
> +  virtual RefPtr<TextureSource> CreateCopy(const gfx::IntRect& aRect) = 0;

I don't really like the idea of MLG* having a dependency on the old 'TextureSource' classes.. at this point in the review I'm not sure why we need it. Although I can live with it if need be.

@@ +99,5 @@
> +  // Force a present without waiting for the previous frame's present to complete.
> +  virtual void ForcePresent() = 0;
> +
> +  // Copy an area of the backbuffer to a draw target.
> +  virtual void CopyBackbuffer(gfx::DrawTarget* aTarget, const gfx::IntRect& aBounds) = 0;

This works, although I'd say allowing getting a 'read-only' texture object for the front buffer (and then having a potential user do the copy), allows potential optimizations by users to avoid a wasteful copy. If there's no usecase for that for now this is fine with me though.

@@ +264,5 @@
> +  virtual void SetVSConstantBuffer(uint32_t aSlot, MLGBuffer* aBuffer, uint32_t aFirstConstant, uint32_t aNumConstants) = 0;
> +  virtual void SetPSConstantBuffer(uint32_t aSlot, MLGBuffer* aBuffer, uint32_t aFirstConstant, uint32_t aNumConstants) = 0;
> +
> +  // Set the topology. No API call is made if the topology has not changed.
> +  // The UnitQuad topology implicity binds a unit quad triangle strip as

Why is this special cased and not just a triangle strip Topology with a unit quad vertexbuffer set?

	Milan Sreckovic [:milan] (needinfo for best results)
	Comment 35 • 7 years ago

(In reply to Bas Schouten (:bas.schouten) from comment #33)
> David, do you think turning this on my default for some non-insignificant
> subset of our users within the 57 timeframe is realistic? (Soft codefreeze
> for 57 is August 7th I believe)

Let's worry about the timing separately :)

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 36 • 7 years ago

(In reply to Bas Schouten (:bas.schouten) from comment #34)
> Comment on attachment 8872539 [details] [diff] [review]
> part 20, initial import
> 
> Review of attachment 8872539 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Starting off with reviewing the interface. Overall I'm not happy about the
> re-usal of TextureSource, although I can understand why it might initially
> make things easier, I think we should plan for its removal though? I would
> like a low-level wrapper to still exists, (something like MLGTexture2D),
> historically we have incrementally been needing to lower the level of our
> wrapper classes, and it's been a waste of time. Can we use an MLGTexture
> like class internally for TextureSourceMLGPU and progressively kill it later
> then?
>
> ::: gfx/layers/mlgpu/MLGDevice.h
> @@ +52,5 @@
> > +  virtual gfx::IntSize GetSize() const = 0;
> > +  virtual MLGRenderTargetD3D11* AsD3D11() { return nullptr; }
> > +
> > +  // Create a new texture as a copy of an area in this render target.
> > +  virtual RefPtr<TextureSource> CreateCopy(const gfx::IntRect& aRect) = 0;
> 
> I don't really like the idea of MLG* having a dependency on the old
> 'TextureSource' classes.. at this point in the review I'm not sure why we
> need it. Although I can live with it if need be.

Okay, here's the complete story on TextureSource. I don't like it, but it works and it's expedient. There's three major texture types that flow through layers:
 1. Data buffers from skia: BufferTextureHost
 2. DIBs/HDCs: TextureDIB
 3. ID3D11Texture2Ds from D2D/canvas/video: DXGITextureHostD3D11

There's a lot of code to make all this work and AL would have to duplicate it for very little gain. Data buffers/DIBs would need to reimplement buffer upload and big-image tiling logic. Video/D2D would need new surface descriptor logic. In most cases it'd probably just be a copy+paste. TextureSource also has ancillary features like about:memory reporting. In the end, it happened that TextureSource subsumed all the existing use cases for MLGTexture2D, so I dropped it.

I'd be happy to walk back on that for internal use. As you noted, TextureSource is unnecessary for the render target code, if we have a lower-level abstraction available.

But definitely getting rid of all TextureD3D11 integration would be a big effort and probably needs to happen incrementally.

> @@ +99,5 @@
> > +  // Force a present without waiting for the previous frame's present to complete.
> > +  virtual void ForcePresent() = 0;
> > +
> > +  // Copy an area of the backbuffer to a draw target.
> > +  virtual void CopyBackbuffer(gfx::DrawTarget* aTarget, const gfx::IntRect& aBounds) = 0;
> 
> This works, although I'd say allowing getting a 'read-only' texture object
> for the front buffer (and then having a potential user do the copy), allows
> potential optimizations by users to avoid a wasteful copy. If there's no
> usecase for that for now this is fine with me though.

This is for compositor readback, which I *think* only happens for reftests? So probably not perf-critical, but if I'm wrong, that seems like a fine thing to do.

> 
> @@ +264,5 @@
> > +  virtual void SetVSConstantBuffer(uint32_t aSlot, MLGBuffer* aBuffer, uint32_t aFirstConstant, uint32_t aNumConstants) = 0;
> > +  virtual void SetPSConstantBuffer(uint32_t aSlot, MLGBuffer* aBuffer, uint32_t aFirstConstant, uint32_t aNumConstants) = 0;
> > +
> > +  // Set the topology. No API call is made if the topology has not changed.
> > +  // The UnitQuad topology implicity binds a unit quad triangle strip as
> 
> Why is this special cased and not just a triangle strip Topology with a unit
> quad vertexbuffer set?

Mostly convenience. It lets callers set up the pipeline without having to fish the buffer out of some global cache and construct the bind call (which is a bit verbose). It also makes it easier for MLGDevice to avoid extra API calls. Similar to how shaders are enumerated and compiled internally, vs. externally.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 37 • 7 years ago

This version makes two significant changes over the previous patch, both prompted by some performance testing.

First, batched rendering makes clipping more complicated (we can't scissor or clamp geometry), and this resulted in slower pixel shaders. There was a noticeable difference in GPU draw time on integrated GPUs. 

Now AL has specialized pixel shaders for when clipping can be done cheaply on the CPU. When layers have rectilinear transforms and are not masked, they can use this path. Otherwise they go through full-blown shaders which handle complex transforms, plane splitting, and masking. The only batch type that doesn't have a path like this is mix-blend modes, since they're rare and complicated.

Second, Direct3D 11.1 supports clearing specific regions of a render target with an arbitrary color. It turns out this is much faster than shader-based clears. So now there is a new batch type for solid colors that avoids using shaders entirely.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 38 • 7 years ago

The previous version of this patch was bogus, the region was computed in the wrong coordinate space. This version is simpler: we take the bounds of the invalid region of child layers. If the container geometry itself changed, the whole bounds get invalidated.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 39 • 7 years ago

There's a separate pref for enabling on Windows 7, since Windows 7 doesn't have some newer D3D11 features, and I haven't done any performance testing there yet. This test turns itself off based on both prefs.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 40 • 7 years ago

Comment on attachment 8872542 [details] [diff] [review]
part 23, telemetry integration

Benjamin, this is for Telemetry data review. The new probe is a keyed histogram of Direct3D failure codes.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 41 • 7 years ago

Pretty much same as the previous patch, with some cleanups/bug fixes, doc changes + optional logging.

	Milan Sreckovic [:milan] (needinfo for best results)
	Comment 42 • 7 years ago

Comment on attachment 8872542 [details] [diff] [review]
part 23, telemetry integration

Review of attachment 8872542 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/telemetry/Scalars.yaml
@@ +646,5 @@
> +# The following section contains graphics-related scalars.
> +gfx.advanced.layers:
> +  failure_id:
> +    bug_numbers:
> +      - 9090909

Does this need to be an actual bug number?

@@ +650,5 @@
> +      - 9090909
> +    description: Advanced Layers initialization failure reasons.
> +    keyed: true
> +    kind: uint
> +    expires: never

Do we want to have an actual expiration, so that we keep reviewing?

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 43 • 7 years ago

(In reply to Milan Sreckovic [:milan] from comment #42)
> ::: toolkit/components/telemetry/Scalars.yaml
> @@ +646,5 @@
> > +# The following section contains graphics-related scalars.
> > +gfx.advanced.layers:
> > +  failure_id:
> > +    bug_numbers:
> > +      - 9090909
> 
> Does this need to be an actual bug number?

Yup, thanks for catching that.

> @@ +650,5 @@
> > +      - 9090909
> > +    description: Advanced Layers initialization failure reasons.
> > +    keyed: true
> > +    kind: uint
> > +    expires: never
> 
> Do we want to have an actual expiration, so that we keep reviewing?

I'm not sure - do you mean get the "this value is expiring soon" e-mail so we can decide whether to keep or drop the probe?

	Benjamin Smedberg
	Comment 44 • 7 years ago

Comment on attachment 8872542 [details] [diff] [review]
part 23, telemetry integration

>diff --git a/toolkit/components/telemetry/Scalars.yaml b/toolkit/components/telemetry/Scalars.yaml

> # NOTE: Please don't add new definitions below this point. Consider adding
> # them earlier in the file and leave the telemetry.test group as the last
> # one for readability.
>+
>+# The following section contains graphics-related scalars.

I think you missed the NOTE? Please move this up.

>+gfx.advanced.layers:
>+  failure_id:
>+    bug_numbers:
>+      - 9090909
>+    description: Advanced Layers initialization failure reasons.

For this keyed scalar, you need to describe both 1) the potential values of the key (reference to an existing enumeration is ok) 2) the potential recorded values (what does a particular uint mean?)

>+    keyed: true
>+    kind: uint
>+    expires: never

Do you have a description of how this is going to be monitored long-term so that it provides permanent value? Is this something that you expect to be asking the PI org to help with long-term monitoring (the mission control project)? If there's not a clear monitoring plan yet, please start out with temporary (6 month, 5 release) collection and that will give time to prove the value and build a long-term monitoring plan.

It seems a bit strange that this is valuable enough to collect permanently but isn't opt-out. I'd encourage you to make this opt-out if this is an important user-quality metric.

	Milan Sreckovic [:milan] (needinfo for best results)
	Comment 45 • 7 years ago

(In reply to David Anderson [:dvander] from comment #43)
> ...
> > > +    expires: never
> > 
> > Do we want to have an actual expiration, so that we keep reviewing?
> 
> I'm not sure - do you mean get the "this value is expiring soon" e-mail so
> we can decide whether to keep or drop the probe?

Benjamin already covered this in his comment - I meant that "never" is a long time, and that we should set an expiry to, say 60 or 61, and then review it when that comes about to see if it's still useful.

	Matt Woodrow (:mattwoodrow)
	Comment 46 • 7 years ago

Comment on attachment 8876582 [details] [diff] [review]
part 16, allow retaining invalid container regions

Review of attachment 8876582 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/Layers.h
@@ +2260,5 @@
>      mFilterChain = aFilterChain;
>    }
>  
>    nsTArray<CSSFilter>& GetFilterChain() { return mFilterChain; }
> +  

Whitespace!

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 47 • 7 years ago

Benjamin, data r? again - I made the expiration Firefox 61. It could be that this never turns up anything useful, but if it does, we can make it opt-out then. For now I'll monitor it on the GFX dashboard.

	Benjamin Smedberg
	Comment 48 • 7 years ago

Comment on attachment 8878501 [details] [diff] [review]
part 23, telemetry integration, v2

perfect

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 49 • 7 years ago

DXGITextureHost already does memory reporting, but we can count vertex/constant buffers, and render targets.

	Bas Schouten (:bas.schouten)
	Comment 50 • 7 years ago

Comment on attachment 8876587 [details] [diff] [review]
part 20, initial import v3

Review of attachment 8876587 [details] [diff] [review]:
-----------------------------------------------------------------

This looks pretty good to me, the design document seems pretty solid, there's some things that maybe I'd like to change but we can iterate on these things once we have things landed and working. I also feel longer term it should be possible to retain a bunch of the CPU work we do here, which could possibly change some of the CPU/GPU trade offs. Regardless, this patch brings us to a much better place I feel on the compositor side.

I didn't review the implementation in a great amount of detail, I'm assuming there's no obvious mistakes if this is passing tests, we'll see what hard to find bugs are hidden in here when bug reports come in :-).

::: gfx/ipc/GfxMessageUtils.h
@@ +1370,5 @@
>  
>    static bool Read(const Message* aMsg, PickleIterator* aIter, paramType* aResult) {
>      return ReadParam(aMsg, aIter, &aResult->mUseAPZ)
> +        && ReadParam(aMsg, aIter, &aResult->mUseWebRender)
> +        && ReadParam(aMsg, aIter, &aResult->mUseAdvancedLayers);

Out of curiosity.. isn't this POD? Why are we writing one bool at a time?

::: gfx/layers/mlgpu/BufferCache.cpp
@@ +113,5 @@
> +BufferPool::GetSizeClassFromHighBit(size_t aBit)
> +{
> +  // If the size is smaller than our smallest size class (which should
> +  // never happen), or bigger than our largest size class, we dump it
> +  // in the catch-all "huge" list.

Maybe debug-assert that smaller than the smallest size class never happens?

::: gfx/layers/mlgpu/LayerManagerMLGPU.h
@@ +117,5 @@
> +  bool mDrawDiagnostics;
> +  bool mUsingInvalidation;
> +  nsIntRegion mInvalidRegion;
> +  Maybe<widget::WidgetRenderingContext> mWidgetContext;
> +  

nit: whitespace

	Pulsebot
	Comment 51 • 7 years ago

Pushed by danderson@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/19a188c6dac3
Fix potential memory leak in TextureSourceProviderMLGPU. (bug 1365879 part 1, r=mattwoodrow)
https://hg.mozilla.org/integration/mozilla-inbound/rev/a2149c8d8245
Add IsRectilinear to Matrix4x4. (bug 1365879 part 2, r=mattwoodrow)
https://hg.mozilla.org/integration/mozilla-inbound/rev/469a06f1b8f4
Extract some compositor functions into a LayersHelpers header. (bug 1365879 part 3, r=mattwoodrow)
https://hg.mozilla.org/integration/mozilla-inbound/rev/97812424a41c
Expose ContentTextureHost buffer properties. (bug 1365879 part 4, r=mattwoodrow)
https://hg.mozilla.org/integration/mozilla-inbound/rev/80dad1368a6a
Allow safe downcasting to ContentTextureHost and ImageHost. (bug 1365879 part 5, r=mattwoodrow)
https://hg.mozilla.org/integration/mozilla-inbound/rev/27b360f3b090
Add support for locklessly getting TextureSources from TextureHosts. (bug 1365879 part 6, r=mattwoodrow)
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1e0b28d21ed
Allow locklessly acquiring the TextureSources for ContentHosts. (bug 1365879 part 7, r=mattwoodrow)
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8d52f9103a7
Refactor ImageHost to allow acquiring the current TextureSource without locking the underlying TextureHost. (bug 1365879 part 8, r=mattwoodrow)
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1724f006a12
Add a way to extract TextureSources from a BigImageIterator. (bug 1365879 part 9, r=mattwoodrow)
https://hg.mozilla.org/integration/mozilla-inbound/rev/f563b6086079
Add IsHidden to CompositorWidget. (bug 1365879 part 10, r=rhunt)
https://hg.mozilla.org/integration/mozilla-inbound/rev/6560351cfbcc
Allow LayerManagers to force intermediate surfaces for blend containers. (bug 1365879 part 11, r=mattwoodrow)
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ef12357a83d
Don't require a Compositor for texture backend checks. (bug 1365879 part 12, r=mattwoodrow)
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4f2141f8cad
Don't require a Compositor to track async compositable ownership. (bug 1365879 part 13, r=mattwoodrow)
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1ed5e292651
Don't require a Compositor for setting DiagnosticTypes. (bug 1365879 part 14, r=mattwoodrow)
https://hg.mozilla.org/integration/mozilla-inbound/rev/e41967e4099d
Add an RAII helper for read-unlocking textures. (bug 1365879 part 15, r=mattwoodrow)
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd2d729fd080
Allow ContainerLayers to track their invalid regions. (bug 1365879 part 16, r=mattwoodrow)
https://hg.mozilla.org/integration/mozilla-inbound/rev/f90fe3e970eb
Rename LayerManager::Composite to LayerManager::ScheduleComposite. (bug 1365879 part 17, r=mattwoodrow)
https://hg.mozilla.org/integration/mozilla-inbound/rev/57385512f6ee
Allow LayerManagers to disable complex component alpha cases. (bug 1365879 part 19, r=mattwoodrow)

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 52 • 7 years ago

Landing the precursor patches and actual merge in separate nightlies. (Even though this is all pref'd off, just to be safe.)

Try runs from that push:
 https://treeherder.mozilla.org/#/jobs?repo=try&revision=bddebc4d9a7cf73b107dbacaef2be3daee479e63
 https://treeherder.mozilla.org/#/jobs?repo=try&revision=93a7f2df10f1b3ca593a6f5d51043fa762abeb21

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 53 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/19a188c6dac3
https://hg.mozilla.org/mozilla-central/rev/a2149c8d8245
https://hg.mozilla.org/mozilla-central/rev/469a06f1b8f4
https://hg.mozilla.org/mozilla-central/rev/97812424a41c
https://hg.mozilla.org/mozilla-central/rev/80dad1368a6a
https://hg.mozilla.org/mozilla-central/rev/27b360f3b090
https://hg.mozilla.org/mozilla-central/rev/b1e0b28d21ed
https://hg.mozilla.org/mozilla-central/rev/c8d52f9103a7
https://hg.mozilla.org/mozilla-central/rev/f1724f006a12
https://hg.mozilla.org/mozilla-central/rev/f563b6086079
https://hg.mozilla.org/mozilla-central/rev/6560351cfbcc
https://hg.mozilla.org/mozilla-central/rev/9ef12357a83d
https://hg.mozilla.org/mozilla-central/rev/a4f2141f8cad
https://hg.mozilla.org/mozilla-central/rev/f1ed5e292651
https://hg.mozilla.org/mozilla-central/rev/e41967e4099d
https://hg.mozilla.org/mozilla-central/rev/fd2d729fd080
https://hg.mozilla.org/mozilla-central/rev/f90fe3e970eb
https://hg.mozilla.org/mozilla-central/rev/57385512f6ee

	Pulsebot
	Comment 54 • 7 years ago

Pushed by danderson@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8069471cda21
Initial import of Advanced Layers. (bug 1365879 part 20, r=bas)
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d0048e04446
Add MLGPU feature bits and compositor initialization. (bug 1365879 part 21, r=mattwoodrow)
https://hg.mozilla.org/integration/mozilla-inbound/rev/be62d3eb3b3b
Add Advanced Layers to the compositor line in about:support. (bug 1365879 part 22, r=milan)
https://hg.mozilla.org/integration/mozilla-inbound/rev/35129e4e204b
Add Telemetry environment and failure tracking for Advanced Layers. (bug 1365879 part 23, r=milan, data_r=bsmedberg)
https://hg.mozilla.org/integration/mozilla-inbound/rev/f05aa479a40e
Add an acceleration test for Advanced Layers on Windows. (bug 1365879 part 25, r=milan)
https://hg.mozilla.org/integration/mozilla-inbound/rev/20203c94ef23
Add memory reporting. (bug 1365879 part 26, r=mattwoodrow)

	Gian-Carlo Pascutto [:gcp]
	Comment 55 • 7 years ago

At least one of these was committed with the wrong bug number: https://hg.mozilla.org/integration/mozilla-inbound/rev/ab715ee9ef99

	Carsten Book [:Tomcat]
	Comment 56 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/8069471cda21
https://hg.mozilla.org/mozilla-central/rev/9d0048e04446
https://hg.mozilla.org/mozilla-central/rev/be62d3eb3b3b
https://hg.mozilla.org/mozilla-central/rev/35129e4e204b
https://hg.mozilla.org/mozilla-central/rev/f05aa479a40e
https://hg.mozilla.org/mozilla-central/rev/20203c94ef23

	Carsten Book [:Tomcat]
	Comment 57 • 7 years ago

https://hg.mozilla.org/mozilla-central/rev/ab715ee9ef99b92f8a47035f8b2538893a413c41 had a wrong bug number in it, so commenting here :)

	Pulsebot
	Comment 58 • 7 years ago

Pushed by cbook@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/671f39850752
Backed out changeset 20203c94ef23 
https://hg.mozilla.org/mozilla-central/rev/0ead1fd2e1d4
Backed out changeset f05aa479a40e 
https://hg.mozilla.org/mozilla-central/rev/b6153802d9d3
Backed out changeset 35129e4e204b 
https://hg.mozilla.org/mozilla-central/rev/db067dba1a21
Backed out changeset be62d3eb3b3b 
https://hg.mozilla.org/mozilla-central/rev/120d49a127d6
Backed out changeset 9d0048e04446 
https://hg.mozilla.org/mozilla-central/rev/a9bc98726bd7
Backed out changeset 8069471cda21 
https://hg.mozilla.org/mozilla-central/rev/2277b567b242
Revert merge conflict resolution to fix bustage a=me

	Carsten Book [:Tomcat]
	Comment 59 • 7 years ago

Hi, had to back this out since this doesn't played well with a other change comming from autoland and so caused https://treeherder.mozilla.org/logviewer.html#?job_id=109510911&repo=mozilla-inbound

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 60 • 7 years ago

(In reply to Carsten Book [:Tomcat] from comment #59)
> Hi, had to back this out since this doesn't played well with a other change
> comming from autoland and so caused
> https://treeherder.mozilla.org/logviewer.html#?job_id=109510911&repo=mozilla-
> inbound

Hrm, I don't see any merge conflict locally. I thought it might be unified build bustage, but I just rebased and pushed to try and nothing seems amiss. I'll try landing again this afternoon.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 61 • 7 years ago

(In reply to David Anderson [:dvander] from comment #60)
> (In reply to Carsten Book [:Tomcat] from comment #59)
> > Hi, had to back this out since this doesn't played well with a other change
> > comming from autoland and so caused
> > https://treeherder.mozilla.org/logviewer.html#?job_id=109510911&repo=mozilla-
> > inbound

Now I'm more confused. Somehow the backout left files sitting in m-i but not m-c.

	Pulsebot
	Comment 62 • 7 years ago

Pushed by danderson@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f8851a5c8a67
Remove leftover files from incomplete backout of bug 1365879 (r=me).

	Pulsebot
	Comment 63 • 7 years ago

Pushed by danderson@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc9e9c6d7e63
Initial import of Advanced Layers. (bug 1365879 part 20, r=bas)
https://hg.mozilla.org/integration/mozilla-inbound/rev/63d3975c2fad
Add MLGPU feature bits and compositor initialization. (bug 1365879 part 21, r=mattwoodrow)
https://hg.mozilla.org/integration/mozilla-inbound/rev/635acc39356b
Add Advanced Layers to the compositor line in about:support. (bug 1365879 part 22, r=milan)
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b38a58a7b56
Add Telemetry environment and failure tracking for Advanced Layers. (bug 1365879 part 23, r=milan, data_r=bsmedberg)
https://hg.mozilla.org/integration/mozilla-inbound/rev/5338b36353ad
Add Advanced Layers to the reftest sandbox. (bug 1365879 part 24, r=mattwoodrow)
https://hg.mozilla.org/integration/mozilla-inbound/rev/5aa4eaeca36a
Add an acceleration test for Advanced Layers on Windows. (bug 1365879 part 25, r=milan)
https://hg.mozilla.org/integration/mozilla-inbound/rev/b8fa3354a96b
Add memory reporting. (bug 1365879 part 26, r=mattwoodrow)

	Carsten Book [:Tomcat]
	Comment 64 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/fc9e9c6d7e63
https://hg.mozilla.org/mozilla-central/rev/63d3975c2fad
https://hg.mozilla.org/mozilla-central/rev/635acc39356b
https://hg.mozilla.org/mozilla-central/rev/6b38a58a7b56
https://hg.mozilla.org/mozilla-central/rev/5338b36353ad
https://hg.mozilla.org/mozilla-central/rev/5aa4eaeca36a
https://hg.mozilla.org/mozilla-central/rev/b8fa3354a96b