Bug 60625

Created attachment 93173 [details] Patch

Comment on attachment 93173 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=93173&action=review I'm not happy about the virtual functions. They are unnecessary for rect, circle and co. Rob will test if transforming SVGMarkerLayoutInfo m_markerLayoutInfo to an OwnPtr could be an compromise. r- for the meantime. > Source/WebCore/rendering/svg/RenderSVGPath.h:69 > + virtual FloatRect calculateMarkerBoundsIfNeeded() { return FloatRect(); } the '&' is on the wrong place. Doesn't match webkit style. > Source/WebCore/rendering/svg/RenderSVGPath.h:109 > + OwnPtr<SVGMarkerLayoutInfo> &markerLayoutInfo() Ditto.

Created attachment 140725 [details] Patch

Comment on attachment 140725 [details] Patch Attachment 140725 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12650362

If you're trying to reduce the size of RenderSVGShape, seems you should add a COMPILE_ASSERT to verify that it never gets bigger again. We could also just make it OwnPtr<MarkerInfo> instead. I'm not sure the global hash is necessary.

I guess what I'm saying is that I don't view this as a "Cleanup". This is not making the code cleaner. It is possibly saving memory in teh common case, but it's making the code harder to follow. The question is if that complexity is worth it.

Hi Eric, (In reply to comment #6) > I guess what I'm saying is that I don't view this as a "Cleanup". This is not making the code cleaner. It is possibly saving memory in teh common case, but it's making the code harder to follow. The question is if that complexity is worth it. Sure, mostly putting the patch up for discussion, I do think markers are rare and the way we now always allocate it is suboptimal though. Cheers, Rob.

Comment on attachment 140725 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=140725&action=review Nice approach in general! Some nitpicks: > Source/WebCore/rendering/svg/RenderSVGShape.cpp:58 > +typedef HashMap<const RenderSVGShape*, SVGMarkerLayoutInfo*> MarkerLayoutInfoMap; > +static MarkerLayoutInfoMap* gMarkerLayoutInfoMap = 0; Why don't you keep this in the getMarkerLayoutInfo method, and make it static?? Also it should keep OwnPtr<SVGMarkerLayoutInfo>, this is possible in trunk since a while. > Source/WebCore/rendering/svg/RenderSVGShape.cpp:60 > +SVGMarkerLayoutInfo* getMarkerLayoutInfo(const RenderSVGShape* shape) Proposal (see below as well): SVGMarkerLayoutInfo* markerLayoutInfoForShape(RenderSVGShape*) SVGMarkerLayoutInfo* ensureMarkerLayoutInfoForShape(RenderSVGShape*). I don't think we need const RenderSVGShape here, correct me if I'm wrong. > Source/WebCore/rendering/svg/RenderSVGShape.cpp:428 > + gMarkerLayoutInfoMap = new MarkerLayoutInfoMap(); s/()// > Source/WebCore/rendering/svg/RenderSVGShape.cpp:434 > + SVGMarkerLayoutInfo* markerLayoutInfo = getMarkerLayoutInfo(this); > + if (!markerLayoutInfo) { > + markerLayoutInfo = new SVGMarkerLayoutInfo(); > + gMarkerLayoutInfoMap->add(this, markerLayoutInfo); > + } This should be done in a function named "ensureMarkerLayoutInfo" which gives you a SVGMarkerLayoutInfo* and stashes it in the cache.

Created attachment 140764 [details] Patch

Comment on attachment 140764 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=140764&action=review I don't like this approach. Unless you have data to suggest this is worth the extra code, I would prefer an OwnPtr/RarePtr approach or leaving the code as-is. If your goal is to make this object smaller, we should have evidence that this actually affects memory usage, and should COMPILE_ASSERT so we never get it bigger. OTherwise we're just making work for ourselves here. :) > Source/WebCore/rendering/svg/RenderSVGShape.cpp:59 > +static SVGMarkerLayoutInfo* getMarkerLayoutInfo(const RenderSVGShape* shape, bool ensure = false) We dont' normally use get in method names. "Precede setters with the word "set". Use bare words for getters. Setter and getter names should match the names of the variables being set/gotten." http://www.webkit.org/coding/coding-style.html > Source/WebCore/rendering/svg/RenderSVGShape.cpp:89 > + delete markerLayoutInfo; You could also have your HashMap OwnPtr the marker and then just always empty the hash map for every shape destruction.

(In reply to comment #10) > (From update of attachment 140764 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=140764&action=review > > I don't like this approach. Unless you have data to suggest this is worth the extra code, I would prefer an OwnPtr/RarePtr approach or leaving the code as-is. If your goal is to make this object smaller, we should have evidence that this actually affects memory usage, and should COMPILE_ASSERT so we never get it bigger. OTherwise we're just making work for ourselves here. :) I think its a good idea to only allocate the SVGMarkerLayoutInfo struct if needed, as its not small. Though as Eric correctly pointed out, an OwnPtr<SVGMarkerLayoutInfo> would work as well. Without a proof that Robs approach is superior in terms of memory, we should probably stick with the easier approach.

This would need a new approach but don't think that we need to do that.