Full Text Bug Listing

Simon Fraser (smfr)

Reported 2010-10-30 13:17:20 PDT

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)

Comment 1 2010-10-30 13:18:07 PDT

<rdar://problem/8613646>

Simon Fraser (smfr)

Comment 2 2010-10-30 13:19:04 PDT

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)

Comment 3 2010-10-30 13:22:39 PDT

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)

Comment 4 2010-10-30 14:25:13 PDT

Yay more perf tests! I'm certain this will be trivial to fix. Will look tomorrow.

Simon Fraser (smfr)

Comment 5 2010-10-31 11:35:32 PDT

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)

Comment 6 2010-10-31 11:54:29 PDT

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)

Comment 7 2010-10-31 11:54:58 PDT

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

Adam Barth

Comment 8 2010-10-31 11:55:09 PDT

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

Simon Fraser (smfr)

Comment 9 2010-10-31 11:55:13 PDT

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

Adam Barth

Comment 10 2010-10-31 11:56:29 PDT

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

Eric Seidel (no email)

Comment 11 2010-10-31 11:58:37 PDT

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)

Comment 12 2010-10-31 18:23:36 PDT

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

Eric Seidel (no email)

Comment 13 2010-10-31 18:25:57 PDT

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)

Comment 14 2010-10-31 18:32:59 PDT

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)

Comment 15 2010-10-31 18:39:08 PDT

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)

Comment 16 2010-10-31 22:11:50 PDT

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)

Comment 17 2010-10-31 22:27:21 PDT

(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)

Comment 18 2010-11-01 00:46:56 PDT

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

Eric Seidel (no email)

Comment 19 2010-11-01 01:02:38 PDT

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 20 2010-11-01 14:10:06 PDT

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)

Comment 21 2010-11-01 14:14:33 PDT

Created attachment 72557 [details] better wip

Eric Seidel (no email)

Comment 22 2010-11-01 14:15:51 PDT

(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)

Comment 23 2010-11-01 14:46:51 PDT

Created attachment 72566 [details] Patch

Darin Adler

Comment 24 2010-11-03 12:07:14 PDT

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

Comment 25 2010-11-03 12:07:46 PDT

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

Eric Seidel (no email)

Comment 26 2010-11-03 12:17:00 PDT

(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)

Comment 27 2010-11-03 12:20:44 PDT

(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

Comment 28 2010-11-03 12:47:46 PDT

(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

Comment 29 2010-11-03 12:48:32 PDT

(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

Comment 30 2010-11-03 12:49:01 PDT

(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

Comment 31 2010-11-03 12:51:40 PDT

(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 32 2011-01-10 18:27:34 PST

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

Comment 33 2011-01-11 09:55:56 PST

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

Eric Seidel (no email)

Comment 34 2011-01-28 17:03:41 PST

Created attachment 80528 [details] Patch for landing

WebKit Commit Bot

Comment 35 2011-01-28 23:38:10 PST

Comment on attachment 80528 [details] Patch for landing Clearing flags on attachment: 80528 Committed r77050: <http://trac.webkit.org/changeset/77050>

WebKit Commit Bot

Comment 36 2011-01-28 23:38:18 PST

All reviewed patches have been landed. Closing bug.

WebKit Review Bot

Comment 37 2011-01-29 00:00:58 PST

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

Ryosuke Niwa

Comment 38 2011-01-29 00:19:33 PST

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

Comment 39 2011-01-29 00:27:05 PST

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

Comment 40 2011-01-29 00:38:48 PST

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

Comment 41 2011-01-31 13:16:54 PST

(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)

Comment 42 2011-02-01 14:52:11 PST

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)

Comment 43 2011-02-01 14:55:06 PST

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

Comment 44 2011-02-01 15:04:58 PST

(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)

Comment 45 2011-02-01 15:08:08 PST

(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)

Comment 46 2011-02-01 15:35:38 PST

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)

Comment 47 2011-02-02 14:25:38 PST

Created attachment 80971 [details] Patch for landing

Eric Seidel (no email)

Comment 48 2011-02-02 14:26:58 PST

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

Comment 49 2011-02-02 14:33:40 PST

(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)

Comment 50 2011-02-02 14:44:45 PST

(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

Comment 51 2011-02-02 14:53:50 PST

(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)

Comment 52 2011-02-02 14:55:07 PST

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

Comment 53 2011-02-02 15:02:25 PST

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

Mihai Parparita

Comment 54 2011-02-02 15:04:56 PST

(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

Comment 55 2011-02-02 15:09:33 PST

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

Eric Seidel (no email)

Comment 56 2011-02-02 15:20:31 PST

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 57 2011-02-02 15:29:10 PST

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)

Comment 58 2011-02-02 15:33:11 PST

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

Eric Seidel (no email)

Comment 59 2011-02-02 15:33:41 PST

I landed the benchmark.

WebKit Commit Bot

Comment 60 2011-02-02 15:53:50 PST

The commit-queue encountered the following flaky tests while processing attachment 80971 [details]: http/tests/xmlhttprequest/cross-origin-no-authorization.html bug 33357 (author: ap@webkit.org) The commit-queue is continuing to process your patch.

Andy Estes

Comment 61 2011-02-02 15:59:31 PST

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)

Comment 62 2011-02-02 16:19:43 PST

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

Comment 63 2011-02-02 16:20:34 PST

(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

Comment 64 2011-02-02 16:28:13 PST

Maybe use a generic ContainerNode instead of a Document?

Eric Seidel (no email)

Comment 65 2011-02-02 16:38:22 PST

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)

Comment 66 2011-02-02 16:41:28 PST

(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)

Comment 67 2011-02-02 16:56:07 PST

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

Andy Estes

Comment 68 2011-02-08 13:42:53 PST

Created attachment 81685 [details] Patch

Andy Estes

Comment 69 2011-02-08 13:45:45 PST

(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 70 2011-02-08 13:49:59 PST

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)

Comment 71 2011-02-08 13:51:58 PST

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

Comment 72 2011-02-08 14:17:39 PST

(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

Comment 73 2011-02-08 14:25:25 PST

(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

Comment 74 2011-02-08 14:30:44 PST

Created attachment 81694 [details] Patch

Adam Barth

Comment 75 2011-02-08 14:34:40 PST

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

Comment 76 2011-02-08 14:37:53 PST

(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

Comment 77 2011-02-08 15:02:37 PST

(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

Comment 78 2011-02-08 15:06:28 PST

Awesome! Thanks.

Geoffrey Garen

Comment 79 2011-02-08 18:25:29 PST

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 80 2011-02-08 21:21:08 PST

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)

Comment 81 2011-02-08 21:23:45 PST

I'd like to take another look through. I'll do so in about an hour.

Eric Seidel (no email)

Comment 82 2011-02-08 23:23:09 PST

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 83 2011-02-08 23:35:32 PST

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 84 2011-02-08 23:41:07 PST

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 85 2011-02-08 23:53:27 PST

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 86 2011-02-09 00:02:34 PST

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 87 2011-02-09 00:10:47 PST

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

Comment 88 2011-02-09 13:33:59 PST

(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

Comment 89 2011-02-09 13:47:45 PST

(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

Comment 90 2011-02-09 13:55:23 PST

(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

Comment 91 2011-02-09 14:00:59 PST

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

Adam Barth

Comment 92 2011-02-09 14:10:40 PST

> > 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

Comment 93 2011-02-09 14:43:13 PST

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)

Comment 94 2011-02-09 14:47:05 PST

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

Comment 95 2011-02-09 16:51:06 PST

(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

Comment 96 2011-02-09 19:38:38 PST

Created attachment 81914 [details] Patch

Andy Estes

Comment 97 2011-02-09 19:39:51 PST

(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 98 2011-02-09 19:54:17 PST

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

Comment 99 2011-02-10 14:50:34 PST

(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

Comment 100 2011-02-10 14:59:31 PST

Created attachment 82053 [details] Patch

Darin Adler

Comment 101 2011-02-10 15:09:04 PST

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)

Comment 102 2011-02-10 15:17:35 PST

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

Eric Seidel (no email)

Comment 103 2011-02-10 15:31:27 PST

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)

Comment 104 2011-02-10 15:35:22 PST

(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 105 2011-02-10 15:42:52 PST

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)

Comment 106 2011-02-10 15:46:31 PST

(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

Comment 107 2011-02-10 15:52:52 PST

(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)

Comment 108 2011-02-10 15:53:30 PST

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

Comment 109 2011-02-10 16:45:38 PST

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

Andy Estes

Comment 110 2011-02-11 15:12:18 PST

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

Comment 111 2011-02-11 15:40:40 PST

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

Comment 112 2011-02-11 17:08:21 PST

(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

Comment 113 2011-02-11 19:25:43 PST

(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

Comment 114 2011-02-11 20:33:36 PST

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)

Comment 115 2011-02-11 21:41:48 PST

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 116 2011-02-16 11:23:42 PST

Comment on attachment 82053 [details] Patch Clearing the review flag so this doesn't show up in pending-commit.

Andy Estes

Comment 117 2011-02-16 13:48:10 PST

(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

Comment 118 2011-02-18 15:15:06 PST

Created attachment 83022 [details] Patch

Andy Estes

Comment 119 2011-02-18 15:17:46 PST

(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

Comment 120 2011-02-18 15:20:01 PST

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 121 2011-02-18 15:35:50 PST

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

Comment 122 2011-02-18 15:42:45 PST

(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 123 2011-02-18 16:01:28 PST

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

Comment 124 2011-02-20 17:31:31 PST

(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 125 2011-02-22 11:57:45 PST

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)

Comment 126 2011-02-22 14:15:53 PST

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

Andy Estes

Comment 127 2011-02-22 15:55:25 PST

(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 = "...")? > > > 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)

Comment 128 2011-02-22 16:00:50 PST

(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)

Comment 129 2011-02-22 19:36:00 PST

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)

Comment 130 2011-02-22 20:06:17 PST

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

Comment 131 2011-02-22 20:08:38 PST

(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)

Comment 132 2011-02-24 01:16:55 PST

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)

Comment 133 2011-02-24 01:23:21 PST

(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)

Comment 134 2011-02-24 01:27:28 PST

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)

Comment 135 2011-02-24 07:58:02 PST

James Robinson has the Peacekeeper sources.

Andy Estes

Comment 136 2011-02-24 12:43:57 PST

(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)

Comment 137 2011-02-24 12:51:30 PST

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

Comment 138 2011-02-24 22:29:27 PST

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 139 2011-02-24 22:55:25 PST

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

Comment 140 2011-02-24 23:04:08 PST

(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 141 2011-02-25 03:38:27 PST

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)

Comment 142 2011-02-25 04:05:11 PST

(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

Comment 143 2011-02-25 18:30:47 PST

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)

Comment 144 2011-02-27 02:13:31 PST

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)

Comment 145 2011-02-27 02:14:46 PST

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)

Comment 146 2011-02-27 02:15:44 PST

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 147 2011-02-27 02:24:17 PST

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)

Comment 148 2011-02-27 02:25:14 PST

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

Maciej Stachowiak

Comment 149 2011-02-28 14:46:54 PST

(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

Comment 150 2011-02-28 17:35:32 PST

(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

Comment 151 2011-02-28 17:59:41 PST

Created attachment 84167 [details] Patch

Andy Estes

Comment 152 2011-02-28 18:24:48 PST

(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

Comment 153 2011-03-01 14:05:46 PST

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)

Comment 154 2011-03-01 22:13:05 PST

(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)

Comment 155 2011-03-01 22:13:48 PST

(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 156 2011-03-01 22:33:12 PST

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 157 2011-03-01 22:40:43 PST

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

Comment 158 2011-03-02 15:08:42 PST

(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

Comment 159 2011-03-02 15:10:36 PST

(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)

Comment 160 2011-03-02 16:09:35 PST

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

Comment 161 2011-03-02 16:29:58 PST

(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

Comment 162 2011-03-02 16:31:16 PST

(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

Comment 163 2011-03-02 17:09:13 PST

(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

Comment 164 2011-03-02 17:10:22 PST

(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

Comment 165 2011-03-02 20:41:16 PST

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

Ryosuke Niwa

Comment 166 2011-03-02 22:00:05 PST

(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

Comment 167 2011-03-02 22:17:49 PST

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

Ryosuke Niwa

Comment 168 2011-03-02 22:19:46 PST

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

Andy Estes

Comment 169 2011-03-03 04:13:57 PST

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

Adam Barth

Comment 170 2011-06-17 23:05:55 PDT

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

Attachments
The js for the test (2.08 KB, text/plain) 2010-10-30 13:22 PDT, Simon Fraser (smfr)	no flags	Details
Profile focused on setInnerHTML, Safari 5.0.2 (9.42 KB, text/plain) 2010-10-31 11:54 PDT, Simon Fraser (smfr)	no flags	Details
Profile focused on setInnerHTML, TOT (9.41 KB, text/plain) 2010-10-31 11:55 PDT, Simon Fraser (smfr)	no flags	Details
Simple Benchmark shows TOT is 10x slower than Safari 5 at innerHTML in a loop (1.52 KB, text/html) 2010-10-31 18:39 PDT, Eric Seidel (no email)	no flags	Details
work in progress, closes the gap to 2x as slow instead of 7x or 10x (4.65 KB, patch) 2010-11-01 00:46 PDT, Eric Seidel (no email)	no flags	Details Formatted Diff Diff
better wip (10.10 KB, patch) 2010-11-01 14:14 PDT, Eric Seidel (no email)	no flags	Details Formatted Diff Diff
Patch (12.52 KB, patch) 2010-11-01 14:46 PDT, Eric Seidel (no email)	no flags	Details Formatted Diff Diff
Patch for landing (10.77 KB, patch) 2011-01-28 17:03 PST, Eric Seidel (no email)	no flags	Details Formatted Diff Diff
Patch for landing (11.56 KB, patch) 2011-02-02 14:25 PST, Eric Seidel (no email)	no flags	Details Formatted Diff Diff
Patch (9.78 KB, patch) 2011-02-08 13:42 PST, Andy Estes	no flags	Details Formatted Diff Diff
Patch (9.83 KB, patch) 2011-02-08 14:30 PST, Andy Estes	no flags	Details Formatted Diff Diff
Fragment DOCTYPE test case (402 bytes, text/html) 2011-02-09 14:43 PST, Andy Estes	no flags	Details
Patch (15.16 KB, patch) 2011-02-09 19:38 PST, Andy Estes	no flags	Details Formatted Diff Diff
Patch (15.20 KB, patch) 2011-02-10 14:59 PST, Andy Estes	no flags	Details Formatted Diff Diff
Instruments profile while running tiny-innerHTML (1.49 MB, application/zip) 2011-02-11 15:12 PST, Andy Estes	no flags	Details
Patch (49.12 KB, patch) 2011-02-18 15:15 PST, Andy Estes	no flags	Details Formatted Diff Diff
Patch (49.06 KB, patch) 2011-02-25 18:30 PST, Andy Estes	no flags	Details Formatted Diff Diff
Patch (47.81 KB, patch) 2011-02-28 17:59 PST, Andy Estes	eric: review+	Details Formatted Diff Diff
Show Obsolete (11) View All Add attachment proposed patch, testcase, etc.

Bug 48719