Bug 48719 – HTML5 TreeBuilder regressed a Peacekeeper DOM test by 14% (was 40%)

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Simon Fraser (smfr)]

The 'domDynamicCreationCreateElement' test in the Peacekeeper suite <http://service.futuremark.com/peacekeeper/index.action> got 40% slower in <http://trac.webkit.org/changeset/65868>

[Simon Fraser (smfr)]

<rdar://problem/8613646>

[Simon Fraser (smfr)]

Here are the performance deltas between Safari 5.0.2 and TOT for Peacekeeper (using a scraped version of the tests that can be run locally):


Test                                TOT         r70732      Unit        % change
--------------------------------------------------------------------------------
renderChart                         57.62       98.93       fps         171.69%
renderGrid01                        73.11       140.85      fps         192.66%
renderPhysics                       55.53       49.77       fps         89.63%
experimentalRipple01                15.56       17.31       fps         111.26%
experimentalRipple02                6.61        6.68        fps         101.04%
experimentalMovie                   41.78       46.12       fps         110.39%
community01Encrypt                  77.25       139.66      fps         180.80%
community03Filter                   63.25       90.76       fps         143.49%
community04Sort                     59.94       72.71       fps         121.30%
arrayWeighted                       84745.76    86956.52    ops         102.61%
domDynamicCreationCreateElement     25974.03    16051.36    ops         61.80%
domDynamicCreationInnerHTML         19455.25    12886.60    ops         66.24%
domJQueryAttributeFilters           5376.34     4746.50     ops         88.28%
domJQueryBasics                     4729.50     4187.00     ops         88.53%
domJQueryContentFilters             668.00      759.50      ops         113.70%
domJQueryHierarchy                  1655.00     1423.00     ops         85.98%
stringChat                          47393.36    38759.69    ops         81.78%
stringDetectBrowser                 416666.67   270270.27   ops         64.86%
stringValidateForm                  769230.77   270270.27   ops         35.14%
stringWeighted                      55865.92    52083.33    ops         93.23%

[Simon Fraser (smfr)]

Created attachment 72443 [details]
The js for the test

This is the JS for the test. The harness just runs this as many times as it can in some time interval.

[Eric Seidel (no email)]

Yay more perf tests!

I'm certain this will be trivial to fix. Will look tomorrow.

[Simon Fraser (smfr)]

I noticed that the test doesn't ever force layout. If you force layout each time through the loop, the regression is more like 26%.

[Simon Fraser (smfr)]

TOT seems to be spending time in HTMLDocument::HTMLDocument() (from HTMLTreeBuilder::FragmentParsingContext::FragmentParsingContext()) than the pre-tree builder code. I see time spent in Document::initSecurityContext. Sees attached traces.

[Simon Fraser (smfr)]

Created attachment 72469 [details]
Profile focused on setInnerHTML, Safari 5.0.2

[Adam Barth]

How much of a win is switching to lazyAttach for the text nodes?

[Simon Fraser (smfr)]

Created attachment 72470 [details]
Profile focused on setInnerHTML, TOT

[Adam Barth]

We could cache a document for fragment parsing.  The new tree builder uses a dummy document for construction.

[Eric Seidel (no email)]

We do create a (dummy) HTMLDocument which the old parser didn't create.  As Adam points out, it's possible we could share those between parsings (although that sounds like a source of future bugs).

[Eric Seidel (no email)]

Would you please attach or email me the scraped version.  The benchmark seems to have no way to download it.

[Eric Seidel (no email)]

It's unclear to me if peacekeeper is clearing hiddenPlayground between runs, for example.  The JS you attached doesn't clear hiddenPlayground, but maybe the original test does?

I'm trying to run the benchmark so I can get a shark sample and see what's hot (your attached samples are nice, but not as useful as my own shark file would be).

[Eric Seidel (no email)]

I'm also not sure how you got the individual breakdown from the benchmarks.  The only link I got was:
http://service.futuremark.com/peacekeeper/results.action?key=4dex

Which doesn't show me any score breakdown.

This is clearly a benchmark designed to sell computers and not to actually benchmark anything or make browsers faster.

[Eric Seidel (no email)]

Created attachment 72480 [details]
Simple Benchmark shows TOT is 10x slower than Safari 5 at innerHTML in a loop

Because peacekeeper is useless, I wrote my own benchmark (based on WebCore/benchmarks/parser/html-parser.html) to test what I believe they're hitting.

This shows we're 10x slower than Safari 5 at doing innerHTML in a tight loop.

[Eric Seidel (no email)]

Actually TOT Safari is only 7 times slower.  Chrome 8 is 10x slower.  My machine may also have been busy building or something.  In either case, the test case attached shows we're much slower at innerHTML in a tight loop, which is likely the cause of this regression on peacekeeper.

[Simon Fraser (smfr)]

(In reply to comment #13)
> It's unclear to me if peacekeeper is clearing hiddenPlayground between runs, for example.  The JS you attached doesn't clear hiddenPlayground, but maybe the original test does?

It does not. It just something close to:

while (elapsedTime() < 2000) { benchmark.run(); }

Yes, Peacekeeper is a pain; I had to scrape the entire suite to be able to run it locally. Unfortunately I don't think I can attach the result here.

[Eric Seidel (no email)]

Created attachment 72488 [details]
work in progress, closes the gap to 2x as slow instead of 7x or 10x

[Eric Seidel (no email)]

OK.  I think my "work in progress" is close enough to a real patch to deserve some commentary from Dr. Barth.

We still have some work to do to optimize the innerHTML-in-a-loop case, but the important first step is to get a benchmark and iterate from there.

[Adam Barth]

Comment on attachment 72488 [details]
work in progress, closes the gap to 2x as slow instead of 7x or 10x

View in context: https://bugs.webkit.org/attachment.cgi?id=72488&action=review

The scary thing with this approach is if document has other state.  Maybe look at all the insertedIntoDocument calls?

> WebCore/html/parser/HTMLTreeBuilder.cpp:421
> +    s_sharedDummyDocument->setURL(baseURL);

I'm not sure this is quite the right thing.  We might need to add a setBaseURL.

[Eric Seidel (no email)]

Created attachment 72557 [details]
better wip

[Eric Seidel (no email)]

(In reply to comment #20)
> (From update of attachment 72488 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=72488&action=review
> 
> The scary thing with this approach is if document has other state.  Maybe look at all the insertedIntoDocument calls?

Agreed.  Both now, and in the future, this is a danger.

It may be better to look at not creating a document at all, but instead auditing all places where HTMLConstructionSite uses document() and instead have it be fragment-aware.

> > WebCore/html/parser/HTMLTreeBuilder.cpp:421
> > +    s_sharedDummyDocument->setURL(baseURL);
> 
> I'm not sure this is quite the right thing.  We might need to add a setBaseURL.

Maybe.  I'm not really sure what all the different urls mean.

[Eric Seidel (no email)]

Created attachment 72566 [details]
Patch

[Darin Adler]

Comment on attachment 72566 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=72566&action=review

Idea seems fine, but I think you could tighten up the execution of the idea a bit. I’ll say review+ but I think you should take another crack at it.

> WebCore/html/parser/HTMLTreeBuilder.cpp:397
> +// innerHTML is a common call).  So we use a shared dummy document.
> +// This sharing works because there can only ever be one fragment
> +// parser at any time.  Fragment parsing is synchronous and done
> +// only from the main thread.  It should be impossible for javascript

One space after periods, please.

> WebCore/html/parser/HTMLTreeBuilder.cpp:408
> +    static int s_sharedDummyDocumentMutex;

I think this should be debug-only.

> WebCore/html/parser/HTMLTreeBuilder.cpp:423
> +void DummyDocumentFactory::releaseDocument(HTMLDocument* dummyDocument)

I think this should be inline.

> WebCore/html/parser/HTMLTreeBuilder.cpp:426
> +    s_sharedDummyDocumentMutex--;

I think this should be debug-only.

> WebCore/html/parser/HTMLTreeBuilder.cpp:450
> +    // Setting the baseURL should work the same as it would have had we passed
> +    // it during HTMLDocument() construction, since the new document is empty.
> +    m_dummyDocumentForFragmentParsing->setURL(fragment->document()->baseURI());

I suggest having the createDummyDocument function take a URL, instead.

> WebCore/html/parser/HTMLTreeBuilder.h:221
> -        RefPtr<Document> m_dummyDocumentForFragmentParsing;
> +        // Use a shared dummy document to avoid expensive Document creation.
> +        // Hold a raw pointer to the document since there is no need to ref it.
> +        HTMLDocument* m_dummyDocumentForFragmentParsing;

Since this is a single shared dummy document, why do we need to store a pointer to it at all?

[Darin Adler]

Does this do a sufficient job of clearing out the document between uses to avoid data leakage?

[Eric Seidel (no email)]

(In reply to comment #24)
> (From update of attachment 72566 [details])

Thanks for the review.

> > WebCore/html/parser/HTMLTreeBuilder.cpp:397
> > +// innerHTML is a common call).  So we use a shared dummy document.
> > +// This sharing works because there can only ever be one fragment
> > +// parser at any time.  Fragment parsing is synchronous and done
> > +// only from the main thread.  It should be impossible for javascript
> 
> One space after periods, please.

I guess I've never really gotten on that bandwagon.  My english teacher of yore never beat that into me.  If we want that project-wide, we need to add that to check-webkit-style and the problem will go away. :)

> > WebCore/html/parser/HTMLTreeBuilder.cpp:408
> > +    static int s_sharedDummyDocumentMutex;
> 
> I think this should be debug-only.

OK.

> > WebCore/html/parser/HTMLTreeBuilder.cpp:423
> > +void DummyDocumentFactory::releaseDocument(HTMLDocument* dummyDocument)
> 
> I think this should be inline.

OK.  It's it already given that it's in the same file?

> > WebCore/html/parser/HTMLTreeBuilder.cpp:450
> > +    // Setting the baseURL should work the same as it would have had we passed
> > +    // it during HTMLDocument() construction, since the new document is empty.
> > +    m_dummyDocumentForFragmentParsing->setURL(fragment->document()->baseURI());
> 
> I suggest having the createDummyDocument function take a URL, instead.

Originally it did.  But then it was strange that we were copying the URL in one place and copying the compat mode in another.

> > WebCore/html/parser/HTMLTreeBuilder.h:221
> > -        RefPtr<Document> m_dummyDocumentForFragmentParsing;
> > +        // Use a shared dummy document to avoid expensive Document creation.
> > +        // Hold a raw pointer to the document since there is no need to ref it.
> > +        HTMLDocument* m_dummyDocumentForFragmentParsing;
> 
> Since this is a single shared dummy document, why do we need to store a pointer to it at all?

I considered that.  Storing the pointer on the FragmentParsingContext makes the context (mostly) obvious to the shared optimization underneath the DummyDocumentFactory.  It also makes it impossible to call some sharedDummyDocument() method w/o first having reset it with the right url, etc.

[Eric Seidel (no email)]

(In reply to comment #25)
> Does this do a sufficient job of clearing out the document between uses to avoid data leakage?

That's unclear.  Document() is HUGE.  So it's unclear what all needs to be done to a Document to re-use it safely.

Another alternative to this patch is to put a bunch of branches in HTMLConstructionSite to educate it about fragment mode and how it shouldn't always use the "document" it was provided.  However that's not how the spec was written and is likely to produce logic errors.

Interestingly, Hixie said to Adam privately that he never expected implementors to actually create a new document for every innerHTML.  I'm not sure what exactly he expected given the way it's specced though.

A follow-up patch after this one might be to make Document() creation faster and make it allocate more of its structures lazily.

My understanding is that the actual parsing of innerHTML content is *faster* with the new parser, but the overhead involved in innerHTML setup/teardown is more.  This patch is attempting to remove some of that overhead (which I think is a noble goal).  It's unclear how much of peacekeeper actually matters on the real web at all.

[Darin Adler]

(In reply to comment #26)
> OK.  It's it already given that it's in the same file?

Perhaps with some toolchains, but not with most. The gcc compiler, for example, will do this at higher optimization levels, but not the one we use for Mac OS X WebKit.

[Darin Adler]

(In reply to comment #27)
> Hixie said to Adam privately that he never expected implementors to actually create a new document for every innerHTML. I'm not sure what exactly he expected given the way it's specced though.

Seems like typical standards language. It’s telling you desired behavior, but not how to implement efficiently.

[Darin Adler]

(In reply to comment #27)
> A follow-up patch after this one might be to make Document() creation faster and make it allocate more of its structures lazily.

That sounds to me like the long way around, but maybe it will work.

[Darin Adler]

(In reply to comment #27)
> My understanding is that the actual parsing of innerHTML content is *faster* with the new parser, but the overhead involved in innerHTML setup/teardown is more.  This patch is attempting to remove some of that overhead (which I think is a noble goal).  It's unclear how much of peacekeeper actually matters on the real web at all.

I would chalk up points for Peacekeeper here, not against it. It caught a massive speed regression in the use of innerHTML to modify elements. I expect that setting innerHTML in performance critical code is relatively common on the web, and it’s probably common to do this repeatedly with many small bits of HTML. Lets not do the “attack the benchmark” thing!

[WebKit Commit Bot]

Comment on attachment 72566 [details]
Patch

Rejecting attachment 72566 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-3', 'apply-attachment', '--no-update', '--non-interactive', 72566]" exit_code: 2
Last 500 characters of output:
Core/benchmarks/parser/resources/performance-test.js
patching file Source/WebCore/benchmarks/parser/tiny-innerHTML.html
patching file Source/WebCore/html/parser/HTMLTreeBuilder.cpp
Hunk #1 succeeded at 394 (offset 6 lines).
Hunk #2 succeeded at 445 (offset 6 lines).
patching file Source/WebCore/html/parser/HTMLTreeBuilder.h
Hunk #1 succeeded at 218 (offset 2 lines).

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Darin Adler', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/7447109

[Darin Adler]

Eric, are you planning on returning to this? It seems like you dropped it a couple months ago.

[Eric Seidel (no email)]

Created attachment 80528 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 80528 [details]
Patch for landing

Clearing flags on attachment: 80528

Committed r77050: <http://trac.webkit.org/changeset/77050>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/77050 might have broken GTK Linux 64-bit Debug

[Ryosuke Niwa]

This patch seems to have broken Chromium's test_shell_tests on Mac:
http://chromesshgw.corp.google.com/i/chromium.webkit/builders/Webkit%20Mac10.5%20(dbg)(1)/builds/184/steps/test_shell_tests/logs/stdio

[----------] 3 tests from PluginTest
[ RUN      ] PluginTest.Refresh
ASSERTION FAILED: !m_dummyDocumentForFragmentParsing->hasChildNodes()
(/b/build/slave/webkit-mac-latest-dbg/build/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../html/parser/HTMLTreeBuilder.cpp:474 void WebCore::HTMLTreeBuilder::FragmentParsingContext::finished())
[91752:267:984460616493993:ERROR:process_util_posix.cc(106)] Received signal 11
	0   test_shell_tests                    0x00020d30 operator new(unsigned long, void*) + 103552
	1   test_shell_tests                    0x003958d5 start + 3449597
	2   libSystem.B.dylib                   0x95f5c2bb _sigtramp + 43
	3   ???                                 0xffffffff 0x0 + 4294967295
	4   test_shell_tests                    0x00e8f3ba start + 14958050
	5   test_shell_tests                    0x00e67fa7 start + 14797263
	6   test_shell_tests                    0x00e680af start + 14797527
	7   test_shell_tests                    0x00e68e38 start + 14800992
	8   test_shell_tests                    0x00e67eb3 start + 14797019
	9   test_shell_tests                    0x00e67eea start + 14797074
	10  test_shell_tests                    0x00e69d3b start + 14804835
	11  test_shell_tests                    0x0155b720 float WebCore::narrowPrecisionToCGFloat<double>(double) + 3136332
	12  test_shell_tests                    0x00dd4328 start + 14191952
	13  test_shell_tests                    0x00dd52ca start + 14195954
	14  test_shell_tests                    0x00fb2d4a start + 16152434
	15  test_shell_tests                    0x00aca331 start + 11005273
	16  test_shell_tests                    0x00ad2b84 start + 11040172
	17  test_shell_tests                    0x00ad2d9f start + 11040711
	18  test_shell_tests                    0x00a73203 start + 10648619
	19  test_shell_tests                    0x00a73306 start + 10648878
	20  ???                                 0x06fd828e 0x0 + 117277326
	21  ???                                 0x1ff33cd3 0x0 + 536034515

[Ryosuke Niwa]

I rolled out the patch in http://trac.webkit.org/changeset/77053 for now since neither eric nor darin was on IRC.  It could be that chromium port is doing something wrong (whole WebCore::narrowPrecisionToCGFloat<double>(double) business makes me suspicious that maybe we're calling this function outside of the main thread) but I couldn't be certain and rolling it out seemed a safer option rather than me trying to debug past midnight.

[Ryosuke Niwa]

FWIW, it seems like this also caused lots of crashes on XP bots:
http://build.webkit.org/results/Windows%20XP%20Debug%20(Tests)/r77049%20(24697)/results.html
http://build.webkit.org/results/Windows%20XP%20Debug%20(Tests)/r77050%20(24698)/results.html

and on GTK:
http://build.webkit.org/results/GTK%20Linux%2064-bit%20Debug/r77049%20(18721)/results.html
http://build.webkit.org/results/GTK%20Linux%2064-bit%20Debug/r77050%20(18722)/results.html

[Mihai Parparita]

(In reply to comment #24)
> WebCore/html/parser/HTMLTreeBuilder.cpp:450
> +    // Setting the baseURL should work the same as it would have had we passed
> +    // it during HTMLDocument() construction, since the new document is empty.
> +    m_dummyDocumentForFragmentParsing->setURL(fragment->document()->baseURI());

URI vs. baseURI for the temporary document used to parse fragments came up a while back (https://bugs.webkit.org/show_bug.cgi?id=45565#c4 and comment 6) and Adam said there are security reasons for explicitly setting the baseURI (instead of URI), so it seems we wouldn't want to regress that behavior.

(reopening since this was rolled out)

[Eric Seidel (no email)]

I bet some code (possibly plugin code) is grabbing the document off the parser and adding nodes to it?

Not sure though.  This should only be in the fragment case.

[Eric Seidel (no email)]

I'm not sure either the windows or the Gtk crashes had anything to do with this patch.

The ASSERT from the chromium plugin tests certainly does though.

[Ryosuke Niwa]

(In reply to comment #43)
> I'm not sure either the windows or the Gtk crashes had anything to do with this patch.

But both crashes had:

ASSERTION FAILED: !m_dummyDocumentForFragmentParsing->hasChildNodes()
(../../Source/WebCore/html/parser/HTMLTreeBuilder.cpp:474 void WebCore::HTMLTreeBuilder::FragmentParsingContext::finished())

[Eric Seidel (no email)]

(In reply to comment #44)
> (In reply to comment #43)
> > I'm not sure either the windows or the Gtk crashes had anything to do with this patch.
> 
> But both crashes had:
> 
> ASSERTION FAILED: !m_dummyDocumentForFragmentParsing->hasChildNodes()
> (../../Source/WebCore/html/parser/HTMLTreeBuilder.cpp:474 void WebCore::HTMLTreeBuilder::FragmentParsingContext::finished())

I must have misread the results.html, I didn't see that.

However I do see that crash locally.  It's a bogus assert.  Looking.

I thought this patch was all working long ago when I posted it.  I guess not.

[Eric Seidel (no email)]

This crash is caused, at least in part by our use of a cached documentElement pointer from http://trac.webkit.org/changeset/40014.

removeAllChildren does not call childrenChanged, so this pointer is never invalidated.  I'm not sure if removeAllChildren should be calling childrenChanged or not.

[Eric Seidel (no email)]

Created attachment 80971 [details]
Patch for landing

[Eric Seidel (no email)]

I found the bogus assert and fixed an ASSERT caused by using removeAllChildren instead of removeChildren.

Landing an updating patch through the commit-queue.

Running the tests with a Debug build shows no regressions as far as I can tell.  (There are 3-4 ASSERTS running the tests in debug mode right now due to regresisons in the last week!)

[Mihai Parparita]

(In reply to comment #41)
> URI vs. baseURI for the temporary document used to parse fragments came up a while back (https://bugs.webkit.org/show_bug.cgi?id=45565#c4 and comment 6) and Adam said there are security reasons for explicitly setting the baseURI (instead of URI), so it seems we wouldn't want to regress that behavior.

No thoughts on the above comment?

[Eric Seidel (no email)]

(In reply to comment #49)
> (In reply to comment #41)
> > URI vs. baseURI for the temporary document used to parse fragments came up a while back (https://bugs.webkit.org/show_bug.cgi?id=45565#c4 and comment 6) and Adam said there are security reasons for explicitly setting the baseURI (instead of URI), so it seems we wouldn't want to regress that behavior.
> 
> No thoughts on the above comment?

I don't understand what your asking.  If you or Adam have a specific requested code change or a test which is affected I'm happy to take action. :)

[Mihai Parparita]

(In reply to comment #50)
> I don't understand what your asking.  If you or Adam have a specific requested code change or a test which is affected I'm happy to take action. :)

You're setting the URI of the dummy document, not the base URI. For my patch which initially did the same thing, Adam said "It's probably harmless right now, but it's pretty dangerous.". My imagination is not good enough to picture how it could be dangerous in the future (unless we start to use the URI of the document to determine its origin), but since Adam had concerns about this then, I figured he would again. Perhaps not.

[Eric Seidel (no email)]

The unfortunate relality of this fix is that it's a hack.  :(  Another way to fix this would be to make Documents lighter weight to contruct.  I suspect that we're going to find a bunch more little tweaks we need to make to this change to make it safe/stable in the long-term since most of WebCore does not expect to re-use Document objects.

wrt to the baseURL stuff.  I remember looking at it some when I wrote this change months ago, but I'm not really an expert there.  Adam would know.  I'm happy to fix any bugs you find in this change, but right now I'm moving forward on the assumption that there are none for the moment. :)

[Adam Barth]

Why not just set the URL to about:blank and then set the base URL?

[Mihai Parparita]

(In reply to comment #53)
> Why not just set the URL to about:blank and then set the base URL?

There's no setter for base URL.

[Adam Barth]

Ok, well, I'm not sure what to tell you.  This whole thing is dangerous.

[Eric Seidel (no email)]

I think this optimization is going to end up causing us no end of pain.  Thankfully JS doesn't have access to the shared Document in any way.  (Unless there are JS events fired during fragment parsing?)  I suspect there is all sorts of state on Document which we're not clearing.

The proper way to do this speedup would be to make Document creation lighter weight.

It's possible we should just can this optimization and live with the speed regression until Document() can be made cheaper.

[Eric Seidel (no email)]

Comment on attachment 80971 [details]
Patch for landing

I'm canning this patch.  I don't think it's worth the risk.

[Eric Seidel (no email)]

Committed r77426: <http://trac.webkit.org/changeset/77426>

[Eric Seidel (no email)]

I landed the benchmark.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 80971 [details]:

http/tests/xmlhttprequest/cross-origin-no-authorization.html bug 33357 (author: [e-mail])
The commit-queue is continuing to process your patch.

[Andy Estes]

Have we discounted the approach of making HTMLConstructionSite fragment-aware (which appears to be the strategy taken by the old parser)? It seems like that approach would eliminate the regression without requiring a large Document refactoring. Are there downsides to that approach other than lack of pedantic spec compliance?

[Eric Seidel (no email)]

Making HTMLContructionSite fragment aware is certainly still a possibility.  It is also dangerous, although less so.  The old parser was a horrible mess, particularly due to that design choice.  But it's possible we could do it cleaner in HTMLConstructionSite.

[Andy Estes]

(In reply to comment #62)
> Making HTMLContructionSite fragment aware is certainly still a possibility.  It is also dangerous, although less so.  The old parser was a horrible mess, particularly due to that design choice.  But it's possible we could do it cleaner in HTMLConstructionSite.

It could also be treated as a stopgap until Document::create() can be sped up.

[Adam Barth]

Maybe use a generic ContainerNode instead of a Document?

[Eric Seidel (no email)]

Btw, I tested Opera 10 and Minefield with the benchmark I wrote and found the Opera is about 2x the speed of TOT, and Firefox seems too slow to even run the benchmark. :(

With the previous proposed fix, we would be faster than Opera.

Looking at the shark profile again to see if there is another way to approach fixing this.

[Eric Seidel (no email)]

(In reply to comment #65)
> Btw, I tested Opera 10 and Minefield with the benchmark I wrote and found the Opera is about 2x the speed of TOT, and Firefox seems too slow to even run the benchmark. :(
> 
> With the previous proposed fix, we would be faster than Opera.
> 
> Looking at the shark profile again to see if there is another way to approach fixing this.

Apologies, my statement about Minefield was inaccurate.  Minefield is about 10% faster than TOT WebKit as of writing on the tiny-innerHTML benchmark.

[Eric Seidel (no email)]

Committed r77436: <http://trac.webkit.org/changeset/77436>

[Andy Estes]

Created attachment 81685 [details]
Patch

[Andy Estes]

(In reply to comment #68)
> Created an attachment (id=81685) [details]
> Patch

This patch removes the creation of a dummy HTMLDocument and improves the Peacekeeper regression from ~40% to ~25%. Unfortunately this indicates that Document creation is not the sole cause of the regression :(

I'll do some additional profiling to try and track down the remaining regression.

[Eric Seidel (no email)]

Comment on attachment 81685 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=81685&action=review

> Source/WebCore/html/parser/HTMLConstructionSite.cpp:212
>          m_document->setCompatibilityMode(Document::QuirksMode);

Won't this get us in trouble?

[Eric Seidel (no email)]

I think this is an interesting attempt.  But I worry it's going to be wrong in subtle ways.

1.  uses of m_document which persist after your patch and cause us to change things on the owner document which we don't mean to.

2.  Things which the spec notes shoudl be added to the document node directly (and thus would end up discarded in most cases after the fragment parse.

We have to be careful with "root" since the spec already talksa bout a "root" which is different from yours:
http://www.w3.org/TR/html5/the-end.html

[Andy Estes]

(In reply to comment #70)
> (From update of attachment 81685 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=81685&action=review
> 
> > Source/WebCore/html/parser/HTMLConstructionSite.cpp:212
> >          m_document->setCompatibilityMode(Document::QuirksMode);
> 
> Won't this get us in trouble?

Good catch. There should be an early return there in the fragment case. I'll upload a corrected patch.

[Andy Estes]

(In reply to comment #71)
> I think this is an interesting attempt.  But I worry it's going to be wrong in subtle ways.
> 
> 1.  uses of m_document which persist after your patch and cause us to change things on the owner document which we don't mean to.

You mean after FragmentParsingContext::finished() is called?

> 
> 2.  Things which the spec notes shoudl be added to the document node directly (and thus would end up discarded in most cases after the fragment parse.
> 

Can you think of a specific example? This would be a good opportunity to add tests for these edge cases if this patch is getting them wrong.

> We have to be careful with "root" since the spec already talksa bout a "root" which is different from yours:
> http://www.w3.org/TR/html5/the-end.html

Yea, what I call root is really the parent of what the spec calls root. Any suggestions for a better name? m_container perhaps?

[Andy Estes]

Created attachment 81694 [details]
Patch

[Adam Barth]

Comment on attachment 81685 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=81685&action=review

I like the approach.  We'll need to carefully audit all uses of m_document by HTMLTreeBuilder and HTMLConstructionSite to make sure they're correct.

> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:-406
> -    : m_dummyDocumentForFragmentParsing(HTMLDocument::create(0, KURL(), fragment->document()->baseURI()))

We can probably remove the baseURI argument from the Document constructor.  I don't think its used by anybody else.  (Could be a follow-up patch.)

> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:418
> +    RefPtr<Node> firstElement = m_fragment->firstChild();
> +    while (firstElement && !firstElement->isElementNode())
> +        firstElement = firstElement->nextSibling();

I'm not sure I quite understand this part.  It's trying to compute the equivalent of the documentElement, right?

> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:420
> +    m_fragment->removeAllChildren();

Are we sure m_fragment is empty when we start parsing?  Maybe we should add an ASSERT to that effect?  I just forget where it comes from.

[Andy Estes]

(In reply to comment #75)
> (From update of attachment 81685 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=81685&action=review
> 
> I like the approach.  We'll need to carefully audit all uses of m_document by HTMLTreeBuilder and HTMLConstructionSite to make sure they're correct.
> 
> > Source/WebCore/html/parser/HTMLTreeBuilder.cpp:-406
> > -    : m_dummyDocumentForFragmentParsing(HTMLDocument::create(0, KURL(), fragment->document()->baseURI()))
> 
> We can probably remove the baseURI argument from the Document constructor.  I don't think its used by anybody else.  (Could be a follow-up patch.)

Okie doke.

> 
> > Source/WebCore/html/parser/HTMLTreeBuilder.cpp:418
> > +    RefPtr<Node> firstElement = m_fragment->firstChild();
> > +    while (firstElement && !firstElement->isElementNode())
> > +        firstElement = firstElement->nextSibling();
> 
> I'm not sure I quite understand this part.  It's trying to compute the equivalent of the documentElement, right?

Right. I could probably factor it such that I can share this routine with Document::documentElement().

> 
> > Source/WebCore/html/parser/HTMLTreeBuilder.cpp:420
> > +    m_fragment->removeAllChildren();
> 
> Are we sure m_fragment is empty when we start parsing?  Maybe we should add an ASSERT to that effect?  I just forget where it comes from.

That's a good point, and I'm not sure about that. I'll add an ASSERT and see what happens.

[Andy Estes]

(In reply to comment #75)
> (From update of attachment 81685 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=81685&action=review
> 
> > Source/WebCore/html/parser/HTMLTreeBuilder.cpp:420
> > +    m_fragment->removeAllChildren();
> 
> Are we sure m_fragment is empty when we start parsing?  Maybe we should add an ASSERT to that effect?  I just forget where it comes from.

I added 'ASSERT(!fragment->hasChildNodes())' to FragmentParsingContext's constructor and wasn't able to trigger it with any of our layout tests. A quick audit of the code revealed no sites where we append children to a DocumentFragment then call DocumentFragment::parseHTML/XML. I also don't believe it's possible to do this via JavaScript.

[Adam Barth]

Awesome!  Thanks.

[Geoffrey Garen]

Comment on attachment 81694 [details]
Patch

This revised patch looks good to me.

Andy, I think you should add a test that would catch this bug:

> Source/WebCore/html/parser/HTMLConstructionSite.cpp:212
>          m_document->setCompatibilityMode(Document::QuirksMode);

Adam, Eric, if you don't have more comments, I'm inclined to say r+ here.

[Adam Barth]

Comment on attachment 81694 [details]
Patch

It's hard to verify that this patch is correct because it involves auditing a lot of code, but I trust that Andy did it correctly.

[Eric Seidel (no email)]

I'd like to take another look through.

I'll do so in about an hour.

[Eric Seidel (no email)]

Comment on attachment 81694 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=81694&action=review

I'm surprised this works.

I would think there are cases which could cause us to modify nodes outside of the container (which shouldn't happen), like how we move <style> nodes to the head during parsing.  I'd have to check what states the TreeBuilder is set in when we enter fragment parsing.

Obviously this bug is important to Apple, given you're the 3rd engineer to try this.
I'm happy to be of help.

I am blind to some of the internal pressures which may have pointed you at this since 1.  I believe the benchmark is crap (and the 40% regression mostly meaningless).  2.  This is a really scary change.  But I'm happy to look at this with you again tomorrow and help you get this landed.

> Source/WebCore/ChangeLog:8
> +        No new tests. No change in behavior.

We have a fragment parsing performance test now, which I added earlier.  How does this change affect that benchmark (which I wrote to model Peacekeeper and make thsi change easier to write).

> Source/WebCore/html/parser/HTMLConstructionSite.h:46
> +    HTMLConstructionSite(Document*, ContainerNode*, FragmentScriptingPermission, bool isParsingFragment);

The ContainerNode parameter should be named.

> Source/WebCore/html/parser/HTMLConstructionSite.h:133
> +    ContainerNode* m_root;

We need to change this name to avoid confusion with "root" which is explicitly covered in the spec:
http://www.whatwg.org/specs/web-apps/current-work/multipage/the-end.html#fragment-case

Maybe m_attachmentRoot? with some sort of comment to explain what it's for.

> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:344
> +    , m_tree(document, document, FragmentScriptingAllowed, false)

Seems we should start thinking about having a separate constructor now that we have 3 implied parameters in the non-fragment case.

> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:418
> +    while (firstElement && !firstElement->isElementNode())
> +        firstElement = firstElement->nextSibling();

Why is this necessary?  What about if nothing was added to the fragment?  (like innerHTML="") won't we hit the later assert?

> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:420
> +    m_fragment->removeAllChildren();

removeAllChildren is differnet from removeChildern (doesn't send certain callbacks) and may be wrong here.  We recently had some use-after-free bug related to these being mis-matched.

Why do we even need to call this?  takeAllChildrenFrom should remove the children for us.

[Eric Seidel (no email)]

Comment on attachment 81694 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=81694&action=review

>> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:418
>> +        firstElement = firstElement->nextSibling();
> 
> Why is this necessary?  What about if nothing was added to the fragment?  (like innerHTML="") won't we hit the later assert?

Nevermind.  My ASSERT assumption was wrong.  We should never need to walk though, unless you have a test case to show otherwise?

[Eric Seidel (no email)]

Comment on attachment 81694 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=81694&action=review

>> Source/WebCore/html/parser/HTMLConstructionSite.h:133
>> +    ContainerNode* m_root;
> 
> We need to change this name to avoid confusion with "root" which is explicitly covered in the spec:
> http://www.whatwg.org/specs/web-apps/current-work/multipage/the-end.html#fragment-case
> 
> Maybe m_attachmentRoot? with some sort of comment to explain what it's for.

Re-reading the spec again, this may not actually be a big deal as it's only used in that one sentence as a "local variable" of sorts.

[Eric Seidel (no email)]

Comment on attachment 81694 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=81694&action=review

It's unclear to me if we need to clear m_root in HTMLConstructionSite::detach() or not.

> Source/WebCore/html/parser/HTMLConstructionSite.cpp:211
> +    if (m_isParsingFragment)

We should probably comment as to why we do this.  Just a simple // We can't modify the compatibility mode of the owning document, since we don't use a dummy document like the spec does.

Also, I believe this might now make us subtly wrong.  I don't know if compatibility mode effects parsing.  If does, than doing something like:
iframe.innerHTML = someWholeDocumentIncludingADocType; would now have different behavior after your change.

> Source/WebCore/html/parser/HTMLConstructionSite.cpp:226
>  void HTMLConstructionSite::insertCommentOnDocument(AtomicHTMLToken& token)

This is only hit in the following case:
void HTMLTreeBuilder::processComment(AtomicHTMLToken& token)
{
    ASSERT(token.type() == HTMLToken::Comment);
    if (m_insertionMode == InitialMode
        || m_insertionMode == BeforeHTMLMode
        || m_insertionMode == AfterAfterBodyMode
        || m_insertionMode == AfterAfterFramesetMode) {
        m_tree.insertCommentOnDocument(token);
        return;
    }

Which can be hit in fragment parsing if the fragment parser is being run w/o a context element (I don't remember on the top of my head what cases that entails).  It's possible we should change the name of this method to reflect that it should insert it on the more general "root" concept you're introducing.

> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:362
> +    , m_document(fragment->document())

We may not even need FragmentContext anymore after this change.

[Eric Seidel (no email)]

Comment on attachment 81694 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=81694&action=review

>> Source/WebCore/html/parser/HTMLConstructionSite.cpp:226
>>  void HTMLConstructionSite::insertCommentOnDocument(AtomicHTMLToken& token)
> 
> This is only hit in the following case:
> void HTMLTreeBuilder::processComment(AtomicHTMLToken& token)
> {
>     ASSERT(token.type() == HTMLToken::Comment);
>     if (m_insertionMode == InitialMode
>         || m_insertionMode == BeforeHTMLMode
>         || m_insertionMode == AfterAfterBodyMode
>         || m_insertionMode == AfterAfterFramesetMode) {
>         m_tree.insertCommentOnDocument(token);
>         return;
>     }
> 
> Which can be hit in fragment parsing if the fragment parser is being run w/o a context element (I don't remember on the top of my head what cases that entails).  It's possible we should change the name of this method to reflect that it should insert it on the more general "root" concept you're introducing.

After further inspection, I believe we *never* run the fragment parsing algorithm w/o a context pointer.

At one point createFragmentFromSource (in XSLTProcessor.cpp) did, but we found that that was wrong and make a fake body element instead.

We could probably make the contextElement a requirement at some point (and get Ian to modify the spec to say so).

>> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:362
>> +    , m_document(fragment->document())
> 
> We may not even need FragmentContext anymore after this change.

We probably still want to keep it for the ASSERTs it provides.

[Eric Seidel (no email)]

Comment on attachment 81694 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=81694&action=review

>> Source/WebCore/html/parser/HTMLConstructionSite.cpp:211
>> +    if (m_isParsingFragment)
> 
> We should probably comment as to why we do this.  Just a simple // We can't modify the compatibility mode of the owning document, since we don't use a dummy document like the spec does.
> 
> Also, I believe this might now make us subtly wrong.  I don't know if compatibility mode effects parsing.  If does, than doing something like:
> iframe.innerHTML = someWholeDocumentIncludingADocType; would now have different behavior after your change.

OK, yes this does change behavior.

Example:

A start tag whose tag name is "table"
If the Document is not set to quirks mode, and the stack of open elements has a p element in button scope, then act as if an end tag with the tag name "p" had been seen.

Unfortunately the html5lib test suite we used when writing the parser does not test fragment parsing very well.  It's possible the suite has been updated to cover more cases like this.

[Andy Estes]

(In reply to comment #82)
> (From update of attachment 81694 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=81694&action=review
> 
> I'm surprised this works.
> 
> I would think there are cases which could cause us to modify nodes outside of the container (which shouldn't happen), like how we move <style> nodes to the head during parsing.  I'd have to check what states the TreeBuilder is set in when we enter fragment parsing.
> 
> Obviously this bug is important to Apple, given you're the 3rd engineer to try this.
> I'm happy to be of help.
> 
> I am blind to some of the internal pressures which may have pointed you at this since 1.  I believe the benchmark is crap (and the 40% regression mostly meaningless).  2.  This is a really scary change.  But I'm happy to look at this with you again tomorrow and help you get this landed.

I appreciate your help and your thorough comments. I definitely want to get this patch right.

> 
> > Source/WebCore/ChangeLog:8
> > +        No new tests. No change in behavior.
> 
> We have a fragment parsing performance test now, which I added earlier.  How does this change affect that benchmark (which I wrote to model Peacekeeper and make thsi change easier to write).

Release build of r65867 (one rev. before new fragment parser was enabled):
avg 1702.4
median 1698
stdev 24.63209288712593
min 1677
max 1775

Release build of r77988 w/o patch:
avg 7849.1
median 7877.5
stdev 134.12005815686183
min 7657
max 8048

w/ patch:
avg 2353.85
median 2352
stdev 12.63833454217762
min 2336
max 2388

> 
> > Source/WebCore/html/parser/HTMLConstructionSite.h:46
> > +    HTMLConstructionSite(Document*, ContainerNode*, FragmentScriptingPermission, bool isParsingFragment);
> 
> The ContainerNode parameter should be named.

Will do.

> 
> > Source/WebCore/html/parser/HTMLConstructionSite.h:133
> > +    ContainerNode* m_root;
> 
> We need to change this name to avoid confusion with "root" which is explicitly covered in the spec:
> http://www.whatwg.org/specs/web-apps/current-work/multipage/the-end.html#fragment-case
> 
> Maybe m_attachmentRoot? with some sort of comment to explain what it's for.

That sounds fine. I read your comment later on that this isn't such a huge deal based on how the spec names it, but it probably still makes sense to rename it to avoid confusion.

> 
> > Source/WebCore/html/parser/HTMLTreeBuilder.cpp:344
> > +    , m_tree(document, document, FragmentScriptingAllowed, false)
> 
> Seems we should start thinking about having a separate constructor now that we have 3 implied parameters in the non-fragment case.

I had this thought too but decided against doing it for some reason. I'll make a separate constructor.

> 
> > Source/WebCore/html/parser/HTMLTreeBuilder.cpp:418
> > +    while (firstElement && !firstElement->isElementNode())
> > +        firstElement = firstElement->nextSibling();
> 
> Why is this necessary?  What about if nothing was added to the fragment?  (like innerHTML="") won't we hit the later assert?

I think it's necessary to avoid retrieving a non-element node such as a comment. This is what the call to Document::documentElement() did prior to the patch.

> 
> > Source/WebCore/html/parser/HTMLTreeBuilder.cpp:420
> > +    m_fragment->removeAllChildren();
> 
> removeAllChildren is differnet from removeChildern (doesn't send certain callbacks) and may be wrong here.  We recently had some use-after-free bug related to these being mis-matched.
> 
> Why do we even need to call this?  takeAllChildrenFrom should remove the children for us.

Good point. I misunderstood how takeAllChildrenFrom() worked.

[Andy Estes]

(In reply to comment #85)
> (From update of attachment 81694 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=81694&action=review
> 
> It's unclear to me if we need to clear m_root in HTMLConstructionSite::detach() or not.

It seems like we should. The DocumentParser can't come back to life after detach() is called, right?

> 
> > Source/WebCore/html/parser/HTMLConstructionSite.cpp:211
> > +    if (m_isParsingFragment)
> 
> We should probably comment as to why we do this.  Just a simple // We can't modify the compatibility mode of the owning document, since we don't use a dummy document like the spec does.

Okay.

> 
> Also, I believe this might now make us subtly wrong.  I don't know if compatibility mode effects parsing.  If does, than doing something like:
> iframe.innerHTML = someWholeDocumentIncludingADocType; would now have different behavior after your change.
> 
> > Source/WebCore/html/parser/HTMLConstructionSite.cpp:226
> >  void HTMLConstructionSite::insertCommentOnDocument(AtomicHTMLToken& token)
> 
> This is only hit in the following case:
> void HTMLTreeBuilder::processComment(AtomicHTMLToken& token)
> {
>     ASSERT(token.type() == HTMLToken::Comment);
>     if (m_insertionMode == InitialMode
>         || m_insertionMode == BeforeHTMLMode
>         || m_insertionMode == AfterAfterBodyMode
>         || m_insertionMode == AfterAfterFramesetMode) {
>         m_tree.insertCommentOnDocument(token);
>         return;
>     }
> 
> Which can be hit in fragment parsing if the fragment parser is being run w/o a context element (I don't remember on the top of my head what cases that entails).  It's possible we should change the name of this method to reflect that it should insert it on the more general "root" concept you're introducing.
> 
> > Source/WebCore/html/parser/HTMLTreeBuilder.cpp:362
> > +    , m_document(fragment->document())
> 
> We may not even need FragmentContext anymore after this change.

[Andy Estes]

(In reply to comment #87)
> (From update of attachment 81694 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=81694&action=review
> 
> >> Source/WebCore/html/parser/HTMLConstructionSite.cpp:211
> >> +    if (m_isParsingFragment)
> > 
> > We should probably comment as to why we do this.  Just a simple // We can't modify the compatibility mode of the owning document, since we don't use a dummy document like the spec does.
> > 
> > Also, I believe this might now make us subtly wrong.  I don't know if compatibility mode effects parsing.  If does, than doing something like:
> > iframe.innerHTML = someWholeDocumentIncludingADocType; would now have different behavior after your change.

To me, this seems contrary to the spec. 10.4.2 says that the dummy Document should assume the quirks behavior of the context's document, and if you're correct that we always have a context, then it would seem that we're doing the right thing here. If we're not, then how are we to rectify the cases where the context's document and the fragment's DOCTYPE specify different quirks behavior? That seems very confusing.

Our legacy parser ignored DOCTYPEs for fragments. I wonder how other browsers behave.

[Andy Estes]

I'll upload a new patch shortly that incorporates the latest feedback.

[Adam Barth]

> > It's unclear to me if we need to clear m_root in HTMLConstructionSite::detach() or not.
> 
> It seems like we should. The DocumentParser can't come back to life after detach() is called, right?

We null out a bunch of member variables in detach to make it easier to catch use-after-free bugs because they turn into null de-references instead of silently using unallocated memory.

[Andy Estes]

Created attachment 81879 [details]
Fragment DOCTYPE test case

This tests whether the browser obeys a fragment's DOCTYPE during parsing. I'm testing specifically whether an open p element is closed when a table element is encountered while in strict mode.

I've tested Safari 5, Opera 11, Firefox 3.6 and Minefield; all of them ignore DOCTYPEs in fragments. Interestingly, so does a WebKit nightly build even though it sounds like that wasn't our intention.

[Eric Seidel (no email)]

Your comment connecting the DOCTYPE behavior and the fact that we always have a contextElement was an insightful one.  I suspect that your behavior change *only* affects the case where we do not have a context element, as I suspect that DOCTYPE is ignored outside of the Initial state.

I'll have to look at hte treebuilder to see when it calls that "insert a doctype and possibly change the quirks mode" call in HTMLConstructionSite.  It's possible that it's never possible to have that called when we have a contextElement.

[Andy Estes]

(In reply to comment #82)
> (From update of attachment 81694 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=81694&action=review
>
> > Source/WebCore/html/parser/HTMLTreeBuilder.cpp:420
> > +    m_fragment->removeAllChildren();
> 
> removeAllChildren is differnet from removeChildern (doesn't send certain callbacks) and may be wrong here.  We recently had some use-after-free bug related to these being mis-matched.
> 
> Why do we even need to call this?  takeAllChildrenFrom should remove the children for us.

Ahh, it turns out that either removeChildren() or removeAllChildren() is necessary. takeAllChildrenFrom() does remove the children from the takee, but it doesn't remove the children's parent, and I'm interested in making the children of firstChild be direct descendants of the DocumentFragment.

I do need to determine if removeChildren() is more appropriate than removeAllChildren().

[Andy Estes]

Created attachment 81914 [details]
Patch

[Andy Estes]

(In reply to comment #96)
> Created an attachment (id=81914) [details]
> Patch

I understand there might still be open issues blocking review, but I wanted to update the patch to account for existing feedback.

[Eric Seidel (no email)]

Comment on attachment 81914 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=81914&action=review

I'm not sure I undertand the test, but in general I think this patch looks good.  I'm curious to know what the shark sample looks like for the remaining 24% :)

> Source/WebCore/dom/ContainerNode.h:174
> +inline Node* ContainerNode::firstElementChild() const

I'm surprised we don't already have this from ElementTraversal:
http://dev.w3.org/2006/webapi/ElementTraversal/publish/ElementTraversal.html

Should this return Element*?  Or I guess it can't easily since ContainerNode.h doesn't knwo about Element.h.

It certainly could at least return ContainerNode*. :)

> Source/WebCore/html/parser/HTMLConstructionSite.cpp:221
> +    // A fragment DOCTYPE shouldn't modify the compatibility mode of the owning

Probably don't need to wrap comment.

> Source/WebCore/html/parser/HTMLConstructionSite.h:47
> +    HTMLConstructionSite(Document*, ContainerNode* attachmentRoot, FragmentScriptingPermission, bool isParsingFragment);

isParsingFragment is now implied.
Might want to add a comment here to explain which is which.

> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:417
> +    RefPtr<Node> firstElement = m_fragment->firstElementChild();

We might want to call this documentElement and add a comment here to explain that this matches the spec.

> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:422
> +    ASSERT(firstElement->isContainerNode());

If firstElementChild returns containerNode then we don't need this assert.

[Andy Estes]

(In reply to comment #98)
> (From update of attachment 81914 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=81914&action=review
> 
> I'm not sure I undertand the test, but in general I think this patch looks good.  I'm curious to know what the shark sample looks like for the remaining 24% :)

The test does two things:

1) Verifies that a fragment is parsed according to the compatibility mode of its owner document, even if the fragment has a conflicting DOCTYPE.
2) Verifies that parsing a fragment's DOCTYPE does not change the compatibility mode of the owner document. This is a bug I almost introduced in my first patch that you caught.

> 
> > Source/WebCore/dom/ContainerNode.h:174
> > +inline Node* ContainerNode::firstElementChild() const
> 
> I'm surprised we don't already have this from ElementTraversal:
> http://dev.w3.org/2006/webapi/ElementTraversal/publish/ElementTraversal.html

Ahh, good catch, although I can't use that at the ContainerNode level. A follow-on patch could make Element::firstElementChild() call the ContainerNode version and downcast to an Element*.

> 
> Should this return Element*?  Or I guess it can't easily since ContainerNode.h doesn't knwo about Element.h.
> 
> It certainly could at least return ContainerNode*. :)

Good call. Fixed.

> 
> > Source/WebCore/html/parser/HTMLConstructionSite.cpp:221
> > +    // A fragment DOCTYPE shouldn't modify the compatibility mode of the owning
> 
> Probably don't need to wrap comment.

Sorry, my 80-character OCD shined through there. Fixed.

> 
> > Source/WebCore/html/parser/HTMLConstructionSite.h:47
> > +    HTMLConstructionSite(Document*, ContainerNode* attachmentRoot, FragmentScriptingPermission, bool isParsingFragment);
> 
> isParsingFragment is now implied.
> Might want to add a comment here to explain which is which.

I rearranged the constructor arguments so that one takes a Document* and the other takes a DocumentFragment* (and FragmentScriptingPermission). I think that should make them self-documenting as to which is which.

> 
> > Source/WebCore/html/parser/HTMLTreeBuilder.cpp:417
> > +    RefPtr<Node> firstElement = m_fragment->firstElementChild();
> 
> We might want to call this documentElement and add a comment here to explain that this matches the spec.

Done.

> 
> > Source/WebCore/html/parser/HTMLTreeBuilder.cpp:422
> > +    ASSERT(firstElement->isContainerNode());
> 
> If firstElementChild returns containerNode then we don't need this assert.

Yup.

Thanks for the review! I'll upload a new shark profile here shortly.

[Andy Estes]

Created attachment 82053 [details]
Patch

[Darin Adler]

Comment on attachment 82053 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=82053&action=review

This seems to address everything Eric raised. r=me

> Source/WebCore/dom/ContainerNode.h:180
> +inline ContainerNode* ContainerNode::firstElementChild() const
> +{
> +    Node* child = firstChild();
> +    while (child && !child->isElementNode())
> +        child = child->nextSibling();
> +    return static_cast<ContainerNode*>(child);
> +}

Could make this return Element* and then move the function body into Element.h. Not important for this check-in, but I think it would be better.

Generally speaking it’s not good for ContainerNode to have the Element concept at all, so maybe this should be a function that takes a ContainerNode* rather than a ContainerNode member function.

> Source/WebCore/html/parser/HTMLConstructionSite.h:138
> +    // This is the root ContainerNode to which the parser attaches all newly
> +    // constructed nodes. It points to a DocumentFragment when parsing fragments
> +    // and a Document in all other cases.
> +    ContainerNode* m_attachmentRoot;

What guarantees the DocumentFragment will live long enough? I’m surprised we don’t keep a reference to the document fragment. The only reason we don’t ref the document is that it would lead to a cycle.

[Eric Seidel (no email)]

Geez you guys are in a hurry.  I"ll look in a bit.

[Eric Seidel (no email)]

Comment on attachment 82053 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=82053&action=review

>> Source/WebCore/dom/ContainerNode.h:180
>> +}
> 
> Could make this return Element* and then move the function body into Element.h. Not important for this check-in, but I think it would be better.
> 
> Generally speaking it’s not good for ContainerNode to have the Element concept at all, so maybe this should be a function that takes a ContainerNode* rather than a ContainerNode member function.

Seems like a nice suggestion.  I guess it becomes firstElementChild(container) then? and we put that on Element.h?  the Element.h DOM method would have to call that one then.

> Source/WebCore/html/parser/HTMLConstructionSite.cpp:142
> +HTMLConstructionSite::HTMLConstructionSite(DocumentFragment* fragment, FragmentScriptingPermission scriptingPermission)

This is *much* nicer, thank you.

>> Source/WebCore/html/parser/HTMLConstructionSite.h:138
>> +    ContainerNode* m_attachmentRoot;
> 
> What guarantees the DocumentFragment will live long enough? I’m surprised we don’t keep a reference to the document fragment. The only reason we don’t ref the document is that it would lead to a cycle.

All fragment parsing is synchronous.  We don't really ASSERT that anywhere, but that is infact the case.

> LayoutTests/fast/parser/fragment-parser-doctype.html:13
> +    container.innerHTML = "<!DOCTYPE html><p><table>"

DOCTYPE nodes are only processed in InitialMode:

void HTMLTreeBuilder::processDoctypeToken(AtomicHTMLToken& token)
{
    ASSERT(token.type() == HTMLToken::DOCTYPE);
    if (m_insertionMode == InitialMode) {
        m_tree.insertDoctype(token);
        setInsertionMode(BeforeHTMLMode);
        return;
    }

Otherwise they're ignored.  So your test doesn't exactly test what you think it does. :)  I'm going to check and see if it's possible to get a fragment (with a context element) to start in InitialMode.  Obviously w/o a context element you always start there.

[Eric Seidel (no email)]

(In reply to comment #103)
> (From update of attachment 82053 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=82053&action=review

> Otherwise they're ignored.  So your test doesn't exactly test what you think it does. :)  I'm going to check and see if it's possible to get a fragment (with a context element) to start in InitialMode.  Obviously w/o a context element you always start there.

http://www.whatwg.org/specs/web-apps/current-work/multipage/the-end.html#parsing-html-fragments

No, it's not possible to ever get the parser in the InitialState when parsing a DocumentFragment with a context element.

Since we basically no longer "support" non-contextual fragments after your patch, we should ASSERT that we never get there.

We could ASSERT(!m_isParsingFragment) in HTMLConstructionSite::insertDoctype with a comment that we never hit this code when parsing fragments with a contextElemetn, and if we ever did, our behavior is subtly wrong (since context-less fragments can determine their own quirks mode which can affect parsing).

[Eric Seidel (no email)]

Comment on attachment 82053 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=82053&action=review

I"m not certain how useful your test case is since DOCTYPE nodes are ignored for fragments with contextElements.  However, testing that they're ignored is also fine, but you may want to change the text in your test to make that more clear.

Thanks for all the patches.  G'luck making your deadline.

> Source/WebCore/html/parser/HTMLConstructionSite.cpp:221
> +    // A fragment DOCTYPE shouldn't modify the compatibility mode of the owning document.

So I agree with Darin, this is a good patch and we should land it. :)

I would like to see you change this comment to something like:

// DOCTYPE nodes are only processed when parsing fragments w/o contextElements, which
// never occurs.  However, if we ever chose to support such, this code is subtly wrong,
// because context-less fragments can determine their own quirks mode, and thus change
// parsing rules (like <p> inside <table>).  For now we ASSERT that we never hit this code
// in a fragment, as changing the owning document's compatibility mode would be wrong.
ASSERT(!m_parsingFragment);

[Eric Seidel (no email)]

(In reply to comment #105)
> I would like to see you change this comment to something like:

to make that statement more clear:

1.  you should feel free to land your patch w/o posting again.

2.  you should add the ASSERT and keep the early return.  Not essential, but the extra two lines of early return are better than modifying the quirks mode of the owning document in the case of non-contextual fragment parsing.

I'll see about petitioning Ian to remove non-contextual fragments from HTML5 since I can't see any use for them currently.

[Andy Estes]

(In reply to comment #106)
> (In reply to comment #105)
> > I would like to see you change this comment to something like:
> 
> to make that statement more clear:
> 
> 1.  you should feel free to land your patch w/o posting again.
> 
> 2.  you should add the ASSERT and keep the early return.  Not essential, but the extra two lines of early return are better than modifying the quirks mode of the owning document in the case of non-contextual fragment parsing.
> 
> I'll see about petitioning Ian to remove non-contextual fragments from HTML5 since I can't see any use for them currently.

Thanks Eric and Darin! I'll land with these changes in place.

[Eric Seidel (no email)]

I've filed a bug with the w3c about removing context-less fragment parsing:
http://www.w3.org/Bugs/Public/show_bug.cgi?id=12031

[Andy Estes]

Committed r78286: <http://trac.webkit.org/changeset/78286>

[Andy Estes]

Created attachment 82190 [details]
Instruments profile while running tiny-innerHTML

Here is a profile of running tiny-innerHTML with r78286. I had a hard time getting Shark to cooperate so I took a profile using Instruments instead. Let me know if anyone has trouble opening it and I can attach a text file instead.

[Adam Barth]

Some things in this trace are wild:

8.4%	 	WebCore::HTMLTreeBuilder::HTMLTreeBuilder(WebCore::HTMLDocumentParser*, WebCore::DocumentFragment*, WebCore::Element*, WebCore::FragmentScriptingPermission, bool)

That's why too much time to be spending in the constructor.

18.4%	 	WebCore::HTMLTreeBuilder::FragmentParsingContext::finished()

Also way more than needed.  Specifically, below there:

13.4%	 	WebCore::ContainerNode::removeChildren()

That's a crazy amount of time.

Anyway, the profile points to a several places that should be easy to optimize.

[Andy Estes]

(In reply to comment #111)
> Some things in this trace are wild:
> 
> 8.4%         WebCore::HTMLTreeBuilder::HTMLTreeBuilder(WebCore::HTMLDocumentParser*, WebCore::DocumentFragment*, WebCore::Element*, WebCore::FragmentScriptingPermission, bool)

Below there, a significant amount of time is spent inserting a fake HTML tag.

> 
> That's why too much time to be spending in the constructor.
> 
> 18.4%         WebCore::HTMLTreeBuilder::FragmentParsingContext::finished()
> 
> Also way more than needed.  Specifically, below there:
> 
> 13.4%         WebCore::ContainerNode::removeChildren()

...and all this work is done in order to remove said fake HTML tag.

It sounds like a significant optimization would be to not insert the fake HTML tag to begin with in the fragment w/ context case.

[Andy Estes]

(In reply to comment #112)
> It sounds like a significant optimization would be to not insert the fake HTML tag to begin with in the fragment w/ context case.

Making this change is a little more involved than I had anticipated. HTMLConstructionSite, with only a handful of exceptions, assumes there is at least one open element on the HTMLElementStack, and not adding a fake HTML element for fragments breaks this assumption. This could be fixed by pushing the DocumentFragment on the HTMLElementStack, but then it is no longer an Element stack but rather a ContainerNode stack. Another option would be to update a bunch of cases in HTMLConstructionSite to check if we are parsing a fragment and have an empty Element stack, and if so attach the new element to the DocumentFragment rather than the top of the Element stack.

Maybe I'm missing a more elegant approach?

[Adam Barth]

I don't think you want to teach everyone else about an empty element stack.  That's pretty wired into how the algorithm works and will introduce a bunch of branches that will hurt real-world performance.

[Eric Seidel (no email)]

You could share the HTMLHTMLElement between fragment parse passes.  LIke how we tried to share the Document.  Sharing <html> is much less scary however.

[Adam Barth]

Comment on attachment 82053 [details]
Patch

Clearing the review flag so this doesn't show up in pending-commit.

[Andy Estes]

(In reply to comment #115)
> You could share the HTMLHTMLElement between fragment parse passes.  LIke how we tried to share the Document.  Sharing <html> is much less scary however.

True, although we'd still have to pay the cost of reparenting at the end of parsing, which is pretty significant in the profile.

I just tested a patch that inserts the DocumentFragment in place of the HTML element. Here is what I get on tiny-innerHTML:

avg 1725.6
median 1725
stdev 7.052659073002182
min 1715
max 1739

That brings down to regression 1.3%, which is almost insignificant given the standard deviation. Let me clean the patch up a little bit before posting it for review.

[Andy Estes]

Created attachment 83022 [details]
Patch

[Andy Estes]

(In reply to comment #118)
> Created an attachment (id=83022) [details]
> Patch

With this patch applied I'm now measuring us even with 5.0.3 on tiny-innerHTML and showing a progression on Peacekeeper if you ignore the 'domDynamicCreationCreateElement', which probably has nothing to do with fragment parsing. Every other DOM-related Peacekeeper test is now faster than 5.0.3 with this patch.

Once this patch lands, I'd say we can close this bug and open up a new one specifically for domDynamicCreationCreateElement.

[Andy Estes]

I'll also note that this patch further breaks our support for the mythical context-less fragment, which should have an HTML element as its root node but now never will. This might or might not be a problem depending on how http://www.w3.org/Bugs/Public/show_bug.cgi?id=12031 shakes out.

[Eric Seidel (no email)]

Comment on attachment 83022 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=83022&action=review

:(  I'm gonna have to chew on this for a bit.  Its sad to lose the type specificity of Element.  I'm not sure how much it buys us, but still sad. :(

> Source/WebCore/dom/Element.h:6
> + * Copyright (C) 2003, 2004, 2005, 2006, 2007, 2008, 2009, 2010, 2011 Apple Inc. All rights reserved.

Seems a bit silly when we start listing 8 years. :)

[Andy Estes]

(In reply to comment #121)
> (From update of attachment 83022 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=83022&action=review
> 
> :(  I'm gonna have to chew on this for a bit.  Its sad to lose the type specificity of Element.  I'm not sure how much it buys us, but still sad. :(

I'm open to alternatives. Perhaps we could make HTMLElementStack a template so that it can have Elements in the regular case and ContainerNodes in the fragment case. Surprisingly, there was little that relied on Element's interface (setAttribute is the only thing that comes to mind), so the loss of specificity might not be so terrible in practice.

> 
> > Source/WebCore/dom/Element.h:6
> > + * Copyright (C) 2003, 2004, 2005, 2006, 2007, 2008, 2009, 2010, 2011 Apple Inc. All rights reserved.
> 
> Seems a bit silly when we start listing 8 years. :)

Heh, yea. Should this be 2003-2011? I'm not sure what our convention is here.

[Adam Barth]

Comment on attachment 83022 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=83022&action=review

Very cool!  I would have marked it R+, but Eric says he'd like to chew on it some more.

> Source/WebCore/html/parser/HTMLElementStack.h:50
> +    HTMLElementStack(PassRefPtr<DocumentFragment>);

Please add the explicit keyword.

> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:-378
> -        processFakeStartTag(htmlTag);

Please add a comment about why we skip the instruction in the spec to processFakeStartTag.

> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:2795
> +    if (!isParsingFragment()) {

Prefer early return.

[Darin Adler]

(In reply to comment #122)
> > > + * Copyright (C) 2003, 2004, 2005, 2006, 2007, 2008, 2009, 2010, 2011 Apple Inc. All rights reserved.
> > 
> > Seems a bit silly when we start listing 8 years. :)
> 
> Heh, yea. Should this be 2003-2011? I'm not sure what our convention is here.

The comma separated list is correct, the range is not, for Apple copyrights at least. Others may chose to do differently with their copyright notices.

[Eric Seidel (no email)]

Comment on attachment 83022 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=83022&action=review

> Source/WebCore/ChangeLog:14
> +        Instead of pushing a fake HTMLHtmlElement onto the open element stack
> +        during fragment parsing only to later remove it and reparent its
> +        children to the DocumentFragment, push the DocumentFragment directly
> +        onto the open element stack as the root node. This requires refactoring
> +        HTMLElementStack to hold ContainerNode pointers rather than Element
> +        pointers, which has implications for HTMLConstructionSite and
> +        HTMLTreeBuilder as well.

Why was pushing the fake element and removing it so expensive?  Could we just make it cheaper instead of teaching everything how to handle nodes just for this one special case?  It seems the old code was cleaner (and certainly closer to the spec).  Not that this new solution isn't doable (since it would be nice to improve perf here).

> Source/WebCore/html/parser/HTMLConstructionSite.cpp:209
> +    if (!m_isParsingFragment)

This needs a "why" comment.

> Source/WebCore/html/parser/HTMLConstructionSite.cpp:242
> +    attach(currentNode(), Comment::create(currentNode()->document(), token.comment()));

We could just leave it as currentElement() since DocumentFragment is the only exception (and document the exception in the header).

> Source/WebCore/html/parser/HTMLElementStack.cpp:72
> +        || node->nodeType() == Node::DOCUMENT_FRAGMENT_NODE;

Needs a comment to document this spec exception.

> Source/WebCore/html/parser/HTMLElementStack.cpp:86
> +        || node->nodeType() == Node::DOCUMENT_FRAGMENT_NODE;

Again.

> Source/WebCore/html/parser/HTMLElementStack.cpp:95
> +        || node->nodeType() == Node::DOCUMENT_FRAGMENT_NODE;

Again.

> Source/WebCore/html/parser/HTMLElementStack.cpp:102
> +        || node->nodeType() == Node::DOCUMENT_FRAGMENT_NODE;

Again.  Another way to do this would be to have a function isRootMarker(node) or similar and call it everywhere the code used to call node->hasTagName(htmlTag).  Then you'd need to only put the explanation in one place and only explian there where all that new function should be used.  I think that's probably cleaner than adding the || nodeType == to every callsite manually.

> Source/WebCore/html/parser/HTMLElementStack.cpp:164
> +    , m_isParsingFragment(false)

I don't think we need this bool.  A function called isParsingFragment() which checks the type of the m_rootNode is sufficient (and slightly less error prone).

> Source/WebCore/html/parser/HTMLElementStack.cpp:175
> +    m_top = adoptPtr(new ElementRecord(fragment, m_top.release()));

Why does this need to be part of the constructor?  Seems this could be done by the caller, and then we wouldn't need a separate constructor.

> Source/WebCore/html/parser/HTMLElementStack.cpp:219
> +        topNode()->finishParsingChildren();

I woudl have just left this as top().

> Source/WebCore/html/parser/HTMLElementStack.cpp:296
> +    ASSERT(!m_isParsingFragment);

This isn't needed.

> Source/WebCore/html/parser/HTMLElementStack.cpp:530
> +    ASSERT(!m_isParsingFragment);

This is redundant with the previous assert, althought if you used a isParsingFragment() function instead maybe it wouldn't be obvious so you'd leave this in.

> Source/WebCore/html/parser/HTMLElementStack.h:64
> +        ContainerNode* node() const { return m_node.get(); }

Oh, I see, you added a new node() function as well as keeping element().  It's not clear to me where which is useable vs. the other.

> Source/WebCore/html/parser/HTMLElementStack.h:91
> +    ContainerNode* topNode() const

I see, and you added a separate topNode() function....

[Eric Seidel (no email)]

I'm staring at the profile now and seeing what other improvements can be made.

[Andy Estes]

(In reply to comment #125)
> (From update of attachment 83022 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=83022&action=review
> 
> > Source/WebCore/ChangeLog:14
> > +        Instead of pushing a fake HTMLHtmlElement onto the open element stack
> > +        during fragment parsing only to later remove it and reparent its
> > +        children to the DocumentFragment, push the DocumentFragment directly
> > +        onto the open element stack as the root node. This requires refactoring
> > +        HTMLElementStack to hold ContainerNode pointers rather than Element
> > +        pointers, which has implications for HTMLConstructionSite and
> > +        HTMLTreeBuilder as well.
> 
> Why was pushing the fake element and removing it so expensive?  Could we just make it cheaper instead of teaching everything how to handle nodes just for this one special case?  It seems the old code was cleaner (and certainly closer to the spec).  Not that this new solution isn't doable (since it would be nice to improve perf here).

I honestly didn't spend much time thinking about optimizing the fake element creation since I reasoned it was unnecessary to begin with. I agree that keeping the fake HTML element around makes the element stack conceptually simpler, but I think the perf win is worth it. Your suggestions below will make this approach a little tidier than it currently is.

> 
> > Source/WebCore/html/parser/HTMLConstructionSite.cpp:209
> > +    if (!m_isParsingFragment)
> 
> This needs a "why" comment.

Ok.

> 
> > Source/WebCore/html/parser/HTMLConstructionSite.cpp:242
> > +    attach(currentNode(), Comment::create(currentNode()->document(), token.comment()));
> 
> We could just leave it as currentElement() since DocumentFragment is the only exception (and document the exception in the header).

What if I'm inserting a comment directly on the Fragment (e.g. innerHTML = "<!-- comment -->...")?

> 
> > Source/WebCore/html/parser/HTMLElementStack.cpp:72
> > +        || node->nodeType() == Node::DOCUMENT_FRAGMENT_NODE;
> 
> Needs a comment to document this spec exception.
> 
> > Source/WebCore/html/parser/HTMLElementStack.cpp:86
> > +        || node->nodeType() == Node::DOCUMENT_FRAGMENT_NODE;
> 
> Again.
> 
> > Source/WebCore/html/parser/HTMLElementStack.cpp:95
> > +        || node->nodeType() == Node::DOCUMENT_FRAGMENT_NODE;
> 
> Again.
> 
> > Source/WebCore/html/parser/HTMLElementStack.cpp:102
> > +        || node->nodeType() == Node::DOCUMENT_FRAGMENT_NODE;
> 
> Again.  Another way to do this would be to have a function isRootMarker(node) or similar and call it everywhere the code used to call node->hasTagName(htmlTag).  Then you'd need to only put the explanation in one place and only explian there where all that new function should be used.  I think that's probably cleaner than adding the || nodeType == to every callsite manually.

I like that idea; a lot cleaner.

> 
> > Source/WebCore/html/parser/HTMLElementStack.cpp:164
> > +    , m_isParsingFragment(false)
> 
> I don't think we need this bool.  A function called isParsingFragment() which checks the type of the m_rootNode is sufficient (and slightly less error prone).

I like this idea too.

> 
> > Source/WebCore/html/parser/HTMLElementStack.cpp:175
> > +    m_top = adoptPtr(new ElementRecord(fragment, m_top.release()));
> 
> Why does this need to be part of the constructor?  Seems this could be done by the caller, and then we wouldn't need a separate constructor.

Good point.

> 
> > Source/WebCore/html/parser/HTMLElementStack.cpp:219
> > +        topNode()->finishParsingChildren();
> 
> I woudl have just left this as top().

If I did that, top() would ASSERT if it were the DocumentFragment.

> 
> > Source/WebCore/html/parser/HTMLElementStack.cpp:296
> > +    ASSERT(!m_isParsingFragment);
> 
> This isn't needed.

I wanted to assert() that clients never attempt to push an HTML element during fragment parsing, which seems different than asserting that there is no root node.

> 
> > Source/WebCore/html/parser/HTMLElementStack.cpp:530
> > +    ASSERT(!m_isParsingFragment);
> 
> This is redundant with the previous assert, althought if you used a isParsingFragment() function instead maybe it wouldn't be obvious so you'd leave this in.

You're right, it's redundant.

> 
> > Source/WebCore/html/parser/HTMLElementStack.h:64
> > +        ContainerNode* node() const { return m_node.get(); }
> 
> Oh, I see, you added a new node() function as well as keeping element().  It's not clear to me where which is useable vs. the other.

There are times when element() is appropriate, such as when we know we have at least one open node on the stack. It made sense to keep the more specific function for these cases, although perhaps it makes it more confusing to know when to call which function. 

> 
> > Source/WebCore/html/parser/HTMLElementStack.h:91
> > +    ContainerNode* topNode() const
> 
> I see, and you added a separate topNode() function....

[Eric Seidel (no email)]

(In reply to comment #127)
> (In reply to comment #125)
> > (From update of attachment 83022 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=83022&action=review
> > 
> > > Source/WebCore/ChangeLog:14
> > > +        Instead of pushing a fake HTMLHtmlElement onto the open element stack
> > > +        during fragment parsing only to later remove it and reparent its
> > > +        children to the DocumentFragment, push the DocumentFragment directly
> > > +        onto the open element stack as the root node. This requires refactoring
> > > +        HTMLElementStack to hold ContainerNode pointers rather than Element
> > > +        pointers, which has implications for HTMLConstructionSite and
> > > +        HTMLTreeBuilder as well.
> > 
> > Why was pushing the fake element and removing it so expensive?  Could we just make it cheaper instead of teaching everything how to handle nodes just for this one special case?  It seems the old code was cleaner (and certainly closer to the spec).  Not that this new solution isn't doable (since it would be nice to improve perf here).

I still question the benchmark, but I agree, the perf win is worth it.  I'm right now looking at other hotspots on the profile to see if this is really needed in the end.  It seems to me that the single malloc needed for the fake root container element (<html> or not) should not really cause that much of a slowdown and a little analysis may yield a simpler solution.  Bug 55005 is the beginnings of that analysis (although I'm going to disable HTMLSouceTracking for fragments entirely since it's not needed).

[Eric Seidel (no email)]

Bug 55011 may make any further changes here moot.

Andy, you might try running peacekeeper with the patch from bug 55011 applied and see if we're faster now. :)

[Eric Seidel (no email)]

Again, let me re-state how broken/meaningless this peacekeeper test is (more explanation at https://bugs.webkit.org/show_bug.cgi?id=55011#c4).

I think the overlooked part of the sample was that although what Andy fixed is one of the hotest functions (Document::nodeChildrenChanged and Document::nodeChildrenWillBeRemoved), doing so overlooks the malloc/free costs which are also very high in the sample.

malloc/free is deceptive, because if we ever see malloc/free on a sample, every malloc should be counted in at least triplicate.  There is the cost to malloc, the cost to free, the bookkeeping costs within the malloc pool, possible paging (due to pressure) and possible heap fragmentation.  In this case, avoiding a single malloc (bug 55011 a workaround for bug 55005) sped up tiny-innerHTML by 2x (most of that related to the brokenness of the benchmark more than the worthyness of the fix).

I'm going to look again at fixing bug 55005 and I expect another large speedup.  I expect after bug 55011 lands we will already be much faster than Safari 5 on this Peacekeeper test, and if we fix 55005 we should be faster still.

Andy:  If you could run Peacekeeper again once bug 55011 lands, then we can make a more educated assessment of if we may yet need your patch.

[Andy Estes]

(In reply to comment #130)
> Andy:  If you could run Peacekeeper again once bug 55011 lands, then we can make a more educated assessment of if we may yet need your patch.

I had applied your patch a few minutes before it was r+'d and I'm compiling a release build as we speak. I'll let you know what I find.

[Eric Seidel (no email)]

Woh.  I've been misunderstanding the whole time.  Attachment 72443 [details] does not look like javascript which woudl actually do anything, so I think simon may have attached the wrong script?

I've been trying to optimized innerHTML performance.  But the test in question domDynamicCreationCreateElement doesn't sound like it has anything to do with innerHTML?

I think I understand now what the JS in question is trying to do.  I'll write a better microbenchmark and go at this again tomorrow.  Thank you for your patience.

[Eric Seidel (no email)]

(In reply to comment #132)
> Woh.  I've been misunderstanding the whole time.  Attachment 72443 [details] does not look like javascript which woudl actually do anything, so I think simon may have attached the wrong script?
> 
> I've been trying to optimized innerHTML performance.  But the test in question domDynamicCreationCreateElement doesn't sound like it has anything to do with innerHTML?
> 
> I think I understand now what the JS in question is trying to do.  I'll write a better microbenchmark and go at this again tomorrow.  Thank you for your patience.

Sorry.  That message got garbled.  I (sorta) figured out what the JS did half-way through writing it and failed in editing the message before posting. :)

I'm still not sure if peacekeeper calls init() before run() in a loop?  and if so, how many times.

But I'm working on a better microbenchmark and improving perf.

[Eric Seidel (no email)]

This explains why when I improved tiny-innerHTML by 2x in bug 55011 it only affected this peacekeeper benchmark a little.  When we started this quest, Document creation during innerHTML was completely dominating the sample.  Now that Andy has fixed fragment parsing to not create a Document innerHTML no longer dominates the sample, and so tiny-innerHTML is no longer a good proxy for Peacekeeper's domDynamicCreationCreateElement.

[Simon Fraser (smfr)]

James Robinson has the Peacekeeper sources.

[Andy Estes]

(In reply to comment #132)
> Woh.  I've been misunderstanding the whole time.  Attachment 72443 [details] does not look like javascript which woudl actually do anything, so I think simon may have attached the wrong script?
> 
> I've been trying to optimized innerHTML performance.  But the test in question domDynamicCreationCreateElement doesn't sound like it has anything to do with innerHTML?
> 

domDynamicCreationCreateElement wasn't the only test that saw a regression. domDynamicCreationInnerHTML also slowed down, which is obviously all about fragment parsing. In fact, I mentioned after uploading https://bugs.webkit.org/attachment.cgi?id=83022 that we could even open a new bug about domDynamicCreationCreateElement since every other Peacekeeper test was now faster than in 5.0.3.

[Eric Seidel (no email)]

I'm finishing bug 55164 while I wait for the commit-queue to catch up and land all the perf patches I wrote the last few days.  I'll upload my next round this afternoon once I can test them all together and see how far we've come. :)

[Maciej Stachowiak]

So what's up with Andy's latest patch on this bug? It's flagged for review, but there are many review comments. Should it be marked r- until there is a revised version? Is the review incomplete?

[Eric Seidel (no email)]

Comment on attachment 83022 [details]
Patch

I don't like the solution, so I've been working on better ones.  As seen in bug 55011, bug 55091, bug 55005, bug 55204 and bug 55127.

If I can't make up the difference in other ways then I'll look more seriously at Andy's proposal again.

Andy'r proposal is narrow:
- It removes a single malloc (creation of the <html> element) and makes us not have to move nodes (which is needlessly expensive due to Node::willRemove() and some other related junk).
I also think it reduces hackability of the parser some.

I'm not over-my-dead-body opposed to his change as-written, but I've spent yesterday and today and plan to spend tomorrow working on better, more broadly applicable solutions.

[Maciej Stachowiak]

(In reply to comment #139)
> (From update of attachment 83022 [details])
> I don't like the solution, so I've been working on better ones.  As seen in bug 55011, bug 55091, bug 55005, bug 55204 and bug 55127.
> 
> If I can't make up the difference in other ways then I'll look more seriously at Andy's proposal again.
> 
> Andy'r proposal is narrow:
> - It removes a single malloc (creation of the <html> element) and makes us not have to move nodes (which is needlessly expensive due to Node::willRemove() and some other related junk).
> I also think it reduces hackability of the parser some.
> 
> I'm not over-my-dead-body opposed to his change as-written, but I've spent yesterday and today and plan to spend tomorrow working on better, more broadly applicable solutions.

One thing to keep in mind is that there are real sites that do innerHTML in a tight loop or on a short timer, so shaving even a relatively small amount off of that can be a big win on selected sites.

In general, we take performance to be a higher priority than keeping code close to a literal-minded translation of spec prose, and I don't really see a hackability issue with the change other than that. In many parts of WebKit, we started with an algorithm that works quite literally the way the spec does, and later improved things to be faster or less memory-intensive.

In any case, I think it would be good for Andy to address the comments and post a new version.

[Eric Seidel (no email)]

Comment on attachment 83022 [details]
Patch

I still have two perf patches waiting to land (if the SL build ever goes green again?).  But otherwise I think I'm out of easy perf wins.

- Getting rid of the ElementRecord malloc for <html> (and possible <head> and <body>) would be relatively easy.
- Removing willRemove has proved a challange.
- It might be possible to optimize NamedNodeMap to include a little inline capacity (makes little sense to ever have a empty NamedNodeMap).
- Removing the malloc from isCSSPropertyName could be straightforward.
- It might be a good idea to share and re-use a HTMLDocumentParser for fragments, that's unclear (but might be really easy).
- Optimizing innerHTML="" to avoid ever creating a DocumentFragment (or parser for that matter) could be a win, unclear if peacekeeper hits that path.
- Calling detachFromElement() on m_attributeMap from ~Element may be wasteful if Element is holding the only reference (but it's possible the individual attributes could be held by JS?)
-  We could also consider optimizing HTMLElement::setInnerHTML to avoid calling the parser if the string is less than N characters and doesn't have a '<' or '&' , such a heuristic could be a huge win, but may not be worth the complexity.
- Shark attributes a fastFree under createFloatingValueList (which makes no sense?) but might be a stray malloc we're missing?
- We spend a lot of time checking WTF::currentTime().  We may be creating PumpSessions too often, or checking for yields when we don't need to (yes! I'll fix this one)
- We spend more time than you'd think in ~Node, possibly more than we should trying to read AXObjectCache::accessibilityEnabled()

But the <html> malloc and the node removal/moving stands out.  This patch fixes those and is thus growing on me.  We'll talk more after I sleep. :)

[Eric Seidel (no email)]

(In reply to comment #141)
> (From update of attachment 83022 [details])
> - We spend a lot of time checking WTF::currentTime().  We may be creating PumpSessions too often, or checking for yields when we don't need to (yes! I'll fix this one)

Bug 55211.  This ended up being a big win!

[Andy Estes]

Created attachment 83914 [details]
Patch

This addresses review feedback from Adam and Eric. I'll be interested to hear where we stand after Eric's numerous speedup patches.

[Eric Seidel (no email)]

Before:

avg 348.7
median 349
stdev 1.268857754044952
min 346
max 351

After:

avg 320.8
median 320.5
stdev 2.0396078054371136
min 318
max 329

Looks like it's still a big win.  I'll give it another review on Monday.

[Eric Seidel (no email)]

For comparison, Safari 5.0.3:
avg 292.9
median 295
stdev 6.554133555754464
min 275
max 303

Looks like we still have a bit to shave.

Also, all of those numbers are from my local peacekeeper-approximation, not the real benchmark.

[Eric Seidel (no email)]

I think the old code avoided spinning up the parser in many simple innerHTML cases.  I will have to look through the svn history a bit.

[Eric Seidel (no email)]

Comment on attachment 83914 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=83914&action=review

I think Node* instead of Element* still creeped a bit too wide here.  I'm convinced we want this change, but I want to be careful to not spread Node/ContainerNode* any further than we absolutely have to.

> Source/WebCore/ChangeLog:19
> +        With this patch, the regression in Peacekeeper from Safari 5.0.3 to ToT
> +        is ~14%. However, if you discount the 'domDynamicCreationCreateElement'
> +        test, ToT is now ~4% faster than Safari 5.0.3. This indicates that the
> +        regression no longer lies in fragment parsing.

I'm not sure I understand this conclusion.  It seems that domDynamicCreationCreateElement is still slower than 5.0.3 after this patch, no?

> Source/WebCore/html/parser/HTMLConstructionSite.cpp:212
> +    // Fragments do not have a root HTML element, so any additional HTML elements
> +    // encountered during fragment parsing should be ignored.
> +    if (m_isParsingFragment)
> +        return;

What's this needed for?  Seems we should test it.  Are we getting in here because of some other if we're failing now but shouldn't be?

> Source/WebCore/html/parser/HTMLElementStack.cpp:392
> +HTMLElementStack::ElementRecord* HTMLElementStack::find(ContainerNode* node) const

We never search for a Node (to my knowledge).

> Source/WebCore/html/parser/HTMLElementStack.cpp:410
> +bool HTMLElementStack::contains(ContainerNode* node) const

I don't see how we shoudl ever hit this with a non-element.

> Source/WebCore/html/parser/HTMLElementStack.h:60
> +            ASSERT(m_node && m_node->isElementNode());
> +            return toElement(m_node.get());

Doesn't toElement already make those same ASSERTs?

> Source/WebCore/html/parser/HTMLElementStack.h:112
> +    void popUntil(ContainerNode*);

I'd be surprised if it were ever possible to pass a DocumentFragment here.

> Source/WebCore/html/parser/HTMLElementStack.h:114
> +    void popUntilPopped(ContainerNode*);

Here too.

[Eric Seidel (no email)]

I'll see if I can find the remaining 10% tomorrow.  I suspect it's all due to old innerHTML "cheats".

[Maciej Stachowiak]

(In reply to comment #147)
> (From update of attachment 83914 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=83914&action=review
> 
> I think Node* instead of Element* still creeped a bit too wide here.  I'm convinced we want this change, but I want to be careful to not spread Node/ContainerNode* any further than we absolutely have to.
> 
> > Source/WebCore/ChangeLog:19
> > +        With this patch, the regression in Peacekeeper from Safari 5.0.3 to ToT
> > +        is ~14%. However, if you discount the 'domDynamicCreationCreateElement'
> > +        test, ToT is now ~4% faster than Safari 5.0.3. This indicates that the
> > +        regression no longer lies in fragment parsing.
> 
> I'm not sure I understand this conclusion.  It seems that domDynamicCreationCreateElement is still slower than 5.0.3 after this patch, no?

Andy believes that the remaining domDynamicCreationCreateElement regression is due to other changes, and not the HTML5 parser. (I haven't looked at a profile to confirm myself.)

[Andy Estes]

(In reply to comment #147)
> (From update of attachment 83914 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=83914&action=review
> 
> I think Node* instead of Element* still creeped a bit too wide here.  I'm convinced we want this change, but I want to be careful to not spread Node/ContainerNode* any further than we absolutely have to.
> 
> > Source/WebCore/ChangeLog:19
> > +        With this patch, the regression in Peacekeeper from Safari 5.0.3 to ToT
> > +        is ~14%. However, if you discount the 'domDynamicCreationCreateElement'
> > +        test, ToT is now ~4% faster than Safari 5.0.3. This indicates that the
> > +        regression no longer lies in fragment parsing.
> 
> I'm not sure I understand this conclusion.  It seems that domDynamicCreationCreateElement is still slower than 5.0.3 after this patch, no?

What Maciej said :)

> 
> > Source/WebCore/html/parser/HTMLConstructionSite.cpp:212
> > +    // Fragments do not have a root HTML element, so any additional HTML elements
> > +    // encountered during fragment parsing should be ignored.
> > +    if (m_isParsingFragment)
> > +        return;
> 
> What's this needed for?  Seems we should test it.  Are we getting in here because of some other if we're failing now but shouldn't be?

No, we always get here when we encounter an HTML element in a fragment. This would be a duplicate of the fake HTML element already inserted, so we'd handle it by migrating the duplicate element's attributes to the original. Since we no longer create a fake HTML element (and since it would be discarded even if we did), we can just return early here.

html5lib has coverage for this case.

> 
> > Source/WebCore/html/parser/HTMLElementStack.cpp:392
> > +HTMLElementStack::ElementRecord* HTMLElementStack::find(ContainerNode* node) const
> 
> We never search for a Node (to my knowledge).

You're right. I kept the interface in terms of Element* and changed the implementation to compare nodes instead.

> 
> > Source/WebCore/html/parser/HTMLElementStack.cpp:410
> > +bool HTMLElementStack::contains(ContainerNode* node) const
> 
> I don't see how we shoudl ever hit this with a non-element.

Same as above.

> 
> > Source/WebCore/html/parser/HTMLElementStack.h:60
> > +            ASSERT(m_node && m_node->isElementNode());
> > +            return toElement(m_node.get());
> 
> Doesn't toElement already make those same ASSERTs?

Good point.

> 
> > Source/WebCore/html/parser/HTMLElementStack.h:112
> > +    void popUntil(ContainerNode*);
> 
> I'd be surprised if it were ever possible to pass a DocumentFragment here.

I thought it was, but I guess it isn't. It looks like we know we have an Element in every popUntil() call site. I changed the interface back to Element*.

> 
> > Source/WebCore/html/parser/HTMLElementStack.h:114
> > +    void popUntilPopped(ContainerNode*);
> 
> Here too.

Same as above.

[Andy Estes]

Created attachment 84167 [details]
Patch

[Andy Estes]

(In reply to comment #149)
> (In reply to comment #147)
> > (From update of attachment 83914 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=83914&action=review
> > 
> > I think Node* instead of Element* still creeped a bit too wide here.  I'm convinced we want this change, but I want to be careful to not spread Node/ContainerNode* any further than we absolutely have to.
> > 
> > > Source/WebCore/ChangeLog:19
> > > +        With this patch, the regression in Peacekeeper from Safari 5.0.3 to ToT
> > > +        is ~14%. However, if you discount the 'domDynamicCreationCreateElement'
> > > +        test, ToT is now ~4% faster than Safari 5.0.3. This indicates that the
> > > +        regression no longer lies in fragment parsing.
> > 
> > I'm not sure I understand this conclusion.  It seems that domDynamicCreationCreateElement is still slower than 5.0.3 after this patch, no?
> 
> Andy believes that the remaining domDynamicCreationCreateElement regression is due to other changes, and not the HTML5 parser. (I haven't looked at a profile to confirm myself.)

And it turns out I was wrong about this, sorry :( r65868 is responsible for a large regression in domDynamicCreationCreateElement. I will edit my ChangeLog comment before landing.

[Maciej Stachowiak]

Eric, based on the latest comments and Andy's responses, should his latest patch be marked r-, or r+ leaving it up to him to clean up remaining issues?

[Eric Seidel (no email)]

(In reply to comment #152)
> > Andy believes that the remaining domDynamicCreationCreateElement regression is due to other changes, and not the HTML5 parser. (I haven't looked at a profile to confirm myself.)
> 
> And it turns out I was wrong about this, sorry :( r65868 is responsible for a large regression in domDynamicCreationCreateElement. I will edit my ChangeLog comment before landing.

I'm still confused then.  We believe the remaining regression in Peacekeeper from 5.0.3 (after this patch) is still all domDynamicCreationCreateElement, likely fragment parsing (which is only one line of domDynamicCreationCreateElement anyway).

[Eric Seidel (no email)]

(In reply to comment #154)
> I'm still confused then.  We believe the remaining regression in Peacekeeper from 5.0.3 (after this patch) is still all domDynamicCreationCreateElement, likely fragment parsing (which is only one line of domDynamicCreationCreateElement anyway).

I meant for that to end with a ?.  Is that statement correct?  We believe the remaining regression is fragment parsing on domDynamicCreationCreateElement?

[Eric Seidel (no email)]

Comment on attachment 84167 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=84167&action=review

I'm not sure further review is very productive, as I expect you're pretty sick of this bug by now. :)  However I'd also like try try and follow what Darin always encouraged me to do, which was not approve a bug just because I'm sick of it. :)

This patch is good.  I'm ready to r+ it.  I still think we have too many currentNode/node calls instead of currentElement/element calls.

Any time we can use a more specific type, it seems worth doing.  Not having written this patch, I don't know why all of these were changed, but I suspect they're not all needed.  The spec is very carful about times when you could fall out of <body>/<html> and when you can't.  Since you're only replacing the sythesized <html> any time where the old code does nto check for hasTagName(htmlTag) or NULL, we don't need to use Node* or check for DocumentFragment.  The parser is very well tested.  Fragment case isn't covered quite as well, but still quite well and I would basically only use node() instead of element when we see the assert in toElement() firing.

I'm OK r+ing this, but I'd rather see one last version.

I'll be around all day tommorow (and the rest of the week) and happy to approve the patch as soon as you post it.

I'll be working on looking at the remaining peacekeeper regressions tomorrow as well.

> Source/WebCore/html/parser/HTMLElementStack.cpp:53
> +inline bool isRootMarker(ContainerNode* node)

I would have probably just called this isRootNode.  But it's OK as is. :)  IIRC, "markers" are a spec concept, of which is this not one. :)

> Source/WebCore/html/parser/HTMLElementStack.cpp:238
> +    while (!isNumberedHeaderElement(topNode()))

This doesn't need to accept a Node*, since clearly the previous implementation didn't handle NULL or <html>.

> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:2269
> +            if (m_tree.currentNode()->hasTagName(optionTag)) {

Can't this be currentElement?

> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:2309
> +                if (nodeRecord->node()->nodeType() == Node::DOCUMENT_FRAGMENT_NODE) {

You shoudl comment on why this is needed.

> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:2638
> +        if (m_tree.currentNode()->hasTagName(scriptTag))

Can the current node ever not be an element here?

[Eric Seidel (no email)]

Comment on attachment 84167 [details]
Patch

Despite Darin's long-ago, good advice. This is ridiculous.  I'm not gonna make you post a 15th patch, or whatever we're up to.

I would however like you to go through and see if you can remove more of the element to node replacements you did before landing (and correct the ChangeLog as you noted you would).

I'm happy to look at another patch if you would like, but only if you'd like.

Thanks again for all your hard work on this.  I'm sorry it took so long, but hopefully you feel good came out of it.  I certainly do. :)

[Andy Estes]

(In reply to comment #155)
> (In reply to comment #154)
> > I'm still confused then.  We believe the remaining regression in Peacekeeper from 5.0.3 (after this patch) is still all domDynamicCreationCreateElement, likely fragment parsing (which is only one line of domDynamicCreationCreateElement anyway).
> 
> I meant for that to end with a ?.  Is that statement correct?  We believe the remaining regression is fragment parsing on domDynamicCreationCreateElement?

Well, if I look at production builds of r65867 and r65868:

r65867:
domDynamicCreationCreateElement 31347.96238244514 ops

r65868:
domDynamicCreationCreateElement 19607.843137254902 ops

So, without looking at a sample, turning on fragment parsing did regress domDynamicCreationCreateElement significantly. For the other tests we're interested in, I see numbers in line with r65867 in ToT + my patch.

[Andy Estes]

(In reply to comment #158)
> (In reply to comment #155)
> > (In reply to comment #154)
> > > I'm still confused then.  We believe the remaining regression in Peacekeeper from 5.0.3 (after this patch) is still all domDynamicCreationCreateElement, likely fragment parsing (which is only one line of domDynamicCreationCreateElement anyway).
> > 
> > I meant for that to end with a ?.  Is that statement correct?  We believe the remaining regression is fragment parsing on domDynamicCreationCreateElement?
> 
> Well, if I look at production builds of r65867 and r65868:
> 
> r65867:
> domDynamicCreationCreateElement 31347.96238244514 ops
> 
> r65868:
> domDynamicCreationCreateElement 19607.843137254902 ops
> 
> So, without looking at a sample, turning on fragment parsing did regress domDynamicCreationCreateElement significantly. For the other tests we're interested in, I see numbers in line with r65867 in ToT + my patch.

I'll admit that this is confusing to me as well as the test seemingly has nothing to do with fragment parsing. I'm sure a sample will clarify this.

[Eric Seidel (no email)]

I've filed bug 55626 about remaining performance troubles with domDynamicCreationCreateElement.  It turns out that all of my testing here was ignoring renderer manipulation, which turns out to be 50% of time spent.  Should be easy to get another HUGE gain on domDynamicCreationCreateElement with a little work in bug 55626.

[Andy Estes]

(In reply to comment #156)
> (From update of attachment 84167 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=84167&action=review
> 
> I'm not sure further review is very productive, as I expect you're pretty sick of this bug by now. :)  However I'd also like try try and follow what Darin always encouraged me to do, which was not approve a bug just because I'm sick of it. :)
> 
> This patch is good.  I'm ready to r+ it.  I still think we have too many currentNode/node calls instead of currentElement/element calls.
> 
> Any time we can use a more specific type, it seems worth doing.  Not having written this patch, I don't know why all of these were changed, but I suspect they're not all needed.  The spec is very carful about times when you could fall out of <body>/<html> and when you can't.  Since you're only replacing the sythesized <html> any time where the old code does nto check for hasTagName(htmlTag) or NULL, we don't need to use Node* or check for DocumentFragment.  The parser is very well tested.  Fragment case isn't covered quite as well, but still quite well and I would basically only use node() instead of element when we see the assert in toElement() firing.

I'll take one last pass before landing, but I will say that I was initially very conservative with converting Elements to ContainerNodes. Most places where a change was made was due to an ASSERT triggered by an existing layout test. I'll specifically look at the cases you mention below.

> 
> I'm OK r+ing this, but I'd rather see one last version.
> 
> I'll be around all day tommorow (and the rest of the week) and happy to approve the patch as soon as you post it.
> 
> I'll be working on looking at the remaining peacekeeper regressions tomorrow as well.
> 
> > Source/WebCore/html/parser/HTMLElementStack.cpp:53
> > +inline bool isRootMarker(ContainerNode* node)
> 
> I would have probably just called this isRootNode.  But it's OK as is. :)  IIRC, "markers" are a spec concept, of which is this not one. :)

I'll rename it.

> 
> > Source/WebCore/html/parser/HTMLElementStack.cpp:238
> > +    while (!isNumberedHeaderElement(topNode()))
> 
> This doesn't need to accept a Node*, since clearly the previous implementation didn't handle NULL or <html>.
> 
> > Source/WebCore/html/parser/HTMLTreeBuilder.cpp:2269
> > +            if (m_tree.currentNode()->hasTagName(optionTag)) {
> 
> Can't this be currentElement?
> 
> > Source/WebCore/html/parser/HTMLTreeBuilder.cpp:2309
> > +                if (nodeRecord->node()->nodeType() == Node::DOCUMENT_FRAGMENT_NODE) {
> 
> You shoudl comment on why this is needed.

Will do.

> 
> > Source/WebCore/html/parser/HTMLTreeBuilder.cpp:2638
> > +        if (m_tree.currentNode()->hasTagName(scriptTag))
> 
> Can the current node ever not be an element here?

[Andy Estes]

(In reply to comment #157)
> (From update of attachment 84167 [details])
> Thanks again for all your hard work on this.  I'm sorry it took so long, but hopefully you feel good came out of it.  I certainly do. :)

Thanks Eric! It feels great to be making progress on this :)

[Andy Estes]

(In reply to comment #156)
> (From update of attachment 84167 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=84167&action=review
> > Source/WebCore/html/parser/HTMLElementStack.cpp:238
> > +    while (!isNumberedHeaderElement(topNode()))
> 
> This doesn't need to accept a Node*, since clearly the previous implementation didn't handle NULL or <html>.

isNumberedHeaderElement() needs to take a ContainerNode* since hasNumberedHeaderElementInScope() can pop the stack all the way to the root, although that function can be reworked to check for a scope marker before checking for a numbered header element.

[Andy Estes]

(In reply to comment #163)
> isNumberedHeaderElement() needs to take a ContainerNode* since hasNumberedHeaderElementInScope() can pop the stack all the way to the root, although that function can be reworked to check for a scope marker before checking for a numbered header element.

Walk the stack, not pop the stack.

[Andy Estes]

Committed r80201: <http://trac.webkit.org/changeset/80201>

[Ryosuke Niwa]

(In reply to comment #165)
> Committed r80201: <http://trac.webkit.org/changeset/80201>

It seems like this patch caused a various tests on crash on many ports.

http://build.webkit.org/builders/Leopard%20Intel%20Release%20(Tests)/builds/29079
http://build.webkit.org/builders/GTK%20Linux%2064-bit%20Debug/builds/19956

[WebKit Review Bot]

http://trac.webkit.org/changeset/80201 might have broken GTK Linux 64-bit Debug

[Ryosuke Niwa]

Rolled out in http://trac.webkit.org/changeset/80209.

[Andy Estes]

Committed r80231: <http://trac.webkit.org/changeset/80231>

[Adam Barth]

Looks like Andy re-landing this patch.  If more work is needed here, please open a new bug.  This bug has gone epic.