Speeding up Node.nodes.addAll

Speeding up Node.nodes.addAll

During profiling of the Dart API app I noticed quite a non-trivial amount of time and garbage in nodes.addAll. This case can be optimized for the case where we know that both lists are node lists.

BUG=

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

[blois, 2013-02-28 19:39:33]

Note that this copy was added to fix http://dartbug.com/6684, but that the
workarounds for this bug still have not been removed from web_ui (I've fixed
this locally but not yet submitted). 

So right now in web_ui we make 2 clones of the list before adding. Fixing this
and web_ui gets this down to zero.

[Siggi Cherem (dart-lang), 2013-02-28 19:43:32]

lgtm

https://codereview.chromium.org/12382022/diff/2001/tools/dom/templates/html/i...
File tools/dom/templates/html/impl/impl_Node.darttemplate (right):

https://codereview.chromium.org/12382022/diff/2001/tools/dom/templates/html/i...
tools/dom/templates/html/impl/impl_Node.darttemplate:72: if (iterable is
_ChildNodeListLazy) {
is it worth doing this also for other types like List<Node>?

[blois, 2013-02-28 20:45:58]

https://codereview.chromium.org/12382022/diff/2001/tools/dom/templates/html/i...
File tools/dom/templates/html/impl/impl_Node.darttemplate (right):

https://codereview.chromium.org/12382022/diff/2001/tools/dom/templates/html/i...
tools/dom/templates/html/impl/impl_Node.darttemplate:72: if (iterable is
_ChildNodeListLazy) {
On 2013/02/28 19:43:32, Siggi Cherem (dart-lang) wrote:
> is it worth doing this also for other types like List<Node>?

The optimized loop requires that it be a browser node list as it relies on the
behavior of the items getting automatically removed from the other list.

The general for..in loop could potentially be optimized to avoid the generation
of an iterator for lists, but I'd prefer to delay a change like that until we
have prescriptive guidance around List iterating.

[Jennifer Messerly, 2013-02-28 21:08:16]

lgtm

[blois, 2013-03-01 00:03:41]

Committed patchset #1 manually as r19268 (presubmit successful).

[Siggi Cherem (dart-lang), 2013-03-01 00:05:27]

lgtm

https://codereview.chromium.org/12382022/diff/2001/tools/dom/templates/html/i...
File tools/dom/templates/html/impl/impl_Node.darttemplate (right):

https://codereview.chromium.org/12382022/diff/2001/tools/dom/templates/html/i...
tools/dom/templates/html/impl/impl_Node.darttemplate:72: if (iterable is
_ChildNodeListLazy) {
On 2013/02/28 20:45:58, blois wrote:
> On 2013/02/28 19:43:32, Siggi Cherem (dart-lang) wrote:
> > is it worth doing this also for other types like List<Node>?
> 
> The optimized loop requires that it be a browser node list as it relies on the
> behavior of the items getting automatically removed from the other list.
> 
> The general for..in loop could potentially be optimized to avoid the
generation
> of an iterator for lists, but I'd prefer to delay a change like that until we
> have prescriptive guidance around List iterating.

I see, thanks! - somehow the [0] below didn't sink in =).

[sra1, 2013-03-01 01:17:59]

A couple of things that might make a few percent more difference since they
avoid interceptors.

https://codereview.chromium.org/12382022/diff/2001/tools/dom/templates/html/i...
File tools/dom/templates/html/impl/impl_Node.darttemplate (right):

https://codereview.chromium.org/12382022/diff/2001/tools/dom/templates/html/i...
tools/dom/templates/html/impl/impl_Node.darttemplate:73: if (iterable._this !=
_this) {
if (!identical(iterable._this, _this))

https://codereview.chromium.org/12382022/diff/2001/tools/dom/templates/html/i...
tools/dom/templates/html/impl/impl_Node.darttemplate:76:
_this.$dom_appendChild(iterable[0]);
iterable.first should be slightly faster.

[sra1, 2013-03-01 01:28:46]

https://codereview.chromium.org/12382022/diff/2001/tools/dom/templates/html/i...
File tools/dom/templates/html/impl/impl_Node.darttemplate (right):

https://codereview.chromium.org/12382022/diff/2001/tools/dom/templates/html/i...
tools/dom/templates/html/impl/impl_Node.darttemplate:76:
_this.$dom_appendChild(iterable[0]);
On 2013/03/01 01:17:59, sra1 wrote:
> iterable.first should be slightly faster.

or, better, iterable._this.$dom_firstChild