Optimize dartium dart:html bindings so real world application performance is acceptable. Improves d…

Optimize dartium dart:html bindings so real world application performance is acceptable. Improves dart:html performance by 2X-30X for microbenchmarks. Real world improvement is typically 2X-3X.

BUG=

Committed: https://github.com/dart-lang/sdk/commit/c65854d2e4141524b5ab7485cb0952fd7bd2c624

[Alan Knight, 2016-03-25 21:39:08]

alanknight@google.com changed reviewers:
+ alanknight@google.com

[Alan Knight, 2016-03-25 21:39:09]

https://codereview.chromium.org/1832713002/diff/1/sdk/lib/html/html_common/co...
File sdk/lib/html/html_common/conversions_dartium.dart (right):

https://codereview.chromium.org/1832713002/diff/1/sdk/lib/html/html_common/co...
sdk/lib/html/html_common/conversions_dartium.dart:143: return date; //
TODO(jacobr): I don't get why this is needed. new
js.JsObject(js.context["Date"], [date.millisecondsSinceEpoch]);
There will be explicit calls to it from conversions in generator.py. Maybe those
could be removed?

https://codereview.chromium.org/1832713002/diff/1/sdk/lib/html/html_common/co...
sdk/lib/html/html_common/conversions_dartium.dart:219: Type
lookupType(js.JsObject jsObject) {
Will probably break if we have JS objects that share a prototype but map to
different things in Dart. But so does everything else.  See dartbug.com/23144

https://codereview.chromium.org/1832713002/diff/1/tools/dom/src/native_DOMImp...
File tools/dom/src/native_DOMImplementation.dart (right):

https://codereview.chromium.org/1832713002/diff/1/tools/dom/src/native_DOMImp...
tools/dom/src/native_DOMImplementation.dart:817: // Or worse, we are probaly
returning a raw JSObject.
Yes, this is already broken.  https://github.com/dart-lang/sdk/issues/24443

https://codereview.chromium.org/1832713002/diff/1/tools/dom/templates/html/da...
File tools/dom/templates/html/dartium/html_dartium.darttemplate (right):

https://codereview.chromium.org/1832713002/diff/1/tools/dom/templates/html/da...
tools/dom/templates/html/dartium/html_dartium.darttemplate:441: // TODO(jacobr):
clearly this can't happen otherwise an exception wouldn't
I think I remember Terry introducing this because of the case where a user's
custom element constructor throws an exception. But I may be wrong or thinking
of something else.

https://codereview.chromium.org/1832713002/diff/1/tools/dom/templates/html/im...
File tools/dom/templates/html/impl/impl_Element.darttemplate (right):

https://codereview.chromium.org/1832713002/diff/1/tools/dom/templates/html/im...
tools/dom/templates/html/impl/impl_Element.darttemplate:1487: // We could
support this method on Dartium but it does not seem worthwhile
They can be corrupted in a way that doesn't show up with direct Dartium access,
but would show up if calling indirectly through JS Interop. I think what we need
to do is have this check be done by calling a JS function. I'll take a look at
that.

https://codereview.chromium.org/1832713002/diff/1/tools/dom/templates/html/im...
tools/dom/templates/html/impl/impl_Element.darttemplate:1509: try {
Don't we need an implementation of this for Dartium, even if it's just a pass
through. I think there's code that calls it.

[Jacob, 2016-03-30 00:18:58]

jacobr@google.com changed reviewers:
+ terry@google.com

[Jacob, 2016-03-30 00:19:00]

Terry please take a look at the delta from patch set 1 to patch set 2 which
mainly has to do with making custom elements work correctly and adding back
conversions where needed.

https://codereview.chromium.org/1832713002/diff/1/sdk/lib/html/html_common/co...
File sdk/lib/html/html_common/conversions_dartium.dart (right):

https://codereview.chromium.org/1832713002/diff/1/sdk/lib/html/html_common/co...
sdk/lib/html/html_common/conversions_dartium.dart:143: return date; //
TODO(jacobr): I don't get why this is needed. new
js.JsObject(js.context["Date"], [date.millisecondsSinceEpoch]);
On 2016/03/25 21:39:09, Alan Knight wrote:
> There will be explicit calls to it from conversions in generator.py. Maybe
those
> could be removed?

Removed. This is already done in Dartium C++

https://codereview.chromium.org/1832713002/diff/1/sdk/lib/html/html_common/co...
sdk/lib/html/html_common/conversions_dartium.dart:219: Type
lookupType(js.JsObject jsObject) {
On 2016/03/25 21:39:09, Alan Knight wrote:
> Will probably break if we have JS objects that share a prototype but map to
> different things in Dart. But so does everything else.  See dartbug.com/23144
Yep. But there is no cost of failing in the same places dart2js would fail.

https://codereview.chromium.org/1832713002/diff/1/tools/dom/src/native_DOMImp...
File tools/dom/src/native_DOMImplementation.dart (right):

https://codereview.chromium.org/1832713002/diff/1/tools/dom/src/native_DOMImp...
tools/dom/src/native_DOMImplementation.dart:817: // Or worse, we are probaly
returning a raw JSObject.
On 2016/03/25 21:39:09, Alan Knight wrote:
> Yes, this is already broken.  https://github.com/dart-lang/sdk/issues/24443

Acknowledged.

https://codereview.chromium.org/1832713002/diff/1/tools/dom/templates/html/da...
File tools/dom/templates/html/dartium/html_dartium.darttemplate (right):

https://codereview.chromium.org/1832713002/diff/1/tools/dom/templates/html/da...
tools/dom/templates/html/dartium/html_dartium.darttemplate:441: // TODO(jacobr):
clearly this can't happen otherwise an exception wouldn't
On 2016/03/25 21:39:09, Alan Knight wrote:
> I think I remember Terry introducing this because of the case where a user's
> custom element constructor throws an exception. But I may be wrong or thinking
> of something else.

This code path is now obsolete. I've rewritten all the custom element support in
this CL so it actually works(tm). Generally, code flow is now simpler as
"upgrading" can happen a bit earlier as it has to happen more in C++ code given
we need to invoke the  created constructor immediately after creating an
instance.
C++ code now dispatches an error on failed upgrades and falls back to
HTMLElement.

https://codereview.chromium.org/1832713002/diff/1/tools/dom/templates/html/im...
File tools/dom/templates/html/impl/impl_Element.darttemplate (right):

https://codereview.chromium.org/1832713002/diff/1/tools/dom/templates/html/im...
tools/dom/templates/html/impl/impl_Element.darttemplate:1487: // We could
support this method on Dartium but it does not seem worthwhile
On 2016/03/25 21:39:09, Alan Knight wrote:
> They can be corrupted in a way that doesn't show up with direct Dartium
access,
> but would show up if calling indirectly through JS Interop. I think what we
need
> to do is have this check be done by calling a JS function. I'll take a look at
> that.

I switched to a version of the cleaned up version you sent me offline that only
uses trusted Eleement properties and now all the tests pass.

https://codereview.chromium.org/1832713002/diff/1/tools/dom/templates/html/im...
tools/dom/templates/html/impl/impl_Element.darttemplate:1509: try {
On 2016/03/25 21:39:09, Alan Knight wrote:
> Don't we need an implementation of this for Dartium, even if it's just a pass
> through. I think there's code that calls it.

not sure I understand. this is an implementation for Dartium that uses the fact
that tagName cannot be corrupted.

[Jacob, 2016-03-30 01:32:57]

Description was changed from

==========
Optimize dartium dart:html bindings so real world application performance is
acceptable. Improves dart:html performance by 2X-30X for microbenchmarks. Real
world improvement is typically 2X-3X.

BUG=
==========

to

==========
Optimize dartium dart:html bindings so real world application performance is
acceptable. Improves dart:html performance by 2X-30X for microbenchmarks. Real
world improvement is typically 2X-3X.

BUG=

Committed:
https://github.com/dart-lang/sdk/commit/c65854d2e4141524b5ab7485cb0952fd7bd2c624
==========

[Jacob, 2016-03-30 01:32:58]

Committed patchset #5 (id:40001) manually as
c65854d2e4141524b5ab7485cb0952fd7bd2c624 (presubmit successful).