Closed Bug 242899 Opened 20 years ago Closed 20 years ago

[FIX]timer munging slows down document.write

Categories

(Core :: DOM: Core & HTML, defect, P1)

x86
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla1.8alpha1

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

(Keywords: perf)

Attachments

(1 file, 2 obsolete files)

The short story: at the start of every document.write we cancel a notification
timer in the HTML sink and at the end we reinit it.  This is about 20% of
document.write time.

Long story:  I was profiling the testcases in bug 23187 -- thousands of tiny
document.write calls.  HTMLContentSink::WillResume cancels the notification
timer, if any, and HTMLContentSink::WillInterrupt sets one if there isn't one. 
Both are called for every document.write.  This timer-munging is rather
expensive on Linux (involving calls to kill() in the threading library and such).

Frankly, all the timer stuff seems over-complicated, but for this particular
issue my proposed course of action is simply:

1)  Don't cancel timers in WillResume; set a "parsing" flag instead
2)  If Notify() is called while "parsing" is set, ignore it and set a "missed
    timer" flag.
3)  When WillInterrupt is called (either out of data or DidProcessAToken called
    it normally), check the "missed timer" flag and do what we would have done
    in Notify.  If there is no timer set, set it.  Unset "parsing" flag.

This should keep behavior on "normal" pages about the same, but keep us from
thrashing the timers.  It makes us about 20-25% faster on those testcases on
Linux (so about 2-3 times slower than Opera ;) ).
Attached patch Something like this (obsolete) — Splinter Review
Attached patch Oops, wrong file. (obsolete) — Splinter Review
Attachment #147861 - Attachment is obsolete: true
Attachment #147862 - Flags: superreview?(roc)
Attachment #147862 - Flags: review?(peterv)
Priority: -- → P1
Summary: timer munging slows down document.write → [FIX]timer munging slows down document.write
Target Milestone: --- → mozilla1.8alpha
Comment on attachment 147862 [details] [diff] [review]
Oops, wrong file.

>Index: content/html/document/src/nsHTMLContentSink.cpp
>===================================================================

>@@ -2450,93 +2486,76 @@ HTMLContentSink::WillInterrupt()

>+      nsInt64 now(PR_Now());
>+      PRInt32 delay = GetNotificationInterval();
>+      nsInt64 interval(delay);

I think you can just do

    nsInt64 interval(GetNotificationInterval());

>+      nsInt64 lastNotification(mLastNotificationTime);
>+      nsInt64 diff(now - lastNotification);
> 
>       // If it's already time for us to have a notification
>-      if (LL_CMP(diff, >, interval)) {
>+      if (diff > interval || mFlags && NS_SINK_FLAG_DROPPED_TIMER) {

This should be || mFlags & NS_SINK_FLAG_DROPPED_TIMER, right?

>+      } else if (!mNotificationTimer) {
>+        interval -= diff;
>+        delay = interval;

and declare delay here:

    PRInt32 delay(interval);

I doesn't seem to be used outside this block.
Attachment #147862 - Flags: review?(peterv) → review+
(In reply to comment #3)
>     nsInt64 interval(GetNotificationInterval());

Indeed.  I used to use delay outside that block in an early iteration of the
patch...

> >+      if (diff > interval || mFlags && NS_SINK_FLAG_DROPPED_TIMER) {
> 
> This should be || mFlags & NS_SINK_FLAG_DROPPED_TIMER, right?

Absolutely!   And with that change, the 20-25% number in comment 0 becomes 50%.
 Down to a factor of 1.5-2 behind Opera.  ;)

Thank you for catching that.
Attachment #147862 - Flags: superreview?(roc) → superreview?(jst)
Comment on attachment 147862 [details] [diff] [review]
Oops, wrong file.

sr=jst
Attachment #147862 - Flags: superreview?(jst) → superreview+
Attachment #147862 - Attachment is obsolete: true
Checked in.
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: