Closed Bug 1419764 Opened 2 years ago Closed 2 years ago

Very poor SVG performance for direct SVGMatrix manipulations (analysis included)

Categories

(Core :: SVG, defect, P3)

57 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: seb.p.mueller, Assigned: longsonr)

Details

(Keywords: perf)

Attachments

(3 files, 6 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/62.0.3202.94 Safari/537.36

Steps to reproduce:

With a moderately complex SVG file, modifying one of the top-level element's transform (e.g. in order to generate a panning/zooming effect) directly via the SVGMatrix results in very poor performance. Doing the same via setAttribute increases performance by an order of magnitude at least, depending on SVG complexity.

Attached is a test-case that performs such an animation. It displays the current framerate for the application (in my case below 10 FPS on a capable desktop machine). Clicking the "Fix It" button issues one single call to setAttribute and the performance immediately improves dramatically.


Actual results:

Changing the property of an SVG matrix in transform list of an SVG element results in very poor performance. 
Here is why:
When the SVG is displayed for the first time, the SVG frames are being rebuilt/cloned. This sets the nsSVGAnimatedTransformList.mHadTransformBeforeLastBaseValChange to false (because initially there was no transform set before the initial construction).
The next time the matrix is updated (not going through SetBaseVal), the falsy flag results in SVGTransformableElement::GetAttributeChangeHint to return nsChangeHint_ReconstructFrame in https://searchfox.org/mozilla-central/source/dom/svg/SVGTransformableElement.cpp#84, resulting in a complete frame reconstruction pass (again setting that flag to false). This cycle continues forever because with each frame the flag is false and GetAttributeChangeHint concludes that this is not just a change in the transform but requires a complete  frame reconstruction.

Once setAttribute("transform", "...") gets called the flag will finally become true for this animation frame and also for all subsequent repaints and only then the overall performance gets on par with all other current browsers.


Expected results:

The performance of changing the transform via the JavaScript API should be at least as good as via the DOM. It is orders of magnitude slower, because internally the complete SVG frames are completely reconstructed (removed, and rebuilt/cloned and restyled from scratch) with every animation frame. In our real world test-case this resulted in about 200ms of "applying style" for every frame where just the transform matrix got changed. This resulted in an unusable application with less than 5 FPS.

The logic in of the mHadTransformBeforeLastBaseValChange is flawed so this bug is basically a regression caused by the hack implemented for bug 1255276 : If the code only ever goes through nsSVGAnimatedTransformList::SetBaseValue once initially (during frame reconstruction/cloning) "hadTransform" and thus "mHadTransformBeforeLastBaseValChange" will always stay false, and thus will always trigger a complete and very costly frame reconstruction.

Going via setAttribute("transform", "...") makes the code use another path, setting the mHadTransformBeforeLastBaseValChange variable to true, causing all subsequent changes *not* to trigger a complete frame reconstruction, thus drastically improving performance.
This profile shows that 73% of the total time of a longer running sample is spent in ProcessRestyledFrames: https://perfht.ml/2BfH3kH whereas once the "fix" has been applied (the button was pressed) there are no traces at all of this anymore and the framerate is back to good values(as expected): https://perfht.ml/2BdV1DQ
This is the internal stack when using direct matrix modification:

	xul.dll!mozilla::dom::SVGTransformableElement::GetAttributeChangeHint(const nsIAtom * aAttribute, int aModType) Line 75	C++
 	xul.dll!mozilla::ServoRestyleManager::AttributeChanged(mozilla::dom::Element * aElement, int aNameSpaceID, nsIAtom * aAttribute, int aModType, const nsAttrValue * aOldValue) Line 1444	C++
 	xul.dll!mozilla::PresShell::AttributeChanged(nsIDocument * aDocument, mozilla::dom::Element * aElement, int aNameSpaceID, nsIAtom * aAttribute, int aModType, const nsAttrValue * aOldValue) Line 4352	C++
 	xul.dll!nsNodeUtils::AttributeChanged(mozilla::dom::Element * aElement, int aNameSpaceID, nsIAtom * aAttribute, int aModType, const nsAttrValue * aOldValue) Line 146	C++
 	xul.dll!mozilla::dom::Element::SetAttrAndNotify(int aNamespaceID, nsIAtom * aName, nsIAtom * aPrefix, const nsAttrValue * aOldValue, nsAttrValue & aParsedValue, unsigned char aModType, bool aFireMutation, bool aNotify, bool aCallAfterSetAttr, nsIDocument * aComposedDocument, const mozAutoDocUpdate &) Line 2692	C++
 	xul.dll!nsSVGElement::DidChangeValue(nsIAtom * aName, const nsAttrValue & aEmptyOrOldValue, nsAttrValue & aNewValue) Line 1513	C++
 	xul.dll!nsSVGElement::DidChangeTransformList(const nsAttrValue & aEmptyOrOldValue) Line 2366	C++
 	xul.dll!mozilla::dom::AutoChangeTransformNotifier::~AutoChangeTransformNotifier() Line 91	C++
 	xul.dll!mozilla::dom::SVGTransform::SetMatrix(const mozilla::gfx::BaseMatrix<double> & aMatrix) Line 367	C++
 	xul.dll!mozilla::dom::SVGMatrix::SetE(float aE, mozilla::ErrorResult & rv) Line 98	C++
 	xul.dll!mozilla::dom::SVGMatrixBinding::set_e(JSContext * cx, JS::Handle<JSObject *> obj, mozilla::dom::SVGMatrix * self, JSJitSetterCallArgs args) Line 296	C++


Whereas this is how we get to the critical code piece if we use setAttribute()

	xul.dll!mozilla::dom::SVGTransformableElement::GetAttributeChangeHint(const nsIAtom * aAttribute, int aModType) Line 87	C++
 	xul.dll!mozilla::ServoRestyleManager::AttributeChanged(mozilla::dom::Element * aElement, int aNameSpaceID, nsIAtom * aAttribute, int aModType, const nsAttrValue * aOldValue) Line 1444	C++
 	xul.dll!mozilla::PresShell::AttributeChanged(nsIDocument * aDocument, mozilla::dom::Element * aElement, int aNameSpaceID, nsIAtom * aAttribute, int aModType, const nsAttrValue * aOldValue) Line 4352	C++
 	xul.dll!nsNodeUtils::AttributeChanged(mozilla::dom::Element * aElement, int aNameSpaceID, nsIAtom * aAttribute, int aModType, const nsAttrValue * aOldValue) Line 146	C++
 	xul.dll!mozilla::dom::Element::SetAttrAndNotify(int aNamespaceID, nsIAtom * aName, nsIAtom * aPrefix, const nsAttrValue * aOldValue, nsAttrValue & aParsedValue, unsigned char aModType, bool aFireMutation, bool aNotify, bool aCallAfterSetAttr, nsIDocument * aComposedDocument, const mozAutoDocUpdate &) Line 2692	C++
 	xul.dll!mozilla::dom::Element::SetAttr(int aNamespaceID, nsIAtom * aName, nsIAtom * aPrefix, const nsTSubstring<char16_t> & aValue, bool aNotify) Line 2518	C++
 	xul.dll!mozilla::dom::Element::SetAttribute(const nsTSubstring<char16_t> & aName, const nsTSubstring<char16_t> & aValue, mozilla::ErrorResult & aError) Line 1292	C++
 	xul.dll!mozilla::dom::ElementBinding::setAttribute(JSContext * cx, JS::Handle<JSObject *> obj, mozilla::dom::Element * self, const JSJitMethodCallArgs & args) Line 749	C++

But this time this happens *after* we went down this road in SetAttr, which sets the flag accordingly, before we continue to the same SetAttrAndNotify path as above:

	xul.dll!mozilla::nsSVGAnimatedTransformList::SetBaseValue(const mozilla::SVGTransformList & aValue) Line 40	C++
 	xul.dll!mozilla::nsSVGAnimatedTransformList::SetBaseValueString(const nsTSubstring<char16_t> & aValue) Line 32	C++
 	xul.dll!nsSVGElement::ParseAttribute(int aNamespaceID, nsIAtom * aAttribute, const nsTSubstring<char16_t> & aValue, nsAttrValue & aResult) Line 628	C++
 	xul.dll!mozilla::dom::Element::SetAttr(int aNamespaceID, nsIAtom * aName, nsIAtom * aPrefix, const nsTSubstring<char16_t> & aValue, bool aNotify) Line 2511	C++
 	xul.dll!mozilla::dom::Element::SetAttribute(const nsTSubstring<char16_t> & aName, const nsTSubstring<char16_t> & aValue, mozilla::ErrorResult & aError) Line 1292	C++
 	xul.dll!mozilla::dom::ElementBinding::setAttribute(JSContext * cx, JS::Handle<JSObject *> obj, mozilla::dom::Element * self, const JSJitMethodCallArgs & args) Line 749	C++

So the code could be made to execute orders of magnitude faster if we get to set the mHadTransformBeforeLastValChange somehow in  DidChangeTransformList?
I just verified that if I set the mHadTransformBeforeLastValChange to true from inside DidChangeTransformList works around this issue. So that would be a possible place to fix this issue because.
Component: Untriaged → SVG
Product: Firefox → Core
Assignee: nobody → longsonr
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attachment #8931195 - Flags: feedback?(seb.p.mueller)
Hi Robert, thanks for looking into this.!
Actually I did not modify the source code, since I don't have the tool-chain set up for compiling. I was debugging the official release version directly in Visual Studio on Windows and I modified the value manually directly in the debugger in memory.

The code looks conceptually good to me, though I think the call to the new setter should be done *before* calling DidChangeValue, since it is further down in the stack of DidChangeValue where the flags is read and then the frame is reconstructed. Setting the flag after the call to DidChangeValue is too late for the current/next frame. This is hardly noticeable in the sample without a complete profile where there should be one too many frame constructions, but in my simple debugging tests and from my understanding of the code, this results in this one additional superfluous frame reconstruction and only in the next frames the newly set variable will take effect.

Note that yesterday was the first time I ever looked into the sources of FF, so although I am confident this will fix the issue, take my analysis with a grain of salt ;-)
Comment on attachment 8931195 [details] [diff] [review]
were you doing something like this?

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

Except for the order of the call to the new Setter, this LGTM

::: dom/svg/nsSVGElement.cpp
@@ +2370,4 @@
>  
>    DidChangeValue(GetTransformListAttrName(), aEmptyOrOldValue, newValue);
> +
> +  transformList->SetTransformBeforeLastBaseValChange(hadTransform);

This should probably be moved to before "DidChangeValue" so that it will affect the next call to GetAttributeChangedHint, already, instead of only the next time this is called.
Attached patch patch (obsolete) — Splinter Review
Attachment #8931195 - Attachment is obsolete: true
Attachment #8931195 - Flags: feedback?(seb.p.mueller)
Attachment #8931429 - Flags: review?(dholbert)
Could you create a mochitest regression-test for this?  It should be fairly straightforward, using the SpecialPowers.DOMWindowUtils.framesConstructed API which tells you how many frames have been constructed up to that point.  I'm imagining you'd just capture that in a local variable, and then do the thing that causes unnecessary frame-reconstruction (which is the bug), and then assert that .framesConstructed hasn't changed.

(see https://dxr.mozilla.org/mozilla-central/source/layout/style/test/test_change_hint_optimizations.html and https://dxr.mozilla.org/mozilla-central/source/layout/base/tests/test_frame_reconstruction_for_pseudo_elements.html for sample usages of this .framesConstructed API)
Flags: needinfo?(longsonr)
Comment on attachment 8931429 [details] [diff] [review]
patch

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

::: dom/svg/nsSVGAnimatedTransformList.h
@@ +109,4 @@
>    bool HadTransformBeforeLastBaseValChange() const {
>      return mHadTransformBeforeLastBaseValChange;
>    }
> +  void SetTransformBeforeLastBaseValChange(bool aValue) {

s/SetTransform/SetHadTransform/

"SetTransform" sounds like we're setting the value of a transform. But we're not -- rather, we're setting a flag that *indicates whether we had* a transform.

::: dom/svg/nsSVGElement.cpp
@@ +2364,5 @@
>    // SVGAnimatedTransformList is/has been allocated:
> +  nsSVGAnimatedTransformList* transformList =
> +    GetAnimatedTransformList(DO_ALLOCATE);
> +  transformList->SetTransformBeforeLastBaseValChange(
> +    transformList->HasTransform());

I'm willing to believe this solves the problem for the attached testcase, but I'm not sure it's quite correct...

The "mHadTransformBeforeLastBaseValChange" flag is supposed to represent the HadTransform() state from *before* an attribute change, so that we can reason about how substantial the change was.

But instead, this patch is making us capture the state *after* the attribute-change (i.e. at this point in the code, we've already overwritten the old matrix-value -- in my backtrace, it looks like we stomp on the old value up a few levels, in mozilla::dom::SVGTransform::SetMatrix, and then we call into DidChangeTransformList afterwards via the AutoChangeTransformNotifier destructor.  Note that the SVGTransform that we're operating on up a few levels *is* the first entry in "transformList" here, I think.)

I'm not sure if it's possible for us to enter this DidChangeTransformList() function with HasTransform having changed from false to true (or true to false) from the transform change.  But if that is possible, then we'll be passing the wrong value in SetTransformBeforeLastBaseValChange() here and we'll be taking the optimized path when really we'd need the full "transform was added/removed" changehint codepath...
Attachment #8931429 - Flags: review?(dholbert) → review-
(Just spitballing -- in DidChangeTransformList, maybe we should really be calling something like...
  transformList->SetTransformBeforeLastBaseValChange(!aEmptyOrOldValue.IsEmptyString())
...?  I'm not sure, but that seems closer to capturing the old attribute-value.

Though that doesn't account for mAnimVal in the way that transformList->HasTransform() does, IIUC, so it might still be incomplete.)
Priority: -- → P3
Re #13: I too thought that this flag is a very ugly way of "solving" the issue. I thought that in a slightly better solution, *after* a frame (re-)construction the flag should be set to the then current state (is a transform set or not), so that when later on GetAttributeChangedHint is called we can compare the then current/new value with the previous state of the frame in order to detect whether reconstruction is necessary. Right now this is done "manually" in several places (and obviously not in all) right before we know that we are changing the value, hoping that it will be picked up later on. If we do it unconditionally after frame construction, I don't see how we would ever get to see the wrong state.

Re #12: that should not be difficult for someone who can run tests - I don't have the tool-chain setup for this, unfortunately. But basically it would look like this and would be very simpilar to the attached interactive test-case:

-setup an svg with a g and a transform in the attribute in HTML (no javascript so far)
-wait for it to be rendered/the frames to be constructed (e.g. two rAF) (because initially a frame *needs* to be constructed, obviously.
-then modify the matrix programmatically like in my example code - this will result in complete frame reconstruction although it should not.
-then modifying the transform via setAttribute will *not* result in reconstruction and any subsequent matrix modification will should not, either, once the flag is set internally.

Thanks!
Attached patch updated patch (obsolete) — Splinter Review
No progress on the tests but how about this for a better fix?
Attachment #8931429 - Attachment is obsolete: true
Attachment #8939255 - Flags: review?(dholbert)
Attached patch test (obsolete) — Splinter Review
Flags: needinfo?(longsonr)
Attachment #8939295 - Flags: review?(dholbert)
Comment on attachment 8939295 [details] [diff] [review]
test

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

::: layout/base/tests/test_frame_reconstruction_for_svg_transforms.html
@@ +27,5 @@
> +    matrix.e = 200;
> +
> +    var endcount = utils.framesConstructed;
> +    is(endcount, startcount,
> +       description + ": should not do frame construction")

It looks like the "description" variable is undefined here.  copypaste typo, I assume?  The message probably wants to be something like
  "SVG transform matrix-tweak shouldn't cause frame construction"

(Sanity-check: have you verified that the test fails before the patch and passes after the patch?)
Comment on attachment 8939255 [details] [diff] [review]
updated patch

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

(Sorry for the delay - just returned from vacation and am catching up.)

I think this new patch makes sense, but it'd really benefit from a few code-comments to make it clearer why we're doing what we're doing here.  Could you include some documentation?  Setting feedback+ for now.

(Also, the comment tweaks in nsSVGElement.cpp should probably just directly land on their own, rather than being lumped in as part of this patch & making it artificially seem more complex than it is in terms of file-count.  Maybe just land those as "(no bug): Fix comment-only typos in nsSVGElement.cpp. rs=dholbert"  ?)
Attachment #8939255 - Flags: review?(dholbert) → feedback+
I took the liberty of landing the nsSVGElement.cpp comment-only changes, with you as the author, in a "(no bug)/DONTBUILD" commit:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef1f932198c044df03669bc3e5b703eb3a6946b8
The test does not fail without the fix. I don't know why that would be.
Flags: needinfo?(dholbert)
Another, perhaps better way to fix this would be in nsCSSFrameConstructor to look at whether the element has a transform when constructing the frame, setting the flag accordingly then or waiting for the NS_FRAME_FIRST_REFLOW and setting it at that point, or in setAttribute checking whether we have a primary frame and if not setting the flag to true because the SVG DOM isn't setting the transform, it must exist already.
(In reply to Robert Longson [:longsonr] from comment #23)
> The test does not fail without the fix. I don't know why that would be.

You probably need to explicitly flush layout, e.g. by inserting:
  document.documentElement.offsetTop; // flush layout
...between the matrix.e assignment and the framesConstructed read. Specifically, in these two spots:
https://hg.mozilla.org/try/rev/f61d37278eedc0389b3735c308fd25636faaffa4#l2.28
https://hg.mozilla.org/try/rev/f61d37278eedc0389b3735c308fd25636faaffa4#l2.32

Without that, I expect/hope that we wouldn't do any immediate frame-construction/layout (until we need to, to satisfy some API query like .offsetTop)

(Note that "rect.offsetTop" doesn't work - that doesn't seem to be an API that's exposed on SVG elements, at least according to my local testing with devtools.)

(In reply to Robert Longson [:longsonr] from comment #24)
> Another, perhaps better way to fix this would be in nsCSSFrameConstructor to
> look at whether the element has a transform when constructing the frame,
> setting the flag accordingly then

That sounds like a promising approach, yeah! (I think that's what Sebastian was suggesting at the beginning of comment 14.)
Flags: needinfo?(dholbert)
Comment on attachment 8939295 [details] [diff] [review]
test

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

(marking old test patch as r- for now, to clear my review queue. :))
Attachment #8939295 - Flags: review?(dholbert) → review-
Attached patch patch with testSplinter Review
Attachment #8939255 - Attachment is obsolete: true
Attachment #8939295 - Attachment is obsolete: true
Attachment #8944622 - Flags: review?(dholbert)
Comment on attachment 8944622 [details] [diff] [review]
patch with test

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

r=me, though I think the code-comment might want a tweak, as noted below:

::: dom/svg/nsSVGAnimatedTransformList.cpp
@@ +64,5 @@
>      mIsAttrSet = true;
> +    // If we don't have a frame we can set this flag to true as we're
> +    // the initial reflow will fix us up.
> +    mHadTransformBeforeLastBaseValChange =
> +      !aSVGElement->GetPrimaryFrame() || hadTransform;

This sentence has an extra word, I think ("as we're the initial reflow will fix us up" -- maybe "we're" is extraneous?)  But really, I had a hard time understanding this code with the current comment text -- the comment implyies that there's something that this flag tracks which would be "fixed up" in the initial reflow, but really there's nothing to fix up if there aren't any frames.  (The frames will just be constructed correctly, when they're constructed, and we don't need to bother with this flag.)

How about something like the following (a bit wordy, but worth the wordiness IMO since this flag is a bit confusing to reason about and I have to rediscover its subtleties every time I think about it):
"If we set this flag to false, we're indicating that aSVGElement's frames will need reconstructing to account for stacking context changes.  If aSVGElement doesn't have any frames, then that's clearly unnecessary, so in that case we set the flag to true."
Attachment #8944622 - Flags: review?(dholbert) → review+
Comment on attachment 8944622 [details] [diff] [review]
patch with test

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

::: layout/base/tests/mochitest.ini
@@ +154,4 @@
>  skip-if = true # Bug 688128
>  [test_frame_reconstruction_for_pseudo_elements.html]
>  [test_frame_reconstruction_scroll_restore.html]
> +[test_frame_reconstruction_for_svg_transforms.html]

Nit: The ordering is off here -- this new line should be inserted 1 line further up.
I verified that the test fails without the fix & passes with the fix, too - hooray!  Thanks for fixing this (and thanks Sebastian for reporting)!
Pushed by longsonr@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/640595d09e90
Improve performance where a transform is set by direct matrix manipulation. r=dholbert
Perhaps we should rename the flag from mHadTransformBeforeLastBaseValChange to mRequiresFrameReconstruction or mStackingContextChanged and invert the logic as a follow up.
Attached patch 1419764.txt (obsolete) — Splinter Review
Attachment #8945205 - Flags: review?(dholbert)
Comment on attachment 8945205 [details] [diff] [review]
1419764.txt

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

Thanks! This is probably largely good, but there are some further changes needed to keep the code & comments in sync.

::: dom/svg/SVGTransformableElement.cpp
@@ +77,1 @@
>          // New or old value is empty; this is effectively addition or removal.

This code is getting a little harder to map onto the comment. (And I guess it was already a little obscure.)

Maybe restructure the logic/comment slightly to make this more understandable? Something like:

if (!mTransforms ||
    !mTransforms->HasTransform()) {
  // New value is empty; this is effectively removal.
  isAdditionOrRemoval = true;
}  else if (mTransforms->RequiresFrameReconstruction()) {
  // Old value was empty; this is effectively addition.
  isAdditionOrRemoval = true;
}

What do you think?

::: dom/svg/nsSVGAnimatedTransformList.cpp
@@ +66,5 @@
>      // will need reconstructing to account for stacking context changes.
>      // If aSVGElement doesn't have any frames, then that's clearly unnecessary,
>      // so in that case we set the flag to true.
> +    mRequiresFrameReconstruction =
> +      aSVGElement->GetPrimaryFrame() && !hadTransform;

Since you're flipping the meaning of this flag, you should reword the comment above it (which talks about true/false values that are now the opposite of correct).

(The comment can perhaps simplify a bit now, too, now that the connection to frames / frame-construction is right there in the name of the variable.)

::: dom/svg/nsSVGAnimatedTransformList.h
@@ +108,5 @@
>     * a transform where we previously had none. These cases require a more
>     * thorough nsChangeHint.)
>     */
> +  bool RequiresFrameReconstruction() const {
> +    return mRequiresFrameReconstruction;

The polarity of this bool is inverting, I think (per the usages of this API). So the documentation for this accessor needs rewording to say the opposite of what it says right now.

Also: the naming is probably fine, but the documentation definitely needs to covera bit of subtlety... In particular, this bit doesn't *necessarily* mean we require frame construction. I think all it means (now) is that HasTransform() returned false before the most recent base-value change.... and technically, that information would only *require* frame-reconstruction if we're changing to some value for which HasTransform() returns true.

(I think?)

(Unfortunately, we don't seem to have that subtlety encoded into SVGTransformableElement::GetAttributeChangeHint right now, and I'm not sure it'd be worth encoding that since it'd probably only help in cases where the value is bogus & fails to parse anyway. I suspect those are the sorts of values that'd trigger the MODIFICATION codepath despite leaving us with HasTransform() false, at least...)
Attachment #8945205 - Flags: review?(dholbert) → review-
>  I think all it means (now) is that HasTransform()
> returned false before the most recent base-value change....

To clarify, by "(now)" there, I meant "(now *with this patch*)"
https://hg.mozilla.org/mozilla-central/rev/640595d09e90
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Attached patch 1419764.txt (obsolete) — Splinter Review
Attachment #8945205 - Attachment is obsolete: true
Attachment #8945253 - Flags: review?(dholbert)
Attached patch 1419764.txtSplinter Review
Attachment #8945253 - Attachment is obsolete: true
Attachment #8945253 - Flags: review?(dholbert)
Attachment #8945261 - Flags: review?(dholbert)
Comment on attachment 8945261 [details] [diff] [review]
1419764.txt

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

Need to set isAdditionOrRemoval = true in the newly-created (split) clause in SVGTransformableElement.

(I'm reviewing from an Android tablet and Splinter won't let me double-click to annotate the actual code. :) sorry)

r=me with that
Attachment #8945261 - Flags: superreview+
Comment on attachment 8945261 [details] [diff] [review]
1419764.txt

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

::: dom/svg/SVGTransformableElement.cpp
@@ +73,5 @@
>                   "Unknown modification type.");
>        if (!mTransforms ||
> +          !mTransforms->HasTransform()) {
> +        // New value is empty, treat as removal.
> +      } else if (mTransforms->RequiresFrameReconstruction()) {

(Now that I'm on a computer, here's the spot I was talking about -- you need to add "isAdditionOrRemoval = true;" right before the "else if" here.)
Backed out from m-c for reftest failures.

backout: https://hg.mozilla.org/mozilla-central/rev/6602576987baec9edbaaad117114ba5227db6261

initial push with failures, which got merged to m-c : https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=640595d09e90c717a2c1b24877ac0f5f4817faf2

failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=158189531&repo=mozilla-inbound&lineNumber=15449
Status: RESOLVED → REOPENED
Flags: needinfo?(dholbert)
Resolution: FIXED → ---
Target Milestone: mozilla60 → ---
Flags: needinfo?(dholbert) → needinfo?(longsonr)
The reftest failure is "max difference: 7, number of differing pixels: 60", with a few slightly-fuzzy pixels along the borders of some SMIL-rotated arrow-heads.  It's just antialiasing along a diagonal line, and it's not visually perceptible.

I'll bet this is a case where we used to be *needlessly* doing frame reconstruction (as in this bug), and now we don't, and so there are some layerizing artifacts that are now left behind.

Seems like a reasonable case for a "fuzzy" annotation.
Try run with that fuzzy annotation added:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d6c5c832f6576db692cbeeb732934d6f7e251d76

(There's an existing "fuzzy-if(skiaContent,1,130)" for this test -- I added the new non-platform-specific fuzzy() annotation before that one. I never can recall the exact fallback/overriding behavior for "conflicting" reftest manifest annotations, but this matches the pattern used on the following line, so hopefully it's fine.)
Maybe-better try run (just updating the existing fuzzy-if annotation):
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=a54818d85965558cf9eaa7dcd2afbb27f2f19c4a
In this one, I'm assuming (based on a few other try runs today) that the "skiaContent" condition actually covers all of the failing platforms here, and the old max-difference of 1 is now insufficient and needs bumping up to 6 (and while we're at it, num-pixels=130 might be stale/unnecessarily-large as well so we can clamp that down to 60 hopefully).
Flags: needinfo?(dholbert)
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/96cf099ea7c1
Improve performance where a transform is set by direct matrix manipulation. r=dholbert
Try run looks good, so I re-landed the main patch here, along with a reftest.list fuzzy threshold tweak for rotate-angle-5.svg

(The second renaming/refactoring patch still needs landing [after comment 40 is addressed] though -- I'll leave that to longsonr since I'm not sure if it's gotten a Try run & if there are any other final tweaks he wanted to make. Hence, leaving ni=longsonr open.)
Flags: needinfo?(dholbert)
https://hg.mozilla.org/mozilla-central/rev/96cf099ea7c1
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Looks like the followup patch landed (renaming mHadTransformBeforeLastBaseValChange)  (not sure why pulsebot didn't auto-comment, maybe because this bug is already closed?)
  https://hg.mozilla.org/integration/mozilla-inbound/rev/003668afcef5a0919792fc1331d44e6e8c2fbb40
Keywords: perf
You need to log in before you can comment on or make changes to this bug.