alexander :surkov (:asurkov) Assignee
	Description • 8 years ago

spun off bug 1220502

	Marco Zehe (:MarcoZ)
	Comment 1 • 8 years ago

Is this the same patch as in bug 1220502? I built with it yesterday, and didn't notice any performance gain with my two use cases IRCCloud and Twitter. Is this expected at this stage?

	alexander :surkov (:asurkov) Assignee
	Comment 2 • 8 years ago

(In reply to Marco Zehe (:MarcoZ) from comment #1)
> Is this the same patch as in bug 1220502?

yes

> I built with it yesterday, and
> didn't notice any performance gain with my two use cases IRCCloud and
> Twitter. Is this expected at this stage?

I don't have numbers. But it has to make things much faster if you add many many children under same node. You may notice a difference when bug 1220502 is fixed though.

	Marco Zehe (:MarcoZ)
	Comment 3 • 8 years ago

(In reply to alexander :surkov from comment #2)
> (In reply to Marco Zehe (:MarcoZ) from comment #1)
> > I built with it yesterday, and
> > didn't notice any performance gain with my two use cases IRCCloud and
> > Twitter. Is this expected at this stage?
> I don't have numbers. But it has to make things much faster if you add many
> many children under same node. You may notice a difference when bug 1220502
> is fixed though.

OK! Does the try push you posted to bug 1220502 contain both patches, the one you re-filed here and the one you just attached to that other bug? If so, I will give that a try and give you results.

	alexander :surkov (:asurkov) Assignee
	Comment 4 • 8 years ago

yes, this try has them both  https://archive.mozilla.org/pub/firefox/try-builds/surkov.alexander@gmail.com-1683445b3a3beab18f60571c885575568f5c9b1f/

	Marco Zehe (:MarcoZ)
	Comment 5 • 8 years ago

I just tried this build, and here are a few observations:

1. With my two use cases, it only seems marginally faster.

2. It crashes consistently upon exit. But because this is a try build, there are no stacks I believe. Anyway the report is:
https://crash-stats.mozilla.com/report/index/5645aa7b-9349-4d12-bc67-d59da2160126

	Trevor Saunders (:tbsaunde)
	Comment 6 • 8 years ago

can we get some quick numbers on how often we insert an element that's already in the table?

	alexander :surkov (:asurkov) Assignee
	Comment 7 • 8 years ago

(In reply to Marco Zehe (:MarcoZ) from comment #5)
> I just tried this build, and here are a few observations:
> 
> 1. With my two use cases, it only seems marginally faster.

that has to be something else then

> 2. It crashes consistently upon exit. But because this is a try build, there
> are no stacks I believe.

yep, no stack, what do you use for testing?

(In reply to Trevor Saunders (:tbsaunde) from comment #6)
> can we get some quick numbers on how often we insert an element that's
> already in the table?

it'd be interesting to see some statistics, not sure though if anybody is up to pursue the numbers

	Marco Zehe (:MarcoZ)
	Comment 8 • 8 years ago

(In reply to alexander :surkov from comment #7)
> (In reply to Marco Zehe (:MarcoZ) from comment #5)
> > 2. It crashes consistently upon exit. But because this is a try build, there
> > are no stacks I believe.
> 
> yep, no stack, what do you use for testing?
The x64 release build with NVDA on Win 10.

	Trevor Saunders (:tbsaunde)
	Comment 9 • 8 years ago

> (In reply to Trevor Saunders (:tbsaunde) from comment #6)
> > can we get some quick numbers on how often we insert an element that's
> > already in the table?
> 
> it'd be interesting to see some statistics, not sure though if anybody is up
> to pursue the numbers

It should only take 20 minutes or something to get some quick numbers.  I'm not particularly convinced we should make this change without any data showing it helps.

	alexander :surkov (:asurkov) Assignee
	Comment 10 • 8 years ago

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

Review of attachment 8712171 [details] [diff] [review]:
-----------------------------------------------------------------

David, how would we proceed on this? I'm a bit reluctant to waste time on proving the obvious, but taking into account the importance of the bug, I'm good to obtain some results if you find it helpful.

	David Bolter [:davidb] (NeedInfo me for attention)
	Comment 11 • 8 years ago

(In reply to alexander :surkov from comment #10)
> I'm
> good to obtain some results if you find it helpful.

Yes please and thanks! :)

	alexander :surkov (:asurkov) Assignee
	Comment 12 • 8 years ago

Here's a script that inserts elements into a container.

const kCount = 10000;
var container = document.getElementById('container');
for (var i = 0; i < kCount; i++) {
  var el = document.createElement('div');
  var idx = Math.ceil(Math.random() * container.childNodes.length);
  el.textContent = `hey ${i}, before ${idx}`;
  if (idx == 0) {
    container.appendChild(el);
  }
  else {
    container.insertBefore(el, container.childNodes.item(idx));
  }
}

In case when layout is smart enough to combine the changes, we get a few percents improvement. So the nightly took 762.28 ms in average (from 3 attempts) for the first script run, the patched build took 750.86 ms, which is about 3% improvement. For second (subsequent) script run, when layout keeps us busy with the notifications, the nightly build took 105489.41 ms vs 1373.35 ms for the patched build, which is more than 70 times faster.

	David Bolter [:davidb] (NeedInfo me for attention)
	Comment 13 • 8 years ago

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

Review of attachment 8712171 [details] [diff] [review]:
-----------------------------------------------------------------

The numbers sounds good (and our IRC discussion). Hoping Trevor can review for nits/errors.

	Trevor Saunders (:tbsaunde)
	Comment 14 • 8 years ago

(In reply to alexander :surkov from comment #12)
> Here's a script that inserts elements into a container.
> 
> const kCount = 10000;
> var container = document.getElementById('container');
> for (var i = 0; i < kCount; i++) {
>   var el = document.createElement('div');
>   var idx = Math.ceil(Math.random() * container.childNodes.length);
>   el.textContent = `hey ${i}, before ${idx}`;
>   if (idx == 0) {
>     container.appendChild(el);
>   }
>   else {
>     container.insertBefore(el, container.childNodes.item(idx));
>   }
> }
> 
> In case when layout is smart enough to combine the changes, we get a few
> percents improvement. So the nightly took 762.28 ms in average (from 3
> attempts) for the first script run, the patched build took 750.86 ms, which

well, it really doesn't seem that smart, I can't see a reason it needs to call ContentInserted() more than once, so this patch really shouldn't help that case at all, and should probably be a small regression.

> is about 3% improvement. For second (subsequent) script run, when layout
> keeps us busy with the notifications, the nightly build took 105489.41 ms vs
> 1373.35 ms for the patched build, which is more than 70 times faster.

ok, this sort of case makes sense more, but really we should fix this by getting rid of InvalidateChildren() and not caching everything multiple times. THis probably wouldn't help at all then accept maybe locality a little.  There's really no good reason we can't just do that, but anyway I'm now convinced this does help some cases for the moment.

	Trevor Saunders (:tbsaunde)
	Comment 15 • 8 years ago

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

>+  nsTArray<nsCOMPtr<nsIContent> >* list =

nit, >>*

>+    mContentInsertions.LookupOrAdd(aContainer);
>+
>+  bool needsProcessing = false;
>+  nsIContent* node = aStartChildNode;
>+  while (node != aEndChildNode) {
>+    // Notification triggers for content insertion even if no content was
>+    // actually inserted, check if the given content has a frame to discard
>+    // this case early.
>+    if (node->GetPrimaryFrame()) {
>+      if (list->AppendElement(node))
>+        needsProcessing = true;
>+    }
>+    node = node->GetNextSibling();
>+  }

it would be easier to see nothing changes here if this was its own patch.

>   /**
>-   * Storage for content inserted notification information.
>+   * A pending accessible tree update notifications for content insertions.
>    */

s/a //

>+  nsClassHashtable<nsRefPtrHashKey<Accessible>,
>+                   nsTArray<nsCOMPtr<nsIContent> > > mContentInsertions;

nit, >>>

	alexander :surkov (:asurkov) Assignee
	Comment 16 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3becf26d4e94

	alexander :surkov (:asurkov) Assignee
	Comment 17 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8352f3ee456a

	alexander :surkov (:asurkov) Assignee
	Comment 18 • 8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/c2aeece5eb1067f08cc5e43d0e53b6b694f27e28
Bug 1242989 - keep content insertions in a hash, r=tbsaunde

	Carsten Book [:Tomcat]
	Comment 19 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/c2aeece5eb10