Commit 5f6b9d4c authored by Nate Chapin's avatar Nate Chapin Committed by Commit Bot

Remove custom pausing logic from HTMLDocumentParser

All it does is cancel a timer and restart it when unpausing, which
is unnecessary as the underlying task queue will be paused, too.

HTMLDocumentParser had recently been made a
ContextLifecycleStateObserver, which had caused a substantial
slowdown in perf_tests/parser/tiny-innerHTML.html. This CL removes
that inheritance and reverses the slowdown.

Bug: 1030905
Change-Id: I4c296869b60db8c152c14cf1619051c21918ef82
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1955849
Commit-Queue: Nate Chapin <japhet@chromium.org>
Reviewed-by: default avatarKouhei Ueno <kouhei@chromium.org>
Reviewed-by: default avatarMason Freed <masonfreed@chromium.org>
Auto-Submit: Nate Chapin <japhet@chromium.org>
Cr-Commit-Position: refs/heads/master@{#723497}
parent e868053f
......@@ -131,7 +131,6 @@ HTMLDocumentParser::HTMLDocumentParser(Document& document,
ParserContentPolicy content_policy,
ParserSynchronizationPolicy sync_policy)
: ScriptableDocumentParser(document, content_policy),
ContextLifecycleStateObserver(&document),
options_(&document),
reentry_permit_(HTMLParserReentryPermit::Create()),
token_(sync_policy == kForceSynchronousParsing
......@@ -160,8 +159,6 @@ HTMLDocumentParser::HTMLDocumentParser(Document& document,
// Threading is not allowed in prefetch mode.
DCHECK(!document.IsPrefetchOnly() || !ShouldUseThreading());
UpdateStateIfNeeded();
// Don't create preloader for parsing clipboard content.
if (content_policy == kDisallowScriptingAndPluginContent)
return;
......@@ -194,7 +191,6 @@ void HTMLDocumentParser::Trace(Visitor* visitor) {
visitor->Trace(script_runner_);
visitor->Trace(preloader_);
ScriptableDocumentParser::Trace(visitor);
ContextLifecycleStateObserver::Trace(visitor);
HTMLParserScriptRunnerHost::Trace(visitor);
}
......@@ -390,12 +386,8 @@ void HTMLDocumentParser::EnqueueTokenizedChunk(
speculations_.push_back(std::move(chunk));
if (!IsPaused() && !IsScheduledForUnpause()) {
if (GetDocument()->IsContextPaused())
parser_scheduler_->ForceUnpauseAfterYield();
else
parser_scheduler_->ScheduleForUnpause();
}
if (!IsPaused() && !IsScheduledForUnpause())
parser_scheduler_->ScheduleForUnpause();
}
void HTMLDocumentParser::DidReceiveEncodingDataFromBackgroundParser(
......@@ -1119,16 +1111,6 @@ void HTMLDocumentParser::ParseDocumentFragment(
parser->Detach();
}
void HTMLDocumentParser::ContextLifecycleStateChanged(
mojom::FrameLifecycleState state) {
if (!parser_scheduler_)
return;
if (state == mojom::FrameLifecycleState::kRunning)
parser_scheduler_->Unpause();
else
parser_scheduler_->Pause();
}
void HTMLDocumentParser::AppendBytes(const char* data, size_t length) {
if (!length || IsStopped())
return;
......
......@@ -32,7 +32,6 @@
#include "third_party/blink/renderer/core/core_export.h"
#include "third_party/blink/renderer/core/dom/parser_content_policy.h"
#include "third_party/blink/renderer/core/dom/scriptable_document_parser.h"
#include "third_party/blink/renderer/core/execution_context/context_lifecycle_state_observer.h"
#include "third_party/blink/renderer/core/html/parser/background_html_input_stream.h"
#include "third_party/blink/renderer/core/html/parser/html_input_stream.h"
#include "third_party/blink/renderer/core/html/parser/html_parser_options.h"
......@@ -66,7 +65,6 @@ class HTMLResourcePreloader;
class HTMLTreeBuilder;
class CORE_EXPORT HTMLDocumentParser : public ScriptableDocumentParser,
public ContextLifecycleStateObserver,
private HTMLParserScriptRunnerHost {
USING_GARBAGE_COLLECTED_MIXIN(HTMLDocumentParser);
USING_PRE_FINALIZER(HTMLDocumentParser, Dispose);
......@@ -102,8 +100,6 @@ class CORE_EXPORT HTMLDocumentParser : public ScriptableDocumentParser,
bool IsParsingAtLineNumber() const final;
OrdinalNumber LineNumber() const final;
void ContextLifecycleStateChanged(mojom::FrameLifecycleState) final;
HTMLParserReentryPermit* ReentryPermit() { return reentry_permit_.get(); }
struct TokenizedChunk {
......
......@@ -51,9 +51,7 @@ void SpeculationsPumpSession::AddedElementTokens(size_t count) {
HTMLParserScheduler::HTMLParserScheduler(
HTMLDocumentParser* parser,
scoped_refptr<base::SingleThreadTaskRunner> loading_task_runner)
: parser_(parser),
loading_task_runner_(std::move(loading_task_runner)),
is_paused_with_active_timer_(false) {}
: parser_(parser), loading_task_runner_(std::move(loading_task_runner)) {}
HTMLParserScheduler::~HTMLParserScheduler() = default;
......@@ -62,37 +60,18 @@ void HTMLParserScheduler::Trace(Visitor* visitor) {
}
bool HTMLParserScheduler::IsScheduledForUnpause() const {
return is_paused_with_active_timer_ ||
cancellable_continue_parse_task_handle_.IsActive();
return cancellable_continue_parse_task_handle_.IsActive();
}
void HTMLParserScheduler::ScheduleForUnpause() {
DCHECK(!is_paused_with_active_timer_);
cancellable_continue_parse_task_handle_ =
PostCancellableTask(*loading_task_runner_, FROM_HERE,
WTF::Bind(&HTMLParserScheduler::ContinueParsing,
WrapWeakPersistent(this)));
}
void HTMLParserScheduler::Pause() {
DCHECK(!is_paused_with_active_timer_);
if (!cancellable_continue_parse_task_handle_.IsActive())
return;
is_paused_with_active_timer_ = true;
cancellable_continue_parse_task_handle_.Cancel();
}
void HTMLParserScheduler::Unpause() {
DCHECK(!cancellable_continue_parse_task_handle_.IsActive());
if (!is_paused_with_active_timer_)
return;
is_paused_with_active_timer_ = false;
ScheduleForUnpause();
}
void HTMLParserScheduler::Detach() {
cancellable_continue_parse_task_handle_.Cancel();
is_paused_with_active_timer_ = false;
}
inline bool HTMLParserScheduler::ShouldYield(
......@@ -131,11 +110,6 @@ bool HTMLParserScheduler::YieldIfNeeded(const SpeculationsPumpSession& session,
return false;
}
void HTMLParserScheduler::ForceUnpauseAfterYield() {
DCHECK(!cancellable_continue_parse_task_handle_.IsActive());
is_paused_with_active_timer_ = true;
}
void HTMLParserScheduler::ContinueParsing() {
parser_->ResumeParsingAfterYield();
}
......
......@@ -64,18 +64,6 @@ class HTMLParserScheduler final : public GarbageCollected<HTMLParserScheduler> {
void ScheduleForUnpause();
bool YieldIfNeeded(const SpeculationsPumpSession&, bool starting_script);
/**
* Can only be called if this scheduler is paused. If this is called,
* then after the scheduler is resumed by calling resume(), this call
* ensures that HTMLDocumentParser::resumeAfterYield will be called. Used to
* signal this scheduler that the background html parser sent chunks to
* HTMLDocumentParser while it was paused.
*/
void ForceUnpauseAfterYield();
void Pause();
void Unpause();
void Detach(); // Clear active tasks if any.
void Trace(Visitor*);
......@@ -88,7 +76,6 @@ class HTMLParserScheduler final : public GarbageCollected<HTMLParserScheduler> {
scoped_refptr<base::SingleThreadTaskRunner> loading_task_runner_;
TaskHandle cancellable_continue_parse_task_handle_;
bool is_paused_with_active_timer_;
DISALLOW_COPY_AND_ASSIGN(HTMLParserScheduler);
};
......
Tests that reparenting media elements also reparents ActiveDOMObject.
Before Reparenting
PASS: internals.contextLifecycleStateObserverObjectCount(document) should be '1' and is.
PASS: internals.contextLifecycleStateObserverObjectCount(iframe) should be '4' and is.
PASS: internals.contextLifecycleStateObserverObjectCount(document) should be '0' and is.
PASS: internals.contextLifecycleStateObserverObjectCount(iframe) should be '3' and is.
After Reparenting
PASS: internals.contextLifecycleStateObserverObjectCount(document) should be '4' and is.
PASS: internals.contextLifecycleStateObserverObjectCount(iframe) should be '2' and is.
PASS: internals.contextLifecycleStateObserverObjectCount(document) should be '3' and is.
PASS: internals.contextLifecycleStateObserverObjectCount(iframe) should be '1' and is.
......@@ -10,8 +10,8 @@
window.iframe = document.querySelector('iframe').contentDocument;
log('Before Reparenting');
shouldBe('internals.contextLifecycleStateObserverObjectCount(document)', 1);
shouldBe('internals.contextLifecycleStateObserverObjectCount(iframe)', 4);
shouldBe('internals.contextLifecycleStateObserverObjectCount(document)', 0);
shouldBe('internals.contextLifecycleStateObserverObjectCount(iframe)', 3);
document.body.appendChild(window.iframe.querySelector('video'));
......@@ -20,8 +20,8 @@
// but a new one will be created in |document|, so the count is expected
// to differ by one.
log('After Reparenting');
shouldBe('internals.contextLifecycleStateObserverObjectCount(document)', 4);
shouldBe('internals.contextLifecycleStateObserverObjectCount(iframe)', 2);
shouldBe('internals.contextLifecycleStateObserverObjectCount(document)', 3);
shouldBe('internals.contextLifecycleStateObserverObjectCount(iframe)', 1);
}
</script>
</body>
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment