Skip to content

Commit

Permalink
Revert of HTMLConstructionSite: avoid n^2 running time for large scri…
Browse files Browse the repository at this point in the history
…pts. (patchset #5 id:80001 of https://codereview.chromium.org/494993002/)

Reason for revert:
Speculative revert.

It may break the following blink sheriff bot:
http://build.chromium.org/p/chromium.webkit/builders/Android%20Tests%20%28dbg%29/builds/21403



Original issue's description:
> HTMLConstructionSite: avoid n^2 running time for large scripts.
> 
> Every time background parser sends chunk, tree is flushed.
> 
> If page contains very large script, then script node content is updated
> many times. Every update is causes string concatenation.
> 
> Solution: do not flush pending text until it is mandatory.
> 
> Test: https://codereview.chromium.org/500363002
> Test depends on: https://codereview.chromium.org/544453004/
> 
> BUG=410790
> 
> Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=181635

[email protected],[email protected],[email protected]
NOTREECHECKS=true
NOTRY=true
BUG=410790

Review URL: https://codereview.chromium.org/555223002

git-svn-id: svn://svn.chromium.org/blink/trunk@181664 bbb929c8-8fbe-4397-9dbb-9b2b20218538
  • Loading branch information
jianli-chromium committed Sep 9, 2014
1 parent f03316e commit 5f6530b
Show file tree
Hide file tree
Showing 5 changed files with 16 additions and 30 deletions.
14 changes: 5 additions & 9 deletions Source/core/html/parser/HTMLConstructionSite.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -231,15 +231,11 @@ static String atomizeIfAllWhitespace(const String& string, WhitespaceMode whites
return string;
}

void HTMLConstructionSite::flushPendingText(FlushMode mode)
void HTMLConstructionSite::flushPendingText()
{
if (m_pendingText.isEmpty())
return;

if (mode == FlushIfAtTextLimit
&& !shouldUseLengthLimit(*m_pendingText.parent))
return;

PendingText pendingText;
// Hold onto the current pending text on the stack so that queueTask doesn't recurse infinitely.
m_pendingText.swap(pendingText);
Expand Down Expand Up @@ -273,7 +269,7 @@ void HTMLConstructionSite::flushPendingText(FlushMode mode)

void HTMLConstructionSite::queueTask(const HTMLConstructionSiteTask& task)
{
flushPendingText(FlushAlways);
flushPendingText();
ASSERT(m_pendingText.isEmpty());
m_taskQueue.append(task);
}
Expand Down Expand Up @@ -537,15 +533,15 @@ void HTMLConstructionSite::setCompatibilityModeFromDoctype(const String& name, c
void HTMLConstructionSite::processEndOfFile()
{
ASSERT(currentNode());
flush(FlushAlways);
flush();
openElements()->popAll();
}

void HTMLConstructionSite::finishedParsing()
{
// We shouldn't have any queued tasks but we might have pending text which we need to promote to tasks and execute.
ASSERT(m_taskQueue.isEmpty());
flush(FlushAlways);
flush();
m_document->finishedParsing();
}

Expand Down Expand Up @@ -693,7 +689,7 @@ void HTMLConstructionSite::insertTextNode(const String& string, WhitespaceMode w
// The nextChild != dummy.nextChild case occurs whenever foster parenting happened and we hit a new text node "<table>a</table>b"
// In either case we have to flush the pending text into the task queue before making more.
if (!m_pendingText.isEmpty() && (m_pendingText.parent != dummyTask.parent || m_pendingText.nextChild != dummyTask.nextChild))
flushPendingText(FlushAlways);
flushPendingText();
m_pendingText.append(dummyTask.parent, dummyTask.nextChild, string, whitespaceMode);
}

Expand Down
16 changes: 4 additions & 12 deletions Source/core/html/parser/HTMLConstructionSite.h
Original file line number Diff line number Diff line change
Expand Up @@ -92,14 +92,6 @@ enum WhitespaceMode {
AllWhitespace,
};

enum FlushMode {
// Flush pending text. Flush queued tasks.
FlushAlways,

// Flush pending text if node has length limit. Flush queued tasks.
FlushIfAtTextLimit,
};

class AtomicHTMLToken;
class Document;
class Element;
Expand All @@ -121,16 +113,16 @@ class HTMLConstructionSite FINAL {
void executeQueuedTasks();

// flushPendingText turns pending text into queued Text insertions, but does not execute them.
void flushPendingText(FlushMode);
void flushPendingText();

// Called before every token in HTMLTreeBuilder::processToken, thus inlined:
void flush(FlushMode mode)
void flush()
{
if (!hasPendingTasks())
return;
flushPendingText(mode);
flushPendingText();
executeQueuedTasks(); // NOTE: Possible reentrancy via JavaScript execution.
ASSERT(mode == FlushIfAtTextLimit || !hasPendingTasks());
ASSERT(!hasPendingTasks());
}

bool hasPendingTasks()
Expand Down
7 changes: 3 additions & 4 deletions Source/core/html/parser/HTMLDocumentParser.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -498,10 +498,9 @@ void HTMLDocumentParser::processParsedChunkFromBackgroundParser(PassOwnPtr<Parse
ASSERT(!m_token);
}

// Make sure all required pending text nodes are emitted before returning.
// This leaves "script", "style" and "svg" nodes text nodes intact.
// Make sure any pending text nodes are emitted before returning.
if (!isStopped())
m_treeBuilder->flush(FlushIfAtTextLimit);
m_treeBuilder->flush();
}

void HTMLDocumentParser::pumpPendingSpeculations()
Expand Down Expand Up @@ -636,7 +635,7 @@ void HTMLDocumentParser::pumpTokenizer(SynchronousMode mode)
// There should only be PendingText left since the tree-builder always flushes
// the task queue before returning. In case that ever changes, crash.
if (mode == ForceSynchronous)
m_treeBuilder->flush(FlushAlways);
m_treeBuilder->flush();
RELEASE_ASSERT(!isStopped());

if (session.needsYield)
Expand Down
4 changes: 2 additions & 2 deletions Source/core/html/parser/HTMLTreeBuilder.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -402,7 +402,7 @@ void HTMLTreeBuilder::processToken(AtomicHTMLToken* token)

// Any non-character token needs to cause us to flush any pending text immediately.
// NOTE: flush() can cause any queued tasks to execute, possibly re-entering the parser.
m_tree.flush(FlushAlways);
m_tree.flush();
m_shouldSkipLeadingNewline = false;

switch (token->type()) {
Expand Down Expand Up @@ -2689,7 +2689,7 @@ void HTMLTreeBuilder::processTokenInForeignContent(AtomicHTMLToken* token)
return;
}

m_tree.flush(FlushAlways);
m_tree.flush();
HTMLStackItem* adjustedCurrentNode = adjustedCurrentStackItem();

switch (token->type()) {
Expand Down
5 changes: 2 additions & 3 deletions Source/core/html/parser/HTMLTreeBuilder.h
Original file line number Diff line number Diff line change
Expand Up @@ -82,9 +82,8 @@ class HTMLTreeBuilder FINAL : public NoBaseWillBeGarbageCollectedFinalized<HTMLT
// Done, close any open tags, etc.
void finished();

// Synchronously flush pending text and queued tasks, possibly creating more DOM nodes.
// Flushing pending text depends on |mode|.
void flush(FlushMode mode) { m_tree.flush(mode); }
// Synchronously empty any queues, possibly creating more DOM nodes.
void flush() { m_tree.flush(); }

void setShouldSkipLeadingNewline(bool shouldSkip) { m_shouldSkipLeadingNewline = shouldSkip; }

Expand Down

0 comments on commit 5f6530b

Please sign in to comment.