Open Bug 808838 Opened 12 years ago Updated 2 years ago

3D CSS Periodic table demo is unusably slow in Firefox

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Platform:
x86
Windows 7
Type:
defect

Tracking

()

People

(Reporter: taras.mozilla, Unassigned)

References

Details

(Whiteboard: [sps][Snappy:p2])

Attachments

(1 file)

Force 3d transformed layers to be active
7.31 KB, patch
Details | Diff | Splinter Review 
+++ This bug was initially created as a clone of Bug #806256 +++

STR:
http://mrdoob.com/lab/javascript/threejs/css3d/periodictable/

This is still unusable with D2D.

D2D profile:
http://people.mozilla.com/~bgirard/cleopatra/#report=737b3cf85835b2f648bb9798612038db3a9090be
So this is -not- fixed with Direct2D.

And the problem is rather interesting, this site appears to be triggering a DrawWindow, which tries to render with BasicLayers to a D2D surface. That would be fine, except that it's doing 3D transforms, which BasicLayers does by doing a readback, if we were drawing in software, that would be fine, but when drawing with Direct2D it means loads and loads of readbacks.

I have no idea why this is doing so many DrawWindow calls though, what's triggering it? Roc or Matt, can you tell from the page?
Blocks: 806256
No longer blocks: 725936
Something went wrong rebuilding, so I'm still seeing a lot of time in the DrawWindow, but it's not necessarily the cause. Doing a clean rebuild now to test.
BasicLayers will be slow because we flatten the layers in order to get component alpha.

I doubt that does anything useful for 3d transformed layers though.
I've noticed that this demo on Android (HW accelerated) still about 2x slower comparing to any webkit browser, like chrome... 
it feels like we don't operate with pure OGL/D3D layers transformation during that animation and still doing some software stuff, which I guess we should not.

One profile is showing some PaintBoxShadowOuter for every rendered frame...
This forces us to always have an active layer for 3d transformed content.

Unfortunately it makes ChildrenCanBeInactive quite a bit more expensive since we always have to walk all descendants.

Significant performance improvement on this demo with BasicLayers though.
Attachment #678571 - Flags: feedback?(roc)
Note that we need to bubble the LAYER_ACTIVE_FORCE flag up because there is an outer transform wrapping the 3d transformed elements. We end up flattening both.

We could instead transform the 'forced active' flag onto the layers we create, and only flatten if there are layers that have component alpha, but don't have the forced flag set.
So, I've now got a proper build going. There's very little graphics work in the profile, the relevant work is in AlphaBoxBlur, my patches in bug 509052 take care of that mostly. At the top of the profile then I find FrameLayerBuilder::ProcessRemovedDisplayItem, and a bunch of hashtable stuff. Time there seems to be mostly spent in memory access on a quick look, here's the bottom-up list of top contributions:

Function / Call Stack	CPU Time	Module	Function (Full)
mozilla::FrameLayerBuilder::ProcessRemovedDisplayItems	516.888ms	xul.dll	mozilla::FrameLayerBuilder::ProcessRemovedDisplayItems(class nsRefPtrHashKey<class mozilla::FrameLayerBuilder::DisplayItemData> *,void *)
malloc	297.916ms	MSVCR110.dll	malloc
PL_DHashTableEnumerate	273.049ms	xul.dll	PL_DHashTableEnumerate
PeekMessageW	218.629ms	USER32.dll	PeekMessageW
nsRuleNode::GetStyleDisplay	199.230ms	xul.dll	nsRuleNode::GetStyleDisplay(class nsStyleContext *,bool)
[d3d11.dll]	196.612ms	d3d11.dll	[d3d11.dll]
mozilla::gfx::AlphaBoxBlur::BoxBlur_SSE2	168.010ms	gkmedias.dll	mozilla::gfx::AlphaBoxBlur::BoxBlur_SSE2(int,int,int,int,unsigned int *,unsigned int)
SearchTable	156.743ms	xul.dll	SearchTable
nsTHashtable<struct mozilla::plugins::DeletingObjectEntry>::s_EnumStub	148.190ms	xul.dll	nsTHashtable<struct mozilla::plugins::DeletingObjectEntry>::s_EnumStub(struct PLDHashTable *,struct PLDHashEntryHdr *,unsigned int,void *)
Component: Graphics → Layout
Comment on attachment 678571 [details] [diff] [review]
Force 3d transformed layers to be active

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

It's not clear from the bug comments so far what the actual problem is here. I'd like to understand that before we take a step like this.
Attachment #678571 - Flags: feedback?(roc)
OS: Mac OS X → Windows 7
It's not clear what the reported problem is, but my patch fixes a real issue with BasicLayers.

The underlying issue is that we flatten layers that have component alpha, even if flattening them won't actually fix the issue.

With a 3d transform in BasicLayers, we're always drawing into a temporary surface (and then transforming it using pixman), so we don't get subpixel AA text regardless. Having us trigger component alpha flattening and forcing us to redraw the contents of every layer, every paint is undesirable.
I may be pointing out at the obvious, but there seems to be the exact same issue on MacOS X, so this is most likely not DirectX-specific.
Maybe this is linux (with intel GPU) specific, but I'm pretty sure that since this commit I have:

- an impressive FPS boost on http://bartaz.github.com/impress.js/ (that's the good news :) )
- but also a lot of flickering too. Some elements even disappears once the transition is complete (@see for example slide 4, the String «then you should try» is displayed when switching from slide 3 to 4, but turns invisible once «the camera» isn't moving anymore).
I just tried with a fresh profile and it's the same. Playing with xrender or azure parameters doesn't seems to help there. Can anyone reproduce ?
Thanks.
I can reproduce the problems from comment 12 on OS X 10.6.8 in the latest nightly.
What commit? Nothing has been committed here yet.
(In reply to Romain DEP. from comment #12)
> - but also a lot of flickering too. Some elements even disappears once the
> transition is complete (@see for example slide 4, the String «then you
> should try» is displayed when switching from slide 3 to 4, but turns
> invisible once «the camera» isn't moving anymore).

Probably a new bug should be filed for this.
(In reply to Matt Woodrow (:mattwoodrow) from comment #10)
> The underlying issue is that we flatten layers that have component alpha,
> even if flattening them won't actually fix the issue.
> 
> With a 3d transform in BasicLayers, we're always drawing into a temporary
> surface (and then transforming it using pixman), so we don't get subpixel AA
> text regardless. Having us trigger component alpha flattening and forcing us
> to redraw the contents of every layer, every paint is undesirable.

I remember discussing this with you, but I don't remember where that discussion was written down or what the ultimate result was.

ChooseScaleAndSetTransform currently does:
  if (isRetained && (!canDraw2D || transform2d.HasNonIntegerTranslation())) {
    result.mDisableSubpixelAntialiasingInDescendants = true;
  }
Seems to me that if !canDraw2D we should be setting mDisableSubpixelAntialiasingInDescendants to true even if !isRetained. If we did that, then we shouldn't have to flatten in this case.

I remember you had a response to that, but I don't remember what it was :-).
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #16)
> (In reply to Romain DEP. from comment #12)
> > - but also a lot of flickering too. Some elements even disappears once the
> > transition is complete (@see for example slide 4, the String «then you
> > should try» is displayed when switching from slide 3 to 4, but turns
> > invisible once «the camera» isn't moving anymore).
> 
> Probably a new bug should be filed for this.

Filed bug 815040.
> 
> I remember discussing this with you, but I don't remember where that
> discussion was written down or what the ultimate result was.
> 
> ChooseScaleAndSetTransform currently does:
>   if (isRetained && (!canDraw2D || transform2d.HasNonIntegerTranslation())) {
>     result.mDisableSubpixelAntialiasingInDescendants = true;
>   }
> Seems to me that if !canDraw2D we should be setting
> mDisableSubpixelAntialiasingInDescendants to true even if !isRetained. If we
> did that, then we shouldn't have to flatten in this case.
> 
> I remember you had a response to that, but I don't remember what it was :-).

I don't remember what we decided either.

An equivalent to the patch in this bug has already landed though, so this shouldn't be much of a problem any more.

That said, your change looks valid too.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #20)
> So this bug is fixed now?

Not really, on any non-super-fast CPU this demo is still fairly slow. And the vast majority of the time is spent in FrameLayerBuilder::ProcessRemovedDisplayItems. Which, from a quick look at the profile, seems to partially just be very cache unfriendly.
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: