Closed Bug 539215 Opened 10 years ago Closed 10 years ago

[HTML5] When a tree op flush doesn't need to run to completion, make op flush yield depending on the wall clock


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






(Reporter: hsivonen, Assigned: hsivonen)




(1 file, 1 obsolete file)

Currently, nsHtml5TreeOpExecuter::Flush() always runs to completion (empties the op queue). This is not desirable when the op queue is very long. 

document.write() semantics require Flush() to run to completion. However, parsing the network stream doesn't. When run to completion isn't required, the Flush() method should make a guess of how many ops it can run in some constant amount of time and execute at most that many ops. If there are more ops, a runnable for calling Flush() again should be posted.
Blocks: 539428
Assignee: nobody → hsivonen
Blocks: 508456
 * It seems to me that sampling the clock during the flush loop would be an overkill.
 * When the limit is computed from past performance, it's necessary to put a hard limit on the number of ops to avoid the first flush after a slow-down taking too much time.
 * The hard limit is chosen so that most tp4 pages don't hit the hard limit given the structure of the pages.
 * With the hard limit, on a 2.4 GHz Core Duo, I no longer see the time limit hit at all. It's possible that with the hard limit, the time limit is useless. I left it in, just in case, so that all this can be tuned if necessary without readding the code.
 * Bug 508456 is fixed by this patch.
 * Loading works more nicely with this patch.
Attachment #421622 - Attachment is obsolete: true
Attachment #423497 - Flags: review?(bnewman)
Comment on attachment 423497 [details] [diff] [review]
Make tree op executor put a cap on the number of tree ops flushed without spinning the event loop

Looks good to me. Nice to have the tunability.

I'll just mention that you can do without the opQueueLength local variable in nsHtml5TreeOpExecutor::Flush, if you set numberOfOpsToFlush = mOpQueue.Length() to begin with.
Attachment #423497 - Flags: review?(bnewman) → review+
Thanks. Pushed with the suggested change:
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.