Call checkCompleted() synchronously in HTMLLinkElement::dispatchPendingEvent()

Call checkCompleted() synchronously in HTMLLinkElement::dispatchPendingEvent()

Previously, there was an asynchronous hop from <link>'s load event to
document's load event, because HTMLLinkElement::dispatchPendingEvent()
scheduled checkCompleted() asynchronously via IncrementLoadEventDelayCount.

This CL makes HTMLLinkElement::dispatchPendingEvent() to call checkCompleted()
synchronously, by using
IncrementLoadEventDelayCount::clearAndCheckLoadEvent().

Because checkCompleted() is called at the end of asynchronously called
HTMLLinkElement::dispatchPendingEvent(), this CL doesn't increase synchronous
timing dependencies around Document load events.

This CL fixes the performance regression of smoothness.top_25_smooth on
Mac 10.10 caused by https://codereview.chromium.org/2275493002/.

BUG=648887, 624697
Review-Url: https://codereview.chromium.org/2391923002
Cr-Commit-Position: refs/heads/master@{#444965}
Committed: https://chromium.googlesource.com/chromium/src/+/46649a2f0bf126330d95f47b6c8773823451d23f

[hiroshige, 2016-10-04 05:48:39]

CL for src perf tryjob to run smoothness.top_25_smooth benchmark on mac-10-10
platform(s)

[hiroshige, 2016-10-04 05:57:13]

CL for src perf tryjob to run smoothness.top_25_smooth benchmark on mac-10-10
platform(s)

[hiroshige, 2016-10-04 05:57:58]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-04 05:58:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-04 06:07:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-04 06:07:03]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[hiroshige, 2016-10-04 10:04:33]

CL for src perf tryjob to run smoothness.top_25_smooth benchmark on mac-10-10
platform(s)

[hiroshige, 2016-10-04 10:27:16]

Description was changed from

==========
CL for src perf tryjob to run smoothness.top_25_smooth benchmark on mac-10-10
platform(s)
==========

to

==========
Dispatch Document load event synchronously after HTMLLinkElement load event

This CL is to fix performance regression of smoothness.top_25_smooth on
Mac 10.10 caused by [1].

Before [1], both load events of Document and HTMLLinkElement could be called
in Document::implicitClose() without an asynchronous postTask() between them.
[1] made the Document load event always called asynchronously after the load
event of HTMLLinkElement (via Document::decrementLoadEventDelayCount()).

This CL calls FrameLoader::checkCompleted() synchronously just after a
HTMLLinkElement's load event, to call a Document load event if applicable.
Because checkCompleted() is called at the end of asynchronously called
HTMLLinkElement::dispatchPendingEvent(), this CL doesn't increase synchronous
timing dependencies around Document load events.

[1] https://codereview.chromium.org/2275493002

BUG=648887, 624697
==========

[hiroshige, 2016-10-04 10:27:38]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-04 10:27:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hiroshige, 2016-10-04 10:33:05]

hiroshige@chromium.org changed reviewers:
+ haraken@chromium.org, nhiroki@chromium.org, yhirano@chromium.org

[hiroshige, 2016-10-04 10:33:05]

WDYT?

[hiroshige, 2016-10-04 10:39:54]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-04 10:40:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-04 11:49:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-04 11:49:07]

Dry run: This issue passed the CQ dry run.

[yhirano, 2016-10-06 01:44:06]

On 2016/10/04 10:33:05, hiroshige wrote:
> WDYT?

The code looks correct but I don't have enough expertise to see if the metrics
you are trying to improve is important enough to justify the extra complexity.

[hiroshige, 2016-10-06 03:08:02]

hiroshige@chromium.org changed reviewers:
+ vmiura@chromium.org

[hiroshige, 2016-10-06 03:08:02]

On 2016/10/06 01:44:06, yhirano wrote:
> On 2016/10/04 10:33:05, hiroshige wrote:
> > WDYT?
> 
> The code looks correct but I don't have enough expertise to see if the metrics
> you are trying to improve is important enough to justify the extra complexity.

+vmiura@ as smoothness.top_25_smooth owner.
How important is it to fix a Mac10.10-only regression of
smoothness.top_25_smooth (Issue 648887)?

[hiroshige, 2016-10-16 09:59:04]

On 2016/10/06 03:08:02, hiroshige (OOO) wrote:
> On 2016/10/06 01:44:06, yhirano wrote:
> > On 2016/10/04 10:33:05, hiroshige wrote:
> > > WDYT?
> > 
> > The code looks correct but I don't have enough expertise to see if the
metrics
> > you are trying to improve is important enough to justify the extra
complexity.
> 
> +vmiura@ as smoothness.top_25_smooth owner.
> How important is it to fix a Mac10.10-only regression of
> smoothness.top_25_smooth (Issue 648887)?

Issue 648887 was closed as WontFix, so closing this CL also.

[hiroshige, 2016-12-27 21:40:21]

hiroshige@chromium.org changed reviewers:
- haraken@chromium.org, nhiroki@chromium.org, vmiura@chromium.org,
yhirano@chromium.org

[hiroshige, 2016-12-28 23:51:31]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-28 23:51:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-29 01:22:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-29 01:22:25]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[hiroshige, 2016-12-29 20:50:16]

Description was changed from

==========
Dispatch Document load event synchronously after HTMLLinkElement load event

This CL is to fix performance regression of smoothness.top_25_smooth on
Mac 10.10 caused by [1].

Before [1], both load events of Document and HTMLLinkElement could be called
in Document::implicitClose() without an asynchronous postTask() between them.
[1] made the Document load event always called asynchronously after the load
event of HTMLLinkElement (via Document::decrementLoadEventDelayCount()).

This CL calls FrameLoader::checkCompleted() synchronously just after a
HTMLLinkElement's load event, to call a Document load event if applicable.
Because checkCompleted() is called at the end of asynchronously called
HTMLLinkElement::dispatchPendingEvent(), this CL doesn't increase synchronous
timing dependencies around Document load events.

[1] https://codereview.chromium.org/2275493002

BUG=648887, 624697
==========

to

==========
Call checkCompleted() synchronously in HTMLLinkElement::dispatchPendingEvent()

Previously, there was an asynchronous hop from <link>'s load event to
document's load event, because HTMLLinkElement::dispatchPendingEvent()
scheduled checkCompleted() asynchronously via IncrementLoadEventDelayCount.

This CL makes HTMLLinkElement::dispatchPendingEvent() to call checkCompleted()
synchronously, by using
IncrementLoadEventDelayCount::clearAndCheckLoadEvent().

Because checkCompleted() is called at the end of asynchronously called
HTMLLinkElement::dispatchPendingEvent(), this CL doesn't increase synchronous
timing dependencies around Document load events.

This CL fixes the performance regression of smoothness.top_25_smooth on
Mac 10.10 caused by https://codereview.chromium.org/2275493002/.

BUG=648887, 624697
==========

[hiroshige, 2016-12-29 20:50:16]

hiroshige@chromium.org changed reviewers:
+ japhet@chromium.org, yhirano@chromium.org

[hiroshige, 2016-12-29 21:54:17]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-29 21:54:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hiroshige, 2016-12-29 22:07:22]

Hi Hirano-san,

I reopened this issue because calling checkCompleted() sync also resolves
the performance regression caused by HTMLStyleElement's EventSender removal
(https://codereview.chromium.org/2605173002#ps40001),
and now I feel this might prevent potential performance issues.

I create a separate CL that adds synchronous checkCompleted() functionality
to IncrementLoadEventDelayCount:
https://codereview.chromium.org/2605383002/ and make this CL to use it.

Could you take a look again?

[commit-bot: I haz the power, 2016-12-29 23:40:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-29 23:40:39]

Dry run: This issue passed the CQ dry run.

[hiroshige, 2017-01-03 19:52:44]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-03 19:53:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-03 21:22:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-03 21:22:50]

Dry run: This issue passed the CQ dry run.

[yhirano, 2017-01-06 13:40:04]

lgtm

[hiroshige, 2017-01-13 18:43:45]

japhet@, could you also take a look as a core/ OWNER? Thanks!

[Nate Chapin, 2017-01-18 22:07:25]

lgtm

[hiroshige, 2017-01-19 21:55:31]

The CQ bit was checked by hiroshige@chromium.org

[commit-bot: I haz the power, 2017-01-19 21:56:05]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-19 23:30:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-19 23:30:44]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[hiroshige, 2017-01-19 23:32:19]

The CQ bit was checked by hiroshige@chromium.org

[commit-bot: I haz the power, 2017-01-19 23:33:46]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-20 01:22:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-20 01:22:25]

Failed to apply patch for third_party/WebKit/Source/core/html/HTMLLinkElement.h:
While running git apply --index -p1;
  error: patch failed: third_party/WebKit/Source/core/html/HTMLLinkElement.h:120
  error: third_party/WebKit/Source/core/html/HTMLLinkElement.h: patch does not
apply

Patch:       third_party/WebKit/Source/core/html/HTMLLinkElement.h
Index: third_party/WebKit/Source/core/html/HTMLLinkElement.h
diff --git a/third_party/WebKit/Source/core/html/HTMLLinkElement.h
b/third_party/WebKit/Source/core/html/HTMLLinkElement.h
index
22351de853c80fea82c1c8c45258221d26943d48..b79f54789512cc2f8a1ab78954e06a250ee24d75
100644
--- a/third_party/WebKit/Source/core/html/HTMLLinkElement.h
+++ b/third_party/WebKit/Source/core/html/HTMLLinkElement.h
@@ -88,7 +88,6 @@ class CORE_EXPORT HTMLLinkElement final : public HTMLElement,
 
   DOMTokenList* sizes() const;
 
-  void dispatchPendingEvent(std::unique_ptr<IncrementLoadEventDelayCount>);
   void scheduleEvent();
 
   // From LinkLoaderClient
@@ -120,6 +119,10 @@ class CORE_EXPORT HTMLLinkElement final : public
HTMLElement,
   void process();
   static void processCallback(Node*);
 
+  // Always call this asynchronously because this can cause synchronous
+  // Document load event and JavaScript execution.
+  void dispatchPendingEvent(std::unique_ptr<IncrementLoadEventDelayCount>);
+
   // From Node and subclassses
   void parseAttribute(const QualifiedName&,
                       const AtomicString&,

[hiroshige, 2017-01-20 01:26:01]

The CQ bit was checked by hiroshige@chromium.org

[commit-bot: I haz the power, 2017-01-20 01:26:54]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-20 01:34:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-20 01:34:40]

Failed to apply patch for third_party/WebKit/Source/core/html/HTMLLinkElement.h:
While running git apply --index -p1;
  error: patch failed: third_party/WebKit/Source/core/html/HTMLLinkElement.h:120
  error: third_party/WebKit/Source/core/html/HTMLLinkElement.h: patch does not
apply

Patch:       third_party/WebKit/Source/core/html/HTMLLinkElement.h
Index: third_party/WebKit/Source/core/html/HTMLLinkElement.h
diff --git a/third_party/WebKit/Source/core/html/HTMLLinkElement.h
b/third_party/WebKit/Source/core/html/HTMLLinkElement.h
index
22351de853c80fea82c1c8c45258221d26943d48..b79f54789512cc2f8a1ab78954e06a250ee24d75
100644
--- a/third_party/WebKit/Source/core/html/HTMLLinkElement.h
+++ b/third_party/WebKit/Source/core/html/HTMLLinkElement.h
@@ -88,7 +88,6 @@ class CORE_EXPORT HTMLLinkElement final : public HTMLElement,
 
   DOMTokenList* sizes() const;
 
-  void dispatchPendingEvent(std::unique_ptr<IncrementLoadEventDelayCount>);
   void scheduleEvent();
 
   // From LinkLoaderClient
@@ -120,6 +119,10 @@ class CORE_EXPORT HTMLLinkElement final : public
HTMLElement,
   void process();
   static void processCallback(Node*);
 
+  // Always call this asynchronously because this can cause synchronous
+  // Document load event and JavaScript execution.
+  void dispatchPendingEvent(std::unique_ptr<IncrementLoadEventDelayCount>);
+
   // From Node and subclassses
   void parseAttribute(const QualifiedName&,
                       const AtomicString&,

[hiroshige, 2017-01-20 01:38:04]

The CQ bit was checked by hiroshige@chromium.org

[commit-bot: I haz the power, 2017-01-20 01:38:35]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-20 01:45:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-20 01:45:44]

Failed to apply patch for third_party/WebKit/Source/core/html/HTMLLinkElement.h:
While running git apply --index -p1;
  error: patch failed: third_party/WebKit/Source/core/html/HTMLLinkElement.h:120
  error: third_party/WebKit/Source/core/html/HTMLLinkElement.h: patch does not
apply

Patch:       third_party/WebKit/Source/core/html/HTMLLinkElement.h
Index: third_party/WebKit/Source/core/html/HTMLLinkElement.h
diff --git a/third_party/WebKit/Source/core/html/HTMLLinkElement.h
b/third_party/WebKit/Source/core/html/HTMLLinkElement.h
index
22351de853c80fea82c1c8c45258221d26943d48..b79f54789512cc2f8a1ab78954e06a250ee24d75
100644
--- a/third_party/WebKit/Source/core/html/HTMLLinkElement.h
+++ b/third_party/WebKit/Source/core/html/HTMLLinkElement.h
@@ -88,7 +88,6 @@ class CORE_EXPORT HTMLLinkElement final : public HTMLElement,
 
   DOMTokenList* sizes() const;
 
-  void dispatchPendingEvent(std::unique_ptr<IncrementLoadEventDelayCount>);
   void scheduleEvent();
 
   // From LinkLoaderClient
@@ -120,6 +119,10 @@ class CORE_EXPORT HTMLLinkElement final : public
HTMLElement,
   void process();
   static void processCallback(Node*);
 
+  // Always call this asynchronously because this can cause synchronous
+  // Document load event and JavaScript execution.
+  void dispatchPendingEvent(std::unique_ptr<IncrementLoadEventDelayCount>);
+
   // From Node and subclassses
   void parseAttribute(const QualifiedName&,
                       const AtomicString&,

[hiroshige, 2017-01-20 01:48:17]

The CQ bit was checked by hiroshige@chromium.org

[commit-bot: I haz the power, 2017-01-20 01:49:06]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hiroshige, 2017-01-20 01:49:26]

The CQ bit was checked by hiroshige@chromium.org

[hiroshige, 2017-01-20 01:49:26]

The patchset sent to the CQ was uploaded after l-g-t-m from japhet@chromium.org,
yhirano@chromium.org
Link to the patchset: https://codereview.chromium.org/2391923002/#ps180001
(title: "Rebase")

[commit-bot: I haz the power, 2017-01-20 01:50:23]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-20 03:18:36]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1484876966147720,
"parent_rev": "fbd10854de98cb409beb7fe940f458539555b2e0", "commit_rev":
"46649a2f0bf126330d95f47b6c8773823451d23f"}

[commit-bot: I haz the power, 2017-01-20 03:19:06]

Description was changed from

==========
Call checkCompleted() synchronously in HTMLLinkElement::dispatchPendingEvent()

Previously, there was an asynchronous hop from <link>'s load event to
document's load event, because HTMLLinkElement::dispatchPendingEvent()
scheduled checkCompleted() asynchronously via IncrementLoadEventDelayCount.

This CL makes HTMLLinkElement::dispatchPendingEvent() to call checkCompleted()
synchronously, by using
IncrementLoadEventDelayCount::clearAndCheckLoadEvent().

Because checkCompleted() is called at the end of asynchronously called
HTMLLinkElement::dispatchPendingEvent(), this CL doesn't increase synchronous
timing dependencies around Document load events.

This CL fixes the performance regression of smoothness.top_25_smooth on
Mac 10.10 caused by https://codereview.chromium.org/2275493002/.

BUG=648887, 624697
==========

to

==========
Call checkCompleted() synchronously in HTMLLinkElement::dispatchPendingEvent()

Previously, there was an asynchronous hop from <link>'s load event to
document's load event, because HTMLLinkElement::dispatchPendingEvent()
scheduled checkCompleted() asynchronously via IncrementLoadEventDelayCount.

This CL makes HTMLLinkElement::dispatchPendingEvent() to call checkCompleted()
synchronously, by using
IncrementLoadEventDelayCount::clearAndCheckLoadEvent().

Because checkCompleted() is called at the end of asynchronously called
HTMLLinkElement::dispatchPendingEvent(), this CL doesn't increase synchronous
timing dependencies around Document load events.

This CL fixes the performance regression of smoothness.top_25_smooth on
Mac 10.10 caused by https://codereview.chromium.org/2275493002/.

BUG=648887, 624697

Review-Url: https://codereview.chromium.org/2391923002
Cr-Commit-Position: refs/heads/master@{#444965}
Committed:
https://chromium.googlesource.com/chromium/src/+/46649a2f0bf126330d95f47b6c87...
==========

[commit-bot: I haz the power, 2017-01-20 03:19:07]

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/46649a2f0bf126330d95f47b6c87...