dart:html perf optimization based on runing Dromaeo benchmarks

dart:html perf optimization based on runing Dromaeo benchmarks

BUG=
TEST=

Committed: https://code.google.com/p/dart/source/detail?r=5955

[Jacob, 2012-03-28 00:52:02]

vsm, please review the Dromaeo changes.
I've tweaked the query benchmark to better simulate real world use cases but I
can tweak it back if the change is too large.

Notes:
Modify dart:html to improve performance drastically on Dromaeo 

benchmarks.
Add version of the Dromaeo benchmarks using dart:html (-htmlfast) that should
have identical performance to the dart:dom version once a CL sra is working on
lands.  For now, they are typically within 5% of dart:dom sometimes exceeding
dart:dom in performance.

Implementation details:
documentElement is back as hiding it was hurting performance badly.

add $dom_ methods for previously removed dart:dom functionality where removing
it has a slight performance penalty at the moment

switched the NodeList returned by node.nodes to be lazy which improved traversal
performance by 2X

[nweiz, 2012-03-28 01:16:17]

It would have been nice to split this change up into several more isolated
changes.

https://chromiumcodereview.appspot.com/9732019/diff/12001/lib/dom/scripts/sys...
File lib/dom/scripts/systemhtml.py (right):

https://chromiumcodereview.appspot.com/9732019/diff/12001/lib/dom/scripts/sys...
lib/dom/scripts/systemhtml.py:436: def RenameInHtmlLibrary(self, interface,
member, member_prefix, implementation_class=False):
Line length.

I like keeping member_prefix optional; the ''s that are now showing up below are
less clear than just omitting the argument.

https://chromiumcodereview.appspot.com/9732019/diff/12001/lib/dom/scripts/sys...
lib/dom/scripts/systemhtml.py:890: if ((getter and html_getter_name !=
getter.id) or
None of these parens are really necessary, but even if you want to use the inner
ones for clarity the outer ones are still redundant.

Also below.

https://chromiumcodereview.appspot.com/9732019/diff/12001/lib/dom/templates/h...
File lib/dom/templates/html/impl/impl_Document.darttemplate (right):

https://chromiumcodereview.appspot.com/9732019/diff/12001/lib/dom/templates/h...
lib/dom/templates/html/impl/impl_Document.darttemplate:5: class $CLASSNAME
extends _NodeImpl
Why not "class $CLASSNAME$EXTENDS$IMPLEMENTS$NATIVESPEC"?

https://chromiumcodereview.appspot.com/9732019/diff/12001/lib/dom/templates/h...
lib/dom/templates/html/impl/impl_Document.darttemplate:17: return
$dom_getElementById(selectors.substring(1));
Is this really faster? That's kind of depressing.

How sure are we that Javascript's notion of word characters is identical to
CSS's notion?

https://chromiumcodereview.appspot.com/9732019/diff/12001/lib/dom/templates/h...
lib/dom/templates/html/impl/impl_Document.darttemplate:22: _ElementImpl
$dom_querySelector(String selectors) native "return
this.querySelector(selectors);";
Line length. Also, why isn't this autogenerated?

https://chromiumcodereview.appspot.com/9732019/diff/12001/lib/dom/templates/h...
File lib/dom/templates/html/impl/impl_Node.darttemplate (right):

https://chromiumcodereview.appspot.com/9732019/diff/12001/lib/dom/templates/h...
lib/dom/templates/html/impl/impl_Node.darttemplate:88: throw new
UnsupportedOperationException("Cannot setRange on immutable List.");
Line lengths.

[Jacob, 2012-03-28 17:52:03]

I agree this change is a bit on the large side but I don't think it is large
enough to be hard to review.  The portions each reviewer should review carefully
are very clear as they have different directory prefixes.  I think including the
samples/third_party/dromaeo code in this cl in some ways improves clarity as it
shows how the benchmarks are altered to take advantage of the bindings
generation changes.

The reason the slightly isolated changes are combined together is that these are
the combined set of tweaks needed for solid benchmark performance.  

On 2012/03/28 01:16:17, nweiz wrote:
> It would have been nice to split this change up into several more isolated
> changes.
> 
>
https://chromiumcodereview.appspot.com/9732019/diff/12001/lib/dom/scripts/sys...
> File lib/dom/scripts/systemhtml.py (right):
> 
>
https://chromiumcodereview.appspot.com/9732019/diff/12001/lib/dom/scripts/sys...
> lib/dom/scripts/systemhtml.py:436: def RenameInHtmlLibrary(self, interface,
> member, member_prefix, implementation_class=False):
> Line length.
> 
> I like keeping member_prefix optional; the ''s that are now showing up below
are
> less clear than just omitting the argument.
> 
>
https://chromiumcodereview.appspot.com/9732019/diff/12001/lib/dom/scripts/sys...
> lib/dom/scripts/systemhtml.py:890: if ((getter and html_getter_name !=
> getter.id) or
> None of these parens are really necessary, but even if you want to use the
inner
> ones for clarity the outer ones are still redundant.
> 
> Also below.
> 
>
https://chromiumcodereview.appspot.com/9732019/diff/12001/lib/dom/templates/h...
> File lib/dom/templates/html/impl/impl_Document.darttemplate (right):
> 
>
https://chromiumcodereview.appspot.com/9732019/diff/12001/lib/dom/templates/h...
> lib/dom/templates/html/impl/impl_Document.darttemplate:5: class $CLASSNAME
> extends _NodeImpl
> Why not "class $CLASSNAME$EXTENDS$IMPLEMENTS$NATIVESPEC"?
> 
>
https://chromiumcodereview.appspot.com/9732019/diff/12001/lib/dom/templates/h...
> lib/dom/templates/html/impl/impl_Document.darttemplate:17: return
> $dom_getElementById(selectors.substring(1));
> Is this really faster? That's kind of depressing.
> 
> How sure are we that Javascript's notion of word characters is identical to
> CSS's notion?
> 
>
https://chromiumcodereview.appspot.com/9732019/diff/12001/lib/dom/templates/h...
> lib/dom/templates/html/impl/impl_Document.darttemplate:22: _ElementImpl
> $dom_querySelector(String selectors) native "return
> this.querySelector(selectors);";
> Line length. Also, why isn't this autogenerated?
> 
>
https://chromiumcodereview.appspot.com/9732019/diff/12001/lib/dom/templates/h...
> File lib/dom/templates/html/impl/impl_Node.darttemplate (right):
> 
>
https://chromiumcodereview.appspot.com/9732019/diff/12001/lib/dom/templates/h...
> lib/dom/templates/html/impl/impl_Node.darttemplate:88: throw new
> UnsupportedOperationException("Cannot setRange on immutable List.");
> Line lengths.

[Jacob, 2012-03-28 17:52:54]

Not sure why the code review tool still thought I had unpublished drafts.

https://chromiumcodereview.appspot.com/9732019/diff/12001/lib/dom/scripts/sys...
File lib/dom/scripts/systemhtml.py (right):

https://chromiumcodereview.appspot.com/9732019/diff/12001/lib/dom/scripts/sys...
lib/dom/scripts/systemhtml.py:436: def RenameInHtmlLibrary(self, interface,
member, member_prefix, implementation_class=False):
On 2012/03/28 01:16:17, nweiz wrote:
> Line length.
> 
> I like keeping member_prefix optional; the ''s that are now showing up below
are
> less clear than just omitting the argument.

Done.

https://chromiumcodereview.appspot.com/9732019/diff/12001/lib/dom/scripts/sys...
lib/dom/scripts/systemhtml.py:890: if ((getter and html_getter_name !=
getter.id) or
On 2012/03/28 01:16:17, nweiz wrote:
> None of these parens are really necessary, but even if you want to use the
inner
> ones for clarity the outer ones are still redundant.
> 
> Also below.

I agree that the inner ones aren't needed but I prefer them for clarity.
THe outer one is necessary. Otherwise you get a python syntax error... adding
those extra parens is the cost of not having semicolons.

https://chromiumcodereview.appspot.com/9732019/diff/12001/lib/dom/templates/h...
File lib/dom/templates/html/impl/impl_Document.darttemplate (right):

https://chromiumcodereview.appspot.com/9732019/diff/12001/lib/dom/templates/h...
lib/dom/templates/html/impl/impl_Document.darttemplate:5: class $CLASSNAME
extends _NodeImpl
On 2012/03/28 01:16:17, nweiz wrote:
> Why not "class $CLASSNAME$EXTENDS$IMPLEMENTS$NATIVESPEC"?
Because it extends _NodeImpl not _ElementImpl

Also, we force HTMLDocument as the native class rather than Document

We could hard code in the generator but I believe it is nicer to keep the hard
coded logic in the template for hard coded functionality that is a 1-off.

https://chromiumcodereview.appspot.com/9732019/diff/12001/lib/dom/templates/h...
lib/dom/templates/html/impl/impl_Document.darttemplate:17: return
$dom_getElementById(selectors.substring(1));
On 2012/03/28 01:16:17, nweiz wrote:
> Is this really faster? That's kind of depressing.
> 
> How sure are we that Javascript's notion of word characters is identical to
> CSS's notion?
Sad but true.... This is a LOT faster for getElementById on Chrome and other
browsers.

You're right. I missed an edge case... document.getElementById('23') works but
document.querySelector('#23') throws an exception
changed the regexp to only special case for element ids that start with a
letter.

https://chromiumcodereview.appspot.com/9732019/diff/12001/lib/dom/templates/h...
lib/dom/templates/html/impl/impl_Document.darttemplate:22: _ElementImpl
$dom_querySelector(String selectors) native "return
this.querySelector(selectors);";
On 2012/03/28 01:16:17, nweiz wrote:
> Line length. Also, why isn't this autogenerated?

Obeying line length for these native lines does more harm than good.... Also,
all of the autogen native lines in the output will be >80 chars anyway.  This
isn't auto generated yet because it is marked as hidden in systemhtml.py.  added
a todo to clean that up.

https://chromiumcodereview.appspot.com/9732019/diff/12001/lib/dom/templates/h...
File lib/dom/templates/html/impl/impl_Node.darttemplate (right):

https://chromiumcodereview.appspot.com/9732019/diff/12001/lib/dom/templates/h...
lib/dom/templates/html/impl/impl_Node.darttemplate:88: throw new
UnsupportedOperationException("Cannot setRange on immutable List.");
On 2012/03/28 01:16:17, nweiz wrote:
> Line lengths.

Done.

[vsm, 2012-03-28 18:00:14]

I'm not sure it's worth having html and htmlfast.  I'd suggest replacing html
with htmlfast instead (and keeping the html name).

https://chromiumcodereview.appspot.com/9732019/diff/13001/samples/third_party...
File samples/third_party/dromaeo/tests/dom-query-html.dart (right):

https://chromiumcodereview.appspot.com/9732019/diff/13001/samples/third_party...
samples/third_party/dromaeo/tests/dom-query-html.dart:56: }
Please drop the loop change.  It's too far from the JS we're comparing against.

The type change (ElementList) is fine.

Ditto below.

https://chromiumcodereview.appspot.com/9732019/diff/13001/samples/third_party...
File samples/third_party/dromaeo/tests/dom-query.html (right):

https://chromiumcodereview.appspot.com/9732019/diff/13001/samples/third_party...
samples/third_party/dromaeo/tests/dom-query.html:47: for (var j = 0, len =
elems.length; j < len; j++) {
Please don't change this file.  It's the unmodified upstream Dromaeo.

[nweiz, 2012-03-28 20:29:33]

lgtm

https://chromiumcodereview.appspot.com/9732019/diff/12001/lib/dom/scripts/sys...
File lib/dom/scripts/systemhtml.py (right):

https://chromiumcodereview.appspot.com/9732019/diff/12001/lib/dom/scripts/sys...
lib/dom/scripts/systemhtml.py:890: if ((getter and html_getter_name !=
getter.id) or
On 2012/03/28 17:52:54, Jacob wrote:
> On 2012/03/28 01:16:17, nweiz wrote:
> > None of these parens are really necessary, but even if you want to use the
> inner
> > ones for clarity the outer ones are still redundant.
> > 
> > Also below.
> 
> I agree that the inner ones aren't needed but I prefer them for clarity.
> THe outer one is necessary. Otherwise you get a python syntax error... adding
> those extra parens is the cost of not having semicolons.

Oh, I see. I assumed Python was smart enough to follow expressions across lines.
It works in Ruby :-p.

https://chromiumcodereview.appspot.com/9732019/diff/12001/lib/dom/templates/h...
File lib/dom/templates/html/impl/impl_Document.darttemplate (right):

https://chromiumcodereview.appspot.com/9732019/diff/12001/lib/dom/templates/h...
lib/dom/templates/html/impl/impl_Document.darttemplate:17: return
$dom_getElementById(selectors.substring(1));
On 2012/03/28 17:52:54, Jacob wrote:
> On 2012/03/28 01:16:17, nweiz wrote:
> > Is this really faster? That's kind of depressing.
> > 
> > How sure are we that Javascript's notion of word characters is identical to
> > CSS's notion?
> Sad but true.... This is a LOT faster for getElementById on Chrome and other
> browsers.
> 
> You're right. I missed an edge case... document.getElementById('23') works but
> document.querySelector('#23') throws an exception
> changed the regexp to only special case for element ids that start with a
> letter.

I'm not sure how much we want to care about false negatives, but this misses ids
that start with "_" as well as non-ascii characters and escape sequences. "_" at
least would be easy to support.

[Jacob, 2012-03-28 21:01:28]

https://chromiumcodereview.appspot.com/9732019/diff/12001/lib/dom/scripts/sys...
File lib/dom/scripts/systemhtml.py (right):

https://chromiumcodereview.appspot.com/9732019/diff/12001/lib/dom/scripts/sys...
lib/dom/scripts/systemhtml.py:890: if ((getter and html_getter_name !=
getter.id) or
I wish it was... that is one of the things that annoys me the most when I write
python code.

On 2012/03/28 20:29:33, nweiz wrote:
> On 2012/03/28 17:52:54, Jacob wrote:
> > On 2012/03/28 01:16:17, nweiz wrote:
> > > None of these parens are really necessary, but even if you want to use the
> > inner
> > > ones for clarity the outer ones are still redundant.
> > > 
> > > Also below.
> > 
> > I agree that the inner ones aren't needed but I prefer them for clarity.
> > THe outer one is necessary. Otherwise you get a python syntax error...
adding
> > those extra parens is the cost of not having semicolons.
> 
> Oh, I see. I assumed Python was smart enough to follow expressions across
lines.
> It works in Ruby :-p.

https://chromiumcodereview.appspot.com/9732019/diff/12001/lib/dom/templates/h...
File lib/dom/templates/html/impl/impl_Document.darttemplate (right):

https://chromiumcodereview.appspot.com/9732019/diff/12001/lib/dom/templates/h...
lib/dom/templates/html/impl/impl_Document.darttemplate:17: return
$dom_getElementById(selectors.substring(1));
I've added a comment that this has false negatives but that is fine.  False
positives cause logic bugs, false negatives just make the code a little slower
than it could be. Added _ to the RegExp as I agree that could possibly be a
common case and there is little cost to check for it. 

On 2012/03/28 20:29:33, nweiz wrote:
> On 2012/03/28 17:52:54, Jacob wrote:
> > On 2012/03/28 01:16:17, nweiz wrote:
> > > Is this really faster? That's kind of depressing.
> > > 
> > > How sure are we that Javascript's notion of word characters is identical
to
> > > CSS's notion?
> > Sad but true.... This is a LOT faster for getElementById on Chrome and other
> > browsers.
> > 
> > You're right. I missed an edge case... document.getElementById('23') works
but
> > document.querySelector('#23') throws an exception
> > changed the regexp to only special case for element ids that start with a
> > letter.
> 
> I'm not sure how much we want to care about false negatives, but this misses
ids
> that start with "_" as well as non-ascii characters and escape sequences. "_"
at
> least would be easy to support.

https://chromiumcodereview.appspot.com/9732019/diff/13001/samples/third_party...
File samples/third_party/dromaeo/tests/dom-query-html.dart (right):

https://chromiumcodereview.appspot.com/9732019/diff/13001/samples/third_party...
samples/third_party/dromaeo/tests/dom-query-html.dart:56: }
On 2012/03/28 18:00:14, vsm wrote:
> Please drop the loop change.  It's too far from the JS we're comparing
against.
> 
> The type change (ElementList) is fine.
> 
> Ditto below.

Done.

https://chromiumcodereview.appspot.com/9732019/diff/13001/samples/third_party...
File samples/third_party/dromaeo/tests/dom-query.html (right):

https://chromiumcodereview.appspot.com/9732019/diff/13001/samples/third_party...
samples/third_party/dromaeo/tests/dom-query.html:47: for (var j = 0, len =
elems.length; j < len; j++) {
On 2012/03/28 18:00:14, vsm wrote:
> Please don't change this file.  It's the unmodified upstream Dromaeo.

Done.

[vsm, 2012-03-28 21:47:48]

LGTM!

[arv (Not doing code reviews), 2012-03-28 22:37:48]

FYI

https://chromiumcodereview.appspot.com/9732019/diff/9014/lib/dom/templates/ht...
File lib/dom/templates/html/impl/impl_Node.darttemplate (right):

https://chromiumcodereview.appspot.com/9732019/diff/9014/lib/dom/templates/ht...
lib/dom/templates/html/impl/impl_Node.darttemplate:70: bool isEmpty() =>
this.length == 0;
This should be this.$dom_firstChild != null

length is O(n)