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)
Tracking
()
RESOLVED
FIXED
mozilla1.8alpha1
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
Details
(Keywords: perf)
Attachments
(1 file, 2 obsolete files)
4.38 KB,
patch
|
Details | Diff | Splinter Review |
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 ;) ).
Assignee | ||
Comment 1•20 years ago
|
||
Assignee | ||
Comment 2•20 years ago
|
||
Attachment #147861 -
Attachment is obsolete: true
Assignee | ||
Updated•20 years ago
|
Attachment #147862 -
Flags: superreview?(roc)
Attachment #147862 -
Flags: review?(peterv)
Assignee | ||
Updated•20 years ago
|
Priority: -- → P1
Summary: timer munging slows down document.write → [FIX]timer munging slows down document.write
Target Milestone: --- → mozilla1.8alpha
Comment 3•20 years ago
|
||
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+
Assignee | ||
Comment 4•20 years ago
|
||
(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.
Assignee | ||
Updated•20 years ago
|
Attachment #147862 -
Flags: superreview?(roc) → superreview?(jst)
Comment 5•20 years ago
|
||
Comment on attachment 147862 [details] [diff] [review] Oops, wrong file. sr=jst
Attachment #147862 -
Flags: superreview?(jst) → superreview+
Assignee | ||
Comment 6•20 years ago
|
||
Attachment #147862 -
Attachment is obsolete: true
Assignee | ||
Comment 7•20 years ago
|
||
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.
Description
•