Closed
Bug 1086284
Opened 8 years ago
Closed 8 years ago
Painting SVG-as-image causes AddRef/Release churn
Categories
(Core :: SVG, defect)
Core
SVG
Tracking
()
RESOLVED
FIXED
mozilla38
People
(Reporter: smaug, Assigned: jwatt)
References
Details
Attachments
(1 file)
Things under SVGDocumentWrapper::GetWidthOrHeight (Width/Heigth/AnimVal) ends up Addrefing/Releasing svg element during paintlist creation. (At least) that causes the svg-as-image elements to show up in the CC graph all the time.
Reporter | ||
Comment 1•8 years ago
|
||
If my testing is right, fixing this would indeed hide svg-as-image documents from CC graph, and cut down the base CC graph size (when there is just one tab open) about 50%. (In my test I just made the method to return some dummy value in aResult.)
![]() |
Assignee | |
Updated•8 years ago
|
OS: Linux → All
Hardware: x86_64 → All
Version: 34 Branch → Trunk
![]() |
Assignee | |
Comment 2•8 years ago
|
||
Attachment #8532785 -
Flags: review?(dholbert)
Comment 3•8 years ago
|
||
Comment on attachment 8532785 [details] [diff] [review] patch ># User Jonathan Watt <jwatt@jwatt.org> >Bug 1086284 - Avoid using COM when determining an SVG-as-an-image's intrinsic size so that it doesn't end up in the GC graph. r=dholbert s/COM/refcounting/ (I don't think any nsCOMPtrs are involved here -- only nsRefPtrs) Also, s/GC graph/CC graph/ >diff --git a/dom/svg/SVGSVGElement.cpp b/dom/svg/SVGSVGElement.cpp >+float >+SVGSVGElement::GetIntrinsicWidth() const >+{ >+ if (mLengthAttributes[ATTR_WIDTH].GetSpecifiedUnitType() == >+ nsIDOMSVGLength::SVG_LENGTHTYPE_PERCENTAGE) { >+ return UnspecifiedNaN<float>(); >+ } This can be condensed to: if (mLengthAttributes[ATTR_WIDTH].IsPercentage()) { Same in GetIntrinsicHeight. >+ // context is only needed for percentage resolution >+ SVGSVGElement* unused = nullptr; >+ return std::max(mLengthAttributes[ATTR_WIDTH].GetAnimValue(unused), 0.f); >+} Three things: (1) Clarify comment to: // Context is only needed for percentage resolution. We already know we // don't have a percentage, so no context is needed; hence, nullptr. (2) I'm not sure why you're splitting out "unused = nullptr" into a variable -- you could just directly pass nullptr, right? Presumably you're using a variable to be more explicit about what the "nullptr" represents? If so, probably better to name it "context" instead of "unused". ("unused" is no clearer than "nullptr", IMO -- "context = nullptr; foo(context);" seems much clearer than "unused = nullptr; foo(unused)" [with the comment explaining why we pass a null context]) (3) Just an observation: the zero-clamping here (with std::max) *does* represent a behavior-change from what our code currently does here, but I think it's a good change. Up until now, we've considered SVG Images as being able to have e.g. negative intrinsic widths, and client code has just been responsible for handling that, but from a quick GDB session, it looks like client code does not handle it. (e.g. nsImageLoadingContent::GetNaturalWidth foolishly converts the returned int32_t into an uint32_t, without any range-checking) So, it's probably good to just treat negative sizes as 0. >+++ b/dom/svg/SVGSVGElement.h >@@ -253,16 +253,19 @@ public: > already_AddRefed<SVGTransform> CreateSVGTransform(); > already_AddRefed<SVGTransform> CreateSVGTransformFromMatrix(SVGMatrix& matrix); > using nsINode::GetElementById; // This does what we want > already_AddRefed<SVGAnimatedRect> ViewBox(); > already_AddRefed<DOMSVGAnimatedPreserveAspectRatio> PreserveAspectRatio(); > uint16_t ZoomAndPan(); > void SetZoomAndPan(uint16_t aZoomAndPan, ErrorResult& rv); > >+ float GetIntrinsicWidth() const; >+ float GetIntrinsicHeight() const; >+ Move these up higher. (You're adding these at the bottom of the "// WebIDL" section, which is not the right place for them. They belong higher up, probably right above "// WebIDL", or at least somewhere in the "public helpers" section above that.) Also, please add a comment for these -- since they're public methods exposed to everything that knows about SVGSVGElement. (mentioning the behavior w/ percent values, in particular) >+++ b/image/src/SVGDocumentWrapper.cpp >@@ -71,44 +72,24 @@ SVGDocumentWrapper::DestroyViewer() > bool > SVGDocumentWrapper::GetWidthOrHeight(Dimension aDimension, > int32_t& aResult) > { [...] >+ float length = (aDimension == eWidth) ? rootElem->GetIntrinsicWidth() >+ : rootElem->GetIntrinsicHeight(); Optional: Since you're making GetWidthOrHeight much shorter now, I wonder if we should just do away with it entirely, and fold it into VectorImage::GetWidth() and VectorImage::GetHeight()? (That would remove the need for this ternary condition here, for example.) This could be in a followup bug, too. r=me with the above addressed.
Attachment #8532785 -
Flags: review?(dholbert) → review+
Comment 4•8 years ago
|
||
FWIW Regarding 2) there are two pointer overloadings of GetAnimValue nsSVGELement* and nsIFrame* so nullptr wouldn't compile.
Comment 5•8 years ago
|
||
Ah, thanks -- that part of the patch makes a bit more sense, given that. (I do still think it'd be a bit more declarative/readable to name it "context" instead of "unused", but not a big deal.)
Reporter | ||
Comment 6•8 years ago
|
||
Comment on attachment 8532785 [details] [diff] [review] patch This does remove all the svg-img documents we use in FF UI from the CC graph. Thanks.
Attachment #8532785 -
Flags: feedback+
![]() |
Assignee | |
Comment 7•8 years ago
|
||
Comment on attachment 8532785 [details] [diff] [review] patch https://hg.mozilla.org/integration/mozilla-inbound/rev/6d81c1303daf
Attachment #8532785 -
Flags: checkin+
![]() |
Assignee | |
Comment 8•8 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #3) > Optional: Since you're making GetWidthOrHeight much shorter now, I wonder if > we should just do away with it entirely, and fold it into > VectorImage::GetWidth() and VectorImage::GetHeight()? (That would remove > the need for this ternary condition here, for example.) This could be in a > followup bug, too. Bug 1112533.
Comment 9•8 years ago
|
||
sorry back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=4718023&repo=mozilla-inbound
Comment 10•8 years ago
|
||
The failures look like we're resolving the "height" & "width" attributes on <svg> to 0 (in an image), (for the purposes of image intrinsic size determination) when they have "em" units. This is probably because the 'context' pointer is used for "em" resolution *as well as* percent resolution.
Comment 11•8 years ago
|
||
Comment on attachment 8532785 [details] [diff] [review] patch >+float >+SVGSVGElement::GetIntrinsicWidth() const >+{ >+ if (mLengthAttributes[ATTR_WIDTH].GetSpecifiedUnitType() == >+ nsIDOMSVGLength::SVG_LENGTHTYPE_PERCENTAGE) { >+ return UnspecifiedNaN<float>(); >+ } >+ // context is only needed for percentage resolution >+ SVGSVGElement* unused = nullptr; >+ return std::max(mLengthAttributes[ATTR_WIDTH].GetAnimValue(unused), 0.f); Can't we just pass in "this" as the context-pointer here?
![]() |
Assignee | |
Comment 12•8 years ago
|
||
Yeah, turns out we can. I added a comment to the source to note why that's okay. https://hg.mozilla.org/integration/mozilla-inbound/rev/24dbc8542602
https://hg.mozilla.org/mozilla-central/rev/24dbc8542602
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in
before you can comment on or make changes to this bug.
Description
•