1439685 - Convert from SVG 1.1 SVGRect to SVG 2 DOMRect/DOMRectReadOnly

Status: NEW

(Core :: SVG, enhancement, P3)

Description

[Robert Longson [:longsonr]]

Per https://www.w3.org/TR/SVG2/changes.html#types SVGRect has been replaced by DOMRect and DOMRectReadOnly

Comment 1

[Robert Longson [:longsonr]]

Attachment: patch

Comment 2

[Jet Villegas (inactive)]

[ Triage 2017/02/20: P3 ]

Comment 3

[Jonathan Watt [:jwatt]]

baku, what are your thoughts on the dom/base/DOMRect.h changes to make the DOM setters virtual? Is that a perf concern? If so, can we implement the DOMRect WebIDL interface separately for DOMBaseVal and DOMAnimVal, or does that cause other problems?

Comment 4

[Robert Longson [:longsonr]]

It's probably going to be even worse if I try to do s/SVGMatrix/DOMMatrix/ and/or s/SVGPoint/DOMPoint/ Points go in a SVGPointList which is supposed to be live and SVGMatrix has huge numbers of methods quite a number of which I'd have to override.

If we're going to do SVGRect we should assume that SVGPoint and SVGMatrix are viable for elimination too. Else do none of them of course.

Comment 5

[Andrea Marchesini [:baku]]

(In reply to Jonathan Watt [:jwatt] from comment #3)
> baku, what are your thoughts on the dom/base/DOMRect.h changes to make the
> DOM setters virtual?

We already have virtual methods for getters, right? I suspect getters are used more often than setters.
Maybe we can ask somebody working on performances to give a hints here.

Ehsan do you have any opinion here? Thanks!

Comment 6

[(no longer active)]

The performance implications of changes like this are pretty unclear and usually these things tend to bite us in the ways that we least expect them, years later...  I decided to try to give an alternative solution instead of just saying no to making these setters virtual.  I'm not sure if this is a good solution or not, but is it possible to have a [NoInterfaceObject] SVG specific rect interface that SVGAnimatedRect.baseVal returns (and a ReadOnly variant for SVGAnimatedRect.animVal) that would be invisible to content but would help us have a subclass of DOMRect (for example DOMBaseVal and DOMAnimVal in your current patch) that are statically bound to those interfaces so that we could avoid the need for virtual calls?

For example, here's a rough sketch:

  [NoInterfaceObject]
  interface SVGBaseValRect : DOMRect {};

  interface SVGAnimatedRect {
    readonly attribute SVGBaseValRect? baseVal;
    readonly attribute SVGAnimValRectReadOnly? animVal;
  };

  // SVGBaseValRect is DOMBaseVal in your current patch
  struct SVGAnimatedRect final : public DOMRect {
    // same stuff as you have now...

    void SetX(double aX) {
      // custom non-virtual override
    }
    // ...
  };
  // Similarly for SVGAnimValRect

Comment 7

[(no longer active)]

Err, Robert is the patch author.  :-)

Comment 8

[Jonathan Watt [:jwatt]]

Comment on attachment 8952453 [details] [diff] [review]
patch

I'm going to cancel the review request for now until one of us has the time to take a look at ehsan's suggestion.

Comment 10

[Robert Longson [:longsonr]]

Attachment: 1439685.txt

This is what I've got so far but....

1. It doesn't work for AnimVal at all because you can't have an inherit readonly attribute
2. It doesn't work for left, right, width, height (this one's fixable)
3. It's going to be very fragile because if someone adds another property to DOMRect or DOMRectReadOnly it wont work in SVGAnimValRect or SVGBaseValRect without implementing it there too.

So in conclusion I'm rather thinking that

1. we change methods that take an SVGRect/SVGPoint/SVGMatrix to take an DOMRectInit, DOMPointInit, DOMMatrixInit as that's compatible with either
2. we won'tfix this and push back on the w3c to revert the spec at least for live attributes like viewBox.animVal and viewBox.baseVal.

Comment 11

[Robert Longson [:longsonr]]

thoughts Ehsan?

Comment 12

[Robert Longson [:longsonr]]

https://github.com/w3c/svgwg/issues/706

Comment 13

[(no longer active)]

Thanks a lot for looking into this, Robert. I looked at the github issue in comment 12. It looks like option 2 in comment 10 isn't getting much support yet. If I understand option 1 there correctly, is that really completely compatible with their current signature? For example, if a method void foo(SVGRect arg); exists on a given interface, calling it with {} would be emit a TypeError, but with that option such a call would work successfully, no?

A bit about the performance concern that I had (apologies if I'm repeating something you already know), there specific case I'm worried about is JS code that for example deals with DOMRect and SVGRect objects, and calls DOMRect::SetX() and SVGRect::SetX() alternately. If the SetX() function is virtual then the CPU's branch predictor logic will have a very hard time making correct predictions around which concrete implementation must be called, so each function call will have a lot of overhead because it will be a jump to a mispredicted code address. But perhaps such access patterns where JS code calls these virtual functions in alternating order or other patterns which are hard to predict are so uncommon that we should bite our tongue and live with this spec change? Hard for me to say as a non-domain-expert, but hopefully you'll have useful context to add here.

Comment 14

[Robert Longson [:longsonr]]

This isn't about foo(SVGRect arg), we've already changed all those to DOMRectInit, which is compatible with SVGRect and DOMRect. This is about DOMRect being live i.e. if element.viewBox.animVal is a DOMRect then element.viewBox.animVal.x should update the viewBox attribute like setAttribute does.

Comment 15

[violet.bugreport]

This is about DOMRect being live i.e. if element.viewBox.animVal is a DOMRect then element.viewBox.animVal.x should update the viewBox attribute like setAttribute does.

What about proposing to drop this liveness from SVG spec? It seems the trend now is to make SVG less special with respect to the rest of the web, animVal and baseVal stuff will become more and more irrelevant in the future, especially with more attributes becoming CSS property. animVal already becomes an alias of baseVal in spec, though no one has made this backward-incompatible change yet.

Comment 16

[Robert Longson [:longsonr]]

We'd be worried about breaking existing content. E.g. https://css-tricks.com/creating-a-panning-effect-for-svg/

Comment 17

[violet.bugreport]

This looks unfortunate, its usage is not as rare as I originally thought.

If we make those setters virtual, the potential performance implication is worse than what is described Comment 13. Outside SVG context, those structs are possibly used as basic mathematical utilities, in WebGL for example, where performance does matter. Even if the the branch target predication works perfectly, there is still an inevitable function call for a trivial assignment that would make the generated code less performant (lack of inlining, missing optimization, caller saved register etc.).

Comment 18

[violet.bugreport]

My last comment is not correct. We are talking about calling from JS code with JIT, which is not able to inline native function in the first place IIRC. So the DOMPoint should already be significantly slower than a simple JS dictionary.

Just running a simple benchmark shows DOMPoint is incredibly slower than JS dictionary when being used in a for loop, which makes it unusable as a mathematical utility.

Based on this, I doubt the extra overhead of a virtual function really matters (it's already unusable if performance is a concern).

Comment 19

[(no longer active)]

(In reply to Robert Longson [:longsonr] from comment #14)

This isn't about foo(SVGRect arg), we've already changed all those to DOMRectInit, which is compatible with SVGRect and DOMRect. This is about DOMRect being live i.e. if element.viewBox.animVal is a DOMRect then element.viewBox.animVal.x should update the viewBox attribute like setAttribute does.

Ah now I understand, sorry you had mentioned this before but it somehow didn't "click".

I think violet.bugreport is correct in comment 18 for this use case, so I take back my previous objection.