90756 - DemoteForm takes 1.7% of ibench time

Status: VERIFIED FIXED

(Core :: DOM: HTML Parser, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I just took a realtime jprof profile of a complete run of the ibench HTML load
speed test, and 1.7% of the time was within SinkContext::DemoteContainer.  (It
was in 512 stacks out of 30,300 non-polling stacks in the profile.)

Of those 512 stacks:
 * 334 are calling nsGenericHTMLContainerElement::AppendChild
 * 110 are calling nsGenericHTMLContainerElement::RemoveChild
 * 51 are calling SinkContext::FlushTags

I was wondering why we actually need to move content around.  Would it be
possible to come up with a solution where all the form controls are all parented
to the correct parent (i.e., none, or a different form other than one
incorrectly terminated within a table) and we didn't have to rearrange the
content model as we were going?

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: jprof profile showing only time within DemoteContainer

Comment 2

[Boris Zbarsky [:bzbarsky]]

A correct removeChild() if a form's child should make all controls in that
child's subtree drop pointers to the form.  The current form control handling in
DemoteContainer would break in those circumstances.  So this is blocking bug 98487.

Comment 3

[Asa Dotzler [:asa]]

Bugs targeted at mozilla1.0 without the mozilla1.0 keyword moved to mozilla1.0.1 
(you can query for this string to delete spam or retrieve the list of bugs I've 
moved)

Comment 4

[John Keiser (jkeiser)]

It seems like this should be possible, but I have heard it has been tried before.

Note that DemoteContainer() is now DemoteForm(), since it only works on forms
anyway.

Comment 5

[John Keiser (jkeiser)]

Attachment: Patch to remove DemoteForm

This patch removes DemoteForm entirely, per conversations with Vidur and
Harish.  In fact I modified it a little from what I discussed with you two,
specifically instead of adding that "close form immediately after X tag"
feature, I just let the content sink handle automatic closing of the form tag.

Here is how forms work with this patch:
1. When <form> is found, mCurrentForm is set and the container is opened.
2. When </form> is found, mCurrentForm is nulled and IF we are balanced (if we
would not be closing any other tags inadvertently), we close the container. 
Otherwise, we wait for the sink to close the form container automatically,
which it will do as soon as it finds the close tag for the form's parent (same
as any other unbalanced container).

Big Monkey Testcase and screenshots shortly.

Comment 6

[John Keiser (jkeiser)]

Attachment: Testcase

This testcase shows the three possible cases: one balanced form and two
unbalanced forms.

Comment 7

[John Keiser (jkeiser)]

Attachment: Screenshot WITH fix

Screenshot on build with fix applied

Comment 8

[John Keiser (jkeiser)]

Attachment: Screenshot WITHOUT fix

Screenshot WITHOUT fix

Comment 9

[John Keiser (jkeiser)]

Reassigning to me.

Comment 10

[John Keiser (jkeiser)]

Attachment: Patch v1.0.1

Going through smoketests I noticed a crash with iframes and realized I had
exposed issues with iframe that I had not addressed in other iframe bugs (the
big one being that you should *never* destroy a the frameloader while the frame
is still around).  Iframe issues fixed.

TESTING ON THIS PATCH:
- smoketest - went to every smoketest site and did searches on each one
- testcases on this bug
- bugzilla.mozilla.org (all updates to this bug done with the patched build)
- tinderbox (including verifying that the iframe popup works)
- bug 98487
- bug 52334

I think we are clear.

Harish?  Vidur?  Reviews? :)

Comment 11

[vidur (gone)]

Comment on attachment 82178 [details] [diff] [review]
Patch v1.0.1

sr=vidur on all but the iframe/frameloader stuff. jkeiser says he'll get jst to
look at it.

Comment 12

[vidur (gone)]

Comment on attachment 82178 [details] [diff] [review]
Patch v1.0.1

sr=vidur on all but the iframe/frameloader stuff. jkeiser says he'll get jst to
look at it.

Comment 13

[harishd]

Comment on attachment 82178 [details] [diff] [review]
Patch v1.0.1

r=harishd

Comment 14

[John Keiser (jkeiser)]

Attachment: Patch v1.0.2

Patch to fix an issue JST brought up, that the change of
nsGenericHTML(Container|Leaf)FormElement::mForm to an nsCOMPtr creates a
circular dependency.  This should not create a leak, as it turns out, because
we have code in to break the dependency anyway, but a circular dependency is
still a bad idea.

This is UNTESTED but compiles.	This is mainly for JST to verify and hopefully
sr= alongside vidur; I will test later on Bugzilla, my form tester and on these
testcases to verify that everything still works.  (It will. :)

Comment 15

[John Keiser (jkeiser)]

Attachment: Patch v1.0.3 (Brown Bag Release)

OK, so one should use ->Release() instead of NS_RELEASE if one does not wish
the pointer to be cleared :)  This has been tested specifically against
Bugzilla, the testcase in this bug, and a form at my website.  Uploading with
patched build of course.

Comment 16

[Johnny Stenback (:jst)]

Comment on attachment 82820 [details] [diff] [review]
Patch v1.0.3 (Brown Bag Release)

sr=jst, but please loose the changes in neGenericHTMLElement.h, you don't need
those any more.

Comment 17

[John Keiser (jkeiser)]

Comment on attachment 82820 [details] [diff] [review]
Patch v1.0.3 (Brown Bag Release)

Carrying r= (small changes, I let Harish know what changes before the first r=)

Comment 18

[John Keiser (jkeiser)]

Fixed on trunk, and gave us 10ms pageload time out of 1200 there.  I have not
run iBench, either, but it is literally impossible for DemoteForm to take 1.7%
of the time on it because DemoteForm no longer exists :)

Comment 19

[John Keiser (jkeiser)]

Nominating in case adt wants the 10ms pageload improvement (between 0.5% and 1%).

Comment 20

[kinmoz]

Just wanted to notate that the removal of DemoteForm() fixes a problem seen with
some sites like:

  http://www.google.com/advanced_group_search?hl=en

where a textwidget is programatically given focus, via JS, during the document
load, and then the caret disappears. (First reported in bug 51897 comment 11)

What was happening was that DemoteForm() was triggering frame destruction and
reconstruction, after a textwidget frame was given focus. After the frames were
recreated the textwidget was totally unaware that it had focus, since no focus
event was dispatched during it's lifetime, so it didn't know it should display
it's caret.

Comment 21

[Kevin McCluskey (gone)]

The patch in bug 143917 must be checked in if the patch in this bug is checked in.

Comment 22

[scottputterman]

adding adt1.0.0-.  Let's get this in the next release.

Comment 23

[Moied]

Verified fixed with trunk build ID 20020529 using winxp

Comment 24

[Niels Aufbau]

*** Bug 64450 has been marked as a duplicate of this bug. ***

Comment 25

[Serge Gautherie (:sgautherie)]

Acting on Duplicate resolution from bug 64450 comment 29:

*Moving 'Blocks: bug 64451' from bug 64450 to bug 90756.
 (Take further action as needed.)