Bug 133916 – Implement DOMPoint/DOMPointReadOnly

• Component: DOM
• Status: RESOLVED FIXED

[Dirk Schulze]

Implement DOMPoint/DOMPointReadOnly behind a flag GEOMETRY.

[Dirk Schulze]

Created attachment 233124 [details]
Patch

[WebKit Commit Bot]

Attachment 233124 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/Configurations/FeatureDefines.xcconfig:0:  Any changes made to FeatureDefines should be made to all of them (changed file does not match Source/WebKit/mac/Configurations/FeatureDefines.xcconfig).  [featuredefines/equality] [5]
ERROR: Source/JavaScriptCore/Configurations/FeatureDefines.xcconfig:0:  Any changes made to FeatureDefines should be made to all of them (changed file does not match Source/WebKit2/Configurations/FeatureDefines.xcconfig).  [featuredefines/equality] [5]
ERROR: Source/WebCore/Configurations/FeatureDefines.xcconfig:0:  Any changes made to FeatureDefines should be made to all of them (changed file does not match Source/WebKit/mac/Configurations/FeatureDefines.xcconfig).  [featuredefines/equality] [5]
ERROR: Source/WebCore/Configurations/FeatureDefines.xcconfig:0:  Any changes made to FeatureDefines should be made to all of them (changed file does not match Source/WebKit2/Configurations/FeatureDefines.xcconfig).  [featuredefines/equality] [5]
ERROR: Source/WebCore/bindings/js/JSDOMPointReadOnlyCustom.cpp:31:  You should add a blank line after implementation file's own header.  [build/include_order] [4]
Total errors found: 5 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Benjamin Poulain]

Comment on attachment 233124 [details]
Patch

I am strongly against this. The binding overhead is an order of magnitude larger than the cost of the operations.

With FTL, the compiler has huge opportunities for inlining and vectorizing this kind of operations. Denying this would be shooting ourself in the foot with our biggest gun.

[Dirk Schulze]

(In reply to comment #3)
> (From update of attachment 233124 [details])
> I am strongly against this. The binding overhead is an order of magnitude larger than the cost of the operations.
> 
> With FTL, the compiler has huge opportunities for inlining and vectorizing this kind of operations. Denying this would be shooting ourself in the foot with our biggest gun.

Thanks. I uploaded the patch to get more feedback. I need some guidance for this work. Is there an example that I can use? Also, I did not implement matrixTransform() which requires a 4x4 matrix multiplication. DOMMatrix requires many more 2x3 and 4x4 operations. Would I need to reimplement TransformationMatrix?

[Benjamin Poulain]

(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 233124 [details] [details])
> > I am strongly against this. The binding overhead is an order of magnitude larger than the cost of the operations.
> > 
> > With FTL, the compiler has huge opportunities for inlining and vectorizing this kind of operations. Denying this would be shooting ourself in the foot with our biggest gun.
> 
> Thanks. I uploaded the patch to get more feedback. I need some guidance for this work. Is there an example that I can use? Also, I did not implement matrixTransform() which requires a 4x4 matrix multiplication. DOMMatrix requires many more 2x3 and 4x4 operations. Would I need to reimplement TransformationMatrix?

It is probably ask Filip for advices on how to design this correctly.

The DOMPoint and DOMMatrix seems quite similar to Float64Array in that we want a continuous buffer of memory (ideally aligned to 16 bytes) that should be addressed very efficiently from JavaScript. Filip iterated a lot over the typed array so I am sure he will have plenty of insights about making this great.

Regarding transformation by a matrix, the overhead of that operation is large enough that we can afford a function call. TransformationMatrix has hand written assembly to do that fast, the only requirement is that the buffer is aligned to 16 bytes. This code can be moved to JSC or WTF if needed. We can also relax the alignment constraints but that would be unfortunate.

For simpler operations (matrix scale, translate, etc), I believe built-ins will give the best performance.

[Filip Pizlo]

Comment on attachment 233124 [details]
Patch

This implementation makes it so that creating, populating, and reading a DOMPoint will be about 10-100x more expensive than creating, populated, and reading a JavaScript object with "x", "y", "z", and "w" fields.  It seems like DOMPoint should be implemented entirely in JavaScript.

[Filip Pizlo]

I would like to understand specifically why the entire DOMPoint class cannot just be implemented in JavaScript.

Please assume, for the sake of the argument, that we *can* implement it in JavaScript.  I'm not talking about polyfill.  It would be an integral part of WebKit, just implemented in JS and not C++.  This is already true of many integral ES library functions in JavaScriptCore and making it possible to do the same thing in WebCore shouldn't be hard.

[Benjamin Poulain]

(In reply to comment #7)
> I would like to understand specifically why the entire DOMPoint class cannot just be implemented in JavaScript.
> 
> Please assume, for the sake of the argument, that we *can* implement it in JavaScript.  I'm not talking about polyfill.  It would be an integral part of WebKit, just implemented in JS and not C++.  This is already true of many integral ES library functions in JavaScriptCore and making it possible to do the same thing in WebCore shouldn't be hard.

Dirk seems to also agree this should be implemented in JavaScript. He was asking for guidance. Please have a look at the previous comments.

[Dirk Schulze]

(In reply to comment #7)
> I would like to understand specifically why the entire DOMPoint class cannot just be implemented in JavaScript.
> 
> Please assume, for the sake of the argument, that we *can* implement it in JavaScript.  I'm not talking about polyfill.  It would be an integral part of WebKit, just implemented in JS and not C++.  This is already true of many integral ES library functions in JavaScriptCore and making it possible to do the same thing in WebCore shouldn't be hard.

The only thing I worry about is the manual implementation of the whole API. Parsing, throwing exception and type conversation is done by our code generator for IDLs. Even if our generator needs a lot of work to hook it up to WebIDL 1.0, the code is still much more reliable and less error prone than manually writing the code.

In general, I would like an approach where we slowly move generated APIs to JS all together. For DOMPoint all the above things are easy to do manually. For DOMMatrix it slowly gets harder to do. Also, would it mean reimplementing TransformationMatrix.cpp in JS as well?

If I am mistaken, it would be great to see some examples in JSC. I never touched JSC so far.

[Oliver Hunt]

(In reply to comment #7)
> I would like to understand specifically why the entire DOMPoint class cannot just be implemented in JavaScript.

Unfortunately I don't think it's possible to create constructors using the builtins logic.

I believe what we'd need is the stuff took hook into the global object which would mostly be mechanical, as well as a way to ensure we can always get access to the original constructors.
We also probably need to come up with better ways to access the builtin/original constructors for things :-/

[Dirk Schulze]

(In reply to comment #10)
> (In reply to comment #7)
> > I would like to understand specifically why the entire DOMPoint class cannot just be implemented in JavaScript.
> 
> Unfortunately I don't think it's possible to create constructors using the builtins logic.
> 
> I believe what we'd need is the stuff took hook into the global object which would mostly be mechanical, as well as a way to ensure we can always get access to the original constructors.
> We also probably need to come up with better ways to access the builtin/original constructors for things :-/

How should I proceed then?

[Dirk Schulze]

(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #7)
> > > I would like to understand specifically why the entire DOMPoint class cannot just be implemented in JavaScript.
> > 
> > Unfortunately I don't think it's possible to create constructors using the builtins logic.
> > 
> > I believe what we'd need is the stuff took hook into the global object which would mostly be mechanical, as well as a way to ensure we can always get access to the original constructors.
> > We also probably need to come up with better ways to access the builtin/original constructors for things :-/
> 
> How should I proceed then?

Benjamin, Filip? Any thoughts?

[Benjamin Poulain]

(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #10)
> > > (In reply to comment #7)
> > > > I would like to understand specifically why the entire DOMPoint class cannot just be implemented in JavaScript.
> > > 
> > > Unfortunately I don't think it's possible to create constructors using the builtins logic.
> > > 
> > > I believe what we'd need is the stuff took hook into the global object which would mostly be mechanical, as well as a way to ensure we can always get access to the original constructors.
> > > We also probably need to come up with better ways to access the builtin/original constructors for things :-/
> > 
> > How should I proceed then?
> 
> Benjamin, Filip? Any thoughts?

It is the first time we have had this problem. There are some pieces pieces missing to implements the objects as builtins.

You should start an email thread with Filip and Oliver directly, I think they are not reading all the updates on this bug. This may not be the best time for them with the work on iOS 8 and Yosemite, but you can start the conversation.

[Dirk Schulze]

(In reply to comment #10)
> (In reply to comment #7)
> > I would like to understand specifically why the entire DOMPoint class cannot just be implemented in JavaScript.
> 
> Unfortunately I don't think it's possible to create constructors using the builtins logic.
> 
> I believe what we'd need is the stuff took hook into the global object which would mostly be mechanical, as well as a way to ensure we can always get access to the original constructors.
> We also probably need to come up with better ways to access the builtin/original constructors for things :-/

I was told that Blink has similar issues with constructors as olliej described. For now Blink did not implement it with Blink-in-JS but with C++ in DOM.

https://codereview.chromium.org/404803002/

[Antoine Quint]

Has there been any other discussion about how to move this forward? I'd welcome the advent of DOMPoint, DOMRect, etc., and Firefox already ships support for those (and Chrome has experimental flags for them).

[Dirk Schulze]

(In reply to comment #15)
> Has there been any other discussion about how to move this forward? I'd
> welcome the advent of DOMPoint, DOMRect, etc., and Firefox already ships
> support for those (and Chrome has experimental flags for them).

No, there hasn't been any progress in the discussion. The main issue still seems to be comment 10 and the wish to implement it rather on the JS side than the DOM side. For Blink the DOM side implementation is the show stopper for at least DOMMatrix and blocks shipping.

[Dirk Schulze]

Oliver, Benjamin: Firefox and Blink implemented the APIs behind a flag. Blink did not implement it on the JS side but integrated it into Blink. Any thoughts?

[Dirk Schulze]

Oliver, Benjamin: Firefox and Blink implemented the APIs behind a flag. Blink did not implement it on the JS side but integrated it into Blink. Any thoughts?

[Simon Fraser (smfr)]

So we be considering using built-ins for things like DOMPoint, DOMRect etc now? Is there an example to follow that shows how to do that?

[Simon Fraser (smfr)]

I talked to Geoff about built-ins. Using built-ins would be advantageous if we think most property access on these classes is going to come from script. However, if they are mainly used as parameters to call into native code, or if used largely from native code, then they would be better as native code.

I don't have a feel for how DOMPoint etc are likely to be used in the wild. My guess is that most devs currently just use [x, y] or { x:, y: }, and would get little win from using DOMPoint.

DOMRect and DOMRectInit are certainly going to be used as arguments to IntersectionObserver, but maybe also stand-alone in script?

DOMMatrix is the one class that is more likely to be used extensively from script, so I think it might get the most win from being a built-in.

Of course, we can always change something to being a built-in once we have usage experience from the wild.

[Dirk Schulze]

(In reply to comment #20)
> I talked to Geoff about built-ins. Using built-ins would be advantageous if
> we think most property access on these classes is going to come from script.
> However, if they are mainly used as parameters to call into native code, or
> if used largely from native code, then they would be better as native code.
> 
> I don't have a feel for how DOMPoint etc are likely to be used in the wild.
> My guess is that most devs currently just use [x, y] or { x:, y: }, and
> would get little win from using DOMPoint.
> 
> DOMRect and DOMRectInit are certainly going to be used as arguments to
> IntersectionObserver, but maybe also stand-alone in script?
> 
> DOMMatrix is the one class that is more likely to be used extensively from
> script, so I think it might get the most win from being a built-in.
> 
> Of course, we can always change something to being a built-in once we have
> usage experience from the wild.

DOMPoint and DOMRect are for DOM to JS communication. So built-ins might be less useful. I agree for DOMMatrix though. Especially with WebGL and WebVR as future main consumer.

[Simon Fraser (smfr)]

Created attachment 291776 [details]
Patch

[Build Bot]

Comment on attachment 291776 [details]
Patch

Attachment 291776 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2299374

New failing tests:
js/dom/global-constructors-attributes.html

[Build Bot]

Created attachment 291779 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Simon Fraser (smfr)]

Created attachment 291780 [details]
Patch

[Build Bot]

Comment on attachment 291780 [details]
Patch

Attachment 291780 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2300205

New failing tests:
js/dom/global-constructors-attributes.html

[Build Bot]

Created attachment 291781 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Darin Adler]

Comment on attachment 291780 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=291780&action=review

> Source/WebCore/dom/DOMPoint.h:40
> +    static Ref<DOMPoint> create() { return adoptRef(*new DOMPoint); }

Why do we need this? If we simply have defaults for all four arguments for the following create function, then we don’t need this one. But also, I think we don’t need it at all.

> Source/WebCore/dom/DOMPoint.h:41
> +    static Ref<DOMPoint> create(double x, double y, double z = 0, double w = 1) { return adoptRef(*new DOMPoint(x, y, z, w)); }

It seems to me we don’t need defaults here. The bindings already have their own defaults, so any defaults here are for the convenience of C++ code, and I would suggest we don’t need them yet, and I think starting without them would be best.

> Source/WebCore/dom/DOMPoint.h:51
> +    DOMPoint(double x = 0, double y = 0, double z = 0, double w = 1)

We only need the defaults for these arguments if we are using them for the create function above with no arguments. I suggest omitting them and not writing that function at all, or perhaps putting the values into that function instead of here?

> Source/WebCore/dom/DOMPointReadOnly.h:42
> +    static Ref<DOMPointReadOnly> create() { return adoptRef(*new DOMPointReadOnly); }

Ditto.

> Source/WebCore/dom/DOMPointReadOnly.h:43
> +    static Ref<DOMPointReadOnly> create(double x, double y, double z = 0, double w = 1) { return adoptRef(*new DOMPointReadOnly(x, y, z, w)); }

Ditto.

> Source/WebCore/dom/DOMPointReadOnly.h:53
> +    DOMPointReadOnly(double x = 0, double y = 0, double z = 0, double w = 1)

Ditto.

> LayoutTests/js/dom/global-constructors-attributes-dedicated-worker-expected.txt:52
> +PASS [Worker] Object.getOwnPropertyDescriptor(global, 'DOMPoint').value is DOMPoint

I think we also need to update this:

platform/efl/js/dom/global-constructors-attributes-dedicated-worker-expected.txt

> LayoutTests/js/dom/global-constructors-attributes-expected.txt:291
> +PASS Object.getOwnPropertyDescriptor(global, 'DOMPoint').value is DOMPoint

Besides the three copies of this file that are modified in this patch, I think we also need to update these other four:

platform/efl/js/dom/global-constructors-attributes-expected.txt
platform/gtk/js/dom/global-constructors-attributes-expected.txt
platform/mac-yosemite/js/dom/global-constructors-attributes-expected.txt
platform/win/js/dom/global-constructors-attributes-expected.txt

[Simon Fraser (smfr)]

https://trac.webkit.org/changeset/207420