Closed Bug 482919 Opened 13 years ago Closed 12 years ago

[HTML5] Add speculative parsing to the HTML5 parser

Categories

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

Other Branch
defect

Tracking

()

RESOLVED FIXED

People

(Reporter: hsivonen, Assigned: hsivonen)

References

Details

Attachments

(1 file, 4 obsolete files)

When a script loads synchronously, the HTML5 parser should speculatively start parsing the rest of the network stream.
Priority: -- → P1
Depends on: 483158
Duplicate of this bug: 503292
Blocks: 515610
Attached patch First iteration (obsolete) — Splinter Review
Blocks: 521764
Attached patch Refresh patch (obsolete) — Splinter Review
Attachment #405479 - Attachment is obsolete: true
Attachment #406449 - Attachment is obsolete: true
Attachment #406988 - Attachment is obsolete: true
Attachment #407026 - Flags: review?(bnewman)
Status: NEW → ASSIGNED
+// Not using macros for AddRef and Release in order to be able to refcount on
+// and create on different threads.
+
+nsrefcnt
+nsHtml5UTF16Buffer::AddRef()
+{

What's wrong with NS_IMPL_THREADSAFE_{AddRef,Release}?
(In reply to comment #7)
> +// Not using macros for AddRef and Release in order to be able to refcount on
> +// and create on different threads.
> +
> +nsrefcnt
> +nsHtml5UTF16Buffer::AddRef()
> +{
> 
> What's wrong with NS_IMPL_THREADSAFE_{AddRef,Release}?

This object doesn't really need refcounting from multiple threads concurrently. Therefore, NS_IMPL_THREADSAFE_{AddRef,Release} would add an unnecessary atomic operation. I wanted to avoid adding an unnecessary atomic operation, because I thought it would be bad for CPU caches on multicores.

The thread-unsafe macro variants don't work, because they want the object to have a single owner thread. Therefore, they prevent the hand-off of objects between threads. It seems to me that letting the buffer objects assert about their owner thread and going in an manually changing the owner thread as appropriate would be an overkill, since the buffers are owner by nsHtml5StreamParser and protected by mTokenizerMutex and the parser would already be horribly broken if mTokenizerMutex weren't doing the right thing or used right.
I'm seeing a crash on tp4. I've tried sending various printfs to talos to figure out what's going wrong, but I have failed to get useful data. I suggest dealing with the tp4 crash as a follow-up bug, since this code doesn't run on talos by default.
Attachment #407026 - Attachment is obsolete: true
Attachment #408855 - Flags: review?(bnewman)
Attachment #407026 - Flags: review?(bnewman)
Comment on attachment 408855 [details] [diff] [review]
Remove printfs, make script running tree op initialization type-safe

Patch makes sense to me.

Definitely file a followup bug for the crash.  I think I might be seeing the same thing, and it does not appear related to speculative parsing.
Attachment #408855 - Flags: review?(bnewman) → review+
Thank you for the review.

Landed as http://hg.mozilla.org/mozilla-central/rev/e04af661ed40
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Depends on: 645998
Landed followup to fix no-newline-at-end-of-file build warning on maemo (spammed for every .cpp file that includes nsHtml5Speculation.h):
  http://hg.mozilla.org/integration/mozilla-inbound/rev/94f56bd00f54
sample of warning:
{
In file included from /builds/slave/m-in-lnx-maemo5-gtk/build/parser/html/nsHtml5StreamParser.h:54,
                 from /builds/slave/m-in-lnx-maemo5-gtk/build/parser/html/nsHtml5Parser.h:61,
                 from /builds/slave/m-in-lnx-maemo5-gtk/build/parser/html/nsHtml5TreeBuilder.h:46,
                 from /builds/slave/m-in-lnx-maemo5-gtk/build/parser/html/nsHtml5AttributeName.cpp:50:
/builds/slave/m-in-lnx-maemo5-gtk/build/parser/html/nsHtml5Speculation.h:103:33: warning: no newline at end of file
}
http://tinderbox.mozilla.org/showlog.cgi?log=Mozilla-Inbound/1312310251.1312310578.31642.gz

(noticed this warning when pushing a bustage fix for bug 675499)
You need to log in before you can comment on or make changes to this bug.