This will avoid having to do an allocation and two copies

The basic idea is here it just needs polished up into something that builds.

Jeff, can you take this? If not, can you reassign it to someone else on your team who may be able to help us out here? Marking as P1 since this will be instrumental to fixing a major TART regression (bug 907546).

No short term help from the graphics team that we can commit to. Unassigning to not give false hope.

Updating the progress we made.

Comment on attachment 797285 [details] [diff] [review] Buggy WIP Review of attachment 797285 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/thebes/gfxWindowsSurface.cpp @@ +37,5 @@ > +gfxWindowsSurface::gfxWindowsSurface(IDirect3DSurface9 *surface, uint32_t flags) : > + mOwnsDC(false), mForPrinting(false), mWnd(nullptr) > +{ > + mDC = 0; // GetDC will fail > + Init(cairo_win32_surface_create_with_d3dsurface9(surface)); This patch disables most of the use of the Direct3DSurface9 and just gets the DC. This should be nearly equivilant to the old code but this fails. This needs to be debug before re-enabling the D3DSurface9 codepaths.

The above two comment should of been made under my account.

mconley can you get us some performance numbers for this patch see if it helps you?

I'll do some local measurements as well, but for thoroughness, I've done some try pushes: m-c baseline: https://tbpl.mozilla.org/?tree=Try&rev=aa679ad1ed19 m-c with patch: https://tbpl.mozilla.org/?tree=Try&rev=71b0f1d88072 UX baseline: https://tbpl.mozilla.org/?tree=Try&rev=f2824c8ebafe UX with patch: https://tbpl.mozilla.org/?tree=Try&rev=6ef776e4bdf8

Results are in - here's the data: For m-c, this is a comparison of the TART measurements from baseline to m-c with the patch (green is indicates a performance improvement from the patch, red represents a regression): http://compare-talos.mattn.ca/breakdown.html?oldTestIds=29010917,29011681,29011687,29011709,29011733,29011745,29011759&newTestIds=29010861,29011545,29011577,29011583,29011595,29011607,29011629&testName=tart&osName=Windows%20XP&server=graphs.mozilla.org Here's the same thing for UX - green = good, red = bad: http://compare-talos.mattn.ca/breakdown.html?oldTestIds=29010947,29011657,29011693,29011703,29011715,29011721,29011727&newTestIds=29010931,29011551,29011565,29011571,29011601,29011613,29011635&testName=tart&osName=Windows%20XP&server=graphs.mozilla.org And here's a comparison between m-c with patch, and UX with patch (where m-c with patch is "Original" and UX with patch is "New"): http://compare-talos.mattn.ca/breakdown.html?oldTestIds=29010861,29011545,29011577,29011583,29011595,29011607,29011629&newTestIds=29010931,29011551,29011565,29011571,29011601,29011613,29011635&testName=tart&osName=Windows%20XP&server=graphs.mozilla.org Which seems to be an improvement over m-c without the patch, and UX without the patch: http://compare-talos.mattn.ca/breakdown.html?oldTestIds=29010917,29011681,29011687,29011709,29011733,29011745,29011759&newTestIds=29010947,29011657,29011693,29011703,29011715,29011721,29011727&testName=tart&osName=Windows%20XP&server=graphs.mozilla.org The data is a little noisy, but here are some thoughts: 1) The "error" measurements have always been a little volatile, so it's not surprising that we're seeing them jump around a bit here. For those who don't know, "error" is an absolute measurement of how many milliseconds the actual transition time differs from the duration specified in the CSS. 2) This patch appears to be a win for both m-c and UX, but these measurements suggest that UX gets a little bit of an advantage on some of the measurements. Notably, UX does better on the "fade" tests, which exercises / measures the animation only, without adding or removing tabs or doing any of the other stuff that tabbrowser.xml does when opening and closing tabs. TL;DR: We should see a general TART improvement overall, but we'll likely see a small win for Windows XP in the fade tests for UX. This doesn't kill the UX branch TART regression, but it's definitely a step in the right direction. So I think this is a big thumbs up from me. Feel free to contradict my reading - I'm totally open to hearing other interpretations.

And by "small win" for the fade tests, I should have been more clear - I expect this to essentially neutralize the regression on the fade measurements.

These look great. I noticed a small problem causing the scrollbars not rendering. I'll see if I can find why.

(In reply to Benoit Girard (:BenWa) from comment #12) > These look great. I noticed a small problem causing the scrollbars not > rendering. I'll see if I can find why. I suspect this could be caused by a broken GetDC implementation.

On the gfxWindowSurface? That would explain it. I'll take a look.

This looks great! It appears that the patch solidly improves performance both on m-c and on ux, and ux gains slightly more from it. This means that when we check ux TART regressions compared to m-c, it would only very slightly improve, regardless of the fact that this appears to be a great all-around win. As for the .error values, while I wouldn't call them "volatile" (as they're pretty stable), they do indeed represent a different thing (as Mike described) than the average intervals which are .half/.all . So I wouldn't worry too much about them as long as they (or the regression compared to m-c) don't get into absolute ranges of ~10-20ms (which means the animation took another frame to complete). The specific absolute values we're seeing in the details from comment 10 are generally sub 2ms, and the difference from m-c is typically less than 1ms. This is totally within noise level of these values and means ux is pretty much identical to m-c on these .error values.

I ran with glass and found more wasn't rendering. I implemented GetDC but it didn't work and its hard to debug while traveling. I'll have to debug it Tuesday. Until then the number may not be trustworthy.

(In reply to Benoit Girard (:BenWa) from comment #16) > I ran with glass and found more wasn't rendering. I implemented GetDC but it > didn't work and its hard to debug while traveling. I'll have to debug it > Tuesday. Until then the number may not be trustworthy. Sure - happy to run the numbers again when the next patch comes in.

Fixed the problem

Comment on attachment 799071 [details] [diff] [review] patch v2 https://tbpl.mozilla.org/?tree=Try&rev=68e4941baab8

Thanks - patch pushed to Try based on m-c: https://tbpl.mozilla.org/?tree=Try&rev=dff890def885 and based on UX: https://tbpl.mozilla.org/?tree=Try&rev=a780ad91b47c I'll toss up the breakdown tonight.

Comment on attachment 799071 [details] [diff] [review] patch v2 Review of attachment 799071 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/cairo/cairo/src/cairo-win32-surface.c @@ +522,5 @@ > + if (hr) { > + IDirect3DSurface9_GetDC (surface->d3d9surface, &surface->dc); > + return CAIRO_INT_STATUS_UNSUPPORTED; > + } > + local = cairo_image_surface_create_for_data (rectout.pBits, surface->format, width, height, rectout.Pitch); Probably worth wrapping this line @@ +737,5 @@ > interest_rect->x, > interest_rect->y, > interest_rect->width, > + interest_rect->height, &local); > + trailing whitespace here and other places. ::: gfx/layers/d3d9/ThebesLayerD3D9.cpp @@ +563,5 @@ > break; > } > } > NS_ASSERTION(srcTextures.Length() == destTextures.Length(), "Mismatched lengths"); > + destinationSurface = nullptr; this line could use a comment too. ::: gfx/thebes/gfxWindowsSurface.cpp @@ +36,5 @@ > > +gfxWindowsSurface::gfxWindowsSurface(IDirect3DSurface9 *surface, uint32_t flags) : > + mOwnsDC(false), mForPrinting(false), mWnd(nullptr) > +{ > + mDC = 0; // GetDC will fail This comment needs to be removed.

Hrm. The results of this last patch are less encouraging than the previous patch. While we're definitely still seeing wins for m-c and UX both, when comparing m-c+patch and UX+patch, UX seems to fare worse than m-c vs UX. :/ A few possibilities: 1) Something is different about this patch that is causing the change 2) We're seeing noise in the measurements 3) We're being affected by the fact that my original measurements in comment 10 were on PGO builds, and these latest measurements are all on non-PGO builds. m-c (06dad82dbb36) vs m-c+patch (dff890def885): http://compare-talos.mattn.ca/breakdown.html?oldTestIds=29104563&newTestIds=29083787,29106793,29106803,29106815&testName=tart&osName=Windows%20XP&server=graphs.mozilla.org UX (21244604bef8) vs UX+patch (a780ad91b47c) http://compare-talos.mattn.ca/breakdown.html?oldTestIds=29060413,29061533,29061539,29061551,29061557&newTestIds=29083485,29106787,29106809,29106821&testName=tart&osName=Windows%20XP&server=graphs.mozilla.org m-c+patch (dff890def885) vs UX+patch (a780ad91b47c) http://compare-talos.mattn.ca/breakdown.html?oldTestIds=29083787,29106793,29106803,29106815&newTestIds=29083485,29106787,29106809,29106821&testName=tart&osName=Windows%20XP&server=graphs.mozilla.org m-c (06dad82dbb36) vs UX (21244604bef8) http://compare-talos.mattn.ca/breakdown.html?oldTestIds=29104563&newTestIds=29060413,29061533,29061539,29061551,29061557&testName=tart&osName=Windows%20XP&server=graphs.mozilla.org

Fixed failure, review comment and changed GetDC function.

Minor tweaks

Comment on attachment 799593 [details] [diff] [review] patch https://tbpl.mozilla.org/?tree=Try&rev=f74880c879f4

Waiting on try and still need to fix indention and restore GetDC.

fixed DC and indentation

This seems to give a meaningful 6% TART improvement on windows xp: http://graphs.mozilla.org/graph.html#tests=[[293,131,37],[281,131,37]]&sel=1378246162000,1378418962000&displayrange=7&datatype=running And also 16% tsvgx improvements, also on xp: http://graphs.mozilla.org/graph.html#tests=[[293,131,37],[281,131,37]]&sel=1378246162000,1378418962000&displayrange=7&datatype=running But other windows platforms are less affected by this (because they don't use D3D9 by default on our try slaves?). It also has unexpected (to me) 30% improvement on tsvgr_opacity on ubuntu32, but 100% regression on ubuntu 64. Talos compare of revisions 365e150efda0 (old) to b5d62f5c733c: http://compare-talos.mattn.ca/?oldRevs=365e150efda0&newRev=b5d62f5c733c&server=graphs.mozilla.org&submit=true Are these kind of changes expected from this?

(In reply to Avi Halachmi (:avih) from comment #30) > It also has unexpected (to me) 30% improvement on tsvgr_opacity on ubuntu32, > but 100% regression on ubuntu 64. It looks like these changes are within noise levels. If we can find non d3d9 platforms that are effected by this then we're doing something wrong.

(In reply to Benoit Girard (:BenWa) from comment #32) > It looks like these changes are within noise levels. If we can find non d3d9 > platforms that are effected by this then we're doing something wrong. If we look beyond the bi/tri modal behavior, it actually looks to me as if both regressed in in 20-100%. I don't think it's noise, but it might also be due to another patch (which I don't think I'm aware of). http://graphs.mozilla.org/graph.html#tests=[[225,131,33],[225,131,35]]&sel=1377815674109,1378420474109&displayrange=7&datatype=running

Could be this: https://bugzilla.mozilla.org/show_bug.cgi?id=904533#c43