Closed
Bug 165893
Opened 22 years ago
Closed 22 years ago
Cache parser service in NS_CreateHTMLElement to avoid looking it up every tim
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla1.3alpha
People
(Reporter: bratell, Assigned: bratell)
References
Details
(Keywords: perf)
Attachments
(2 files, 3 obsolete files)
4.38 KB,
patch
|
jst
:
review+
bzbarsky
:
superreview+
|
Details | Diff | Splinter Review |
17.92 KB,
patch
|
bzbarsky
:
superreview+
|
Details | Diff | Splinter Review |
Every time we create an element we refetch the parser service which costs us some. We can cache it which for instance saves us ~4% of the time in createElement testcase in bug 118933. Patch coming up.
Assignee | ||
Comment 1•22 years ago
|
||
Taking bug
Assignee | ||
Comment 2•22 years ago
|
||
Looking for some r= and sr=. jst?
Comment 3•22 years ago
|
||
Comment on attachment 97390 [details] [diff] [review] Cache the parser service No static nsCOMPtr's please, if you want to do caching, do the proper cleanup at shutdown too, we don't want to leak the parser service, even if it's a singleton.
Attachment #97390 -
Flags: needs-work+
Assignee | ||
Comment 4•22 years ago
|
||
How is proper caching done?
![]() |
||
Comment 5•22 years ago
|
||
Use a static global nsIParserService* which you NS_IF_RELEASE at shutdown. See Shutdown() in nsContentModule.cpp and the various shutdown functions it calls (eg nsBaseContentList::Shutdown).
Updated•22 years ago
|
Keywords: mozilla1.2
Assignee | ||
Comment 6•22 years ago
|
||
Here is a new patch that uses a static variable to cache the parser service and a Shutdown method to release it during shutdown. The only thing I'm not sure about is the thread safety. Do I have to protect it with a lock. Also, the mac build? I put the helper class in a new file. Do I have to add something to a mac project? jst, can you review this?
Attachment #97390 -
Attachment is obsolete: true
Assignee | ||
Updated•22 years ago
|
Status: NEW → ASSIGNED
Target Milestone: mozilla1.2alpha → mozilla1.2beta
Assignee | ||
Updated•22 years ago
|
Attachment #100132 -
Flags: superreview?(jst)
Attachment #100132 -
Flags: review?(jst)
Comment 7•22 years ago
|
||
Do we really want to add a new file? Can't you just put it in nsContentUtils? Looks like there's other uses of the parser service in content.
Assignee | ||
Comment 8•22 years ago
|
||
Attachment #100132 -
Attachment is obsolete: true
Assignee | ||
Updated•22 years ago
|
Attachment #106363 -
Flags: superreview?(jst)
Attachment #106363 -
Flags: review?(peterv)
Assignee | ||
Updated•22 years ago
|
Attachment #100132 -
Flags: superreview?(jst)
Attachment #100132 -
Flags: review?(jst)
![]() |
||
Comment 9•22 years ago
|
||
Comment on attachment 106363 [details] [diff] [review] Integrated code into nsContentUtils instead of creating new class + // safe för multiple threads. English, please? ;) With that comment change, and removal of the unnecessary kParserServiceCID from the HTML sink, sr=bzbarsky
Attachment #106363 -
Flags: superreview?(jst) → superreview+
Comment 10•22 years ago
|
||
Comment on attachment 106363 [details] [diff] [review] Integrated code into nsContentUtils instead of creating new class IMO, it's not cool to return weak XPCOM pointer's from a method w/o making it obvious in the method name that they're weak pointers. IOW, rename GetCachedParserService() to something like GetParserServiceWeakRef() or somesuch. IMO there's no need to have the cached word in the name, it's a service, it's cached somewhere by definition. r=jst with that change.
Attachment #106363 -
Flags: review?(peterv) → review+
Assignee | ||
Comment 11•22 years ago
|
||
I've done the requested changes and checked in. I'm not completely sure if I know the definition of a weak pointer in this case. A weak pointer is one that's not reference counted, isn't it? But this pointer is reference counted in nsContentUtils. Or I guess it could be a signal to the users of the API that they shouldn't reference count the pointer they get. Anyway, I just did a grep in content and found some other users of the parser service. Is there a point in changing them to use the new static pointer? content/base/src/nsDocumentEncoder.cpp: nsHTMLCopyEncoder::Init content/base/src/nsHTMLContentSerializer.cpp nsHTMLContentSerializer::AppendElementEnd nsHTMLContentSerializer::AppendToString nsHTMLContentSerializer::LineBreakBeforeOpen nsHTMLContentSerializer::LineBreakAfterClose content/base/src/nsPlainTextSerializer.cpp: nsPlainTextSerializer::GetIdForContent nsPlainTextSerializer::DoAddLeaf nsPlainTextSerializer::IsBlockLevel nsPlainTextSerializer::IsContainer content/base/src/mozSanitizingSerializer.cpp: mozSanitizingSerializer::GetIdForContent mozSanitizingSerializer::IsContainer mozSanitizingSerializer::DoOpenContainer mozSanitizingHTMLSerializer::DoCloseContainer mozSanitizingHTMLSerializer::ParsePrefs mozSanitizingHTMLSerializer::ParseTagPref content/html/document/src/nsHTMLFragmentContentSink.cpp: nsHTMLFragmentContentSink::OpenContainer nsHTMLFragmentContentSink::AddLeaf That's quite a few callers during a processing of a page. I've not profiled anything though and I'm still not sure if threads safety can be an issue. Someone more familiar with |content|, please tell me if I should convert these to use nsContentUtils::GetParserServiceWeakRef(). It would reduce footprint and CPU use, but I don't know how much.
![]() |
||
Comment 12•22 years ago
|
||
The encoder and serializers are only used when converting from DOM to HTML. Typical applications are "copy", for example. The one exception to that that may be worth testing/optimizing is innerHTML access which uses a serializer in the getter and the fragment sink in the setter.
Comment 13•22 years ago
|
||
If it's easy enough to change all users of the parser service to use this new cache then I'd say it's worht doing for code size reduction if nothing else.
Assignee | ||
Comment 14•22 years ago
|
||
It was not hard at all to convert the rest of content. This one should be easy to review. Just mechanical changes. The benefit is mostly footprint and consistancy within content.
Assignee | ||
Updated•22 years ago
|
Attachment #106832 -
Flags: superreview?(bzbarsky)
Attachment #106832 -
Flags: review?(jst)
![]() |
||
Comment 15•22 years ago
|
||
In base/src/nsDocumentEncoder.cpp
> + nsIParserService* mParserService; // Weak ref
I would be happier if this were left an nsCOMPtr, myself....
Assignee | ||
Comment 16•22 years ago
|
||
Why? I would have removed the cached ParserService there completely in favor of the cache in nsContentUtils of it wasn't for the error handling. Now all error handling is in init, and if I had removed the local pointer the error handling would have to be in the middle of deeply nested functions. I can do it an nsCOMPtr<nsIParserService> mParserService = dont_AddRef(...) but I don't see the point since that only adds overhead to accessing the pointer.
![]() |
||
Comment 17•22 years ago
|
||
Actually, if you did that with the dont_AddRef that would be wrong and crash. ;) I guess I'm paranoid that this code will try to run after Shutdown() is called for whatever reason and will access a stale pointer because the object has gone away.
Assignee | ||
Comment 18•22 years ago
|
||
You must be the judge of that risk. I don't know enough about how the system collapses during shutdown. I haven't seen any crashes but if it's possible the probability is extremely low since Shutdown sets the pointer to 0 so there can only be a stale pointer for a very short time. That is if it's possible at all.
![]() |
||
Comment 19•22 years ago
|
||
> since Shutdown sets the pointer to 0
It doesn't set the member pointer in nsHTMLCopyEncoder.... Here is the scenario
that I don't like:
1) nsHTMLCopyEncoder gets a pointer to the parser service (weak ref) and stores
it in mParserService
2) Shutdown starts
3) Shutdown releases the cached ref to the parser service, triggering its
destructor
4) nsHTMLCopyEncoder tries to do something to mParserService and jumps to
lala-land.
Granted, there should probably be no nsHTMLCopyEncoder objects hanging around at
this stage of shutdown... but note the probably.
Assignee | ||
Comment 20•22 years ago
|
||
Would removing the member variable in favor of getting the service in the actual methods using peace your mind?
![]() |
||
Comment 21•22 years ago
|
||
Yes. ;)
Comment 22•22 years ago
|
||
Comment on attachment 106832 [details] [diff] [review] Convert content over to using the new utility function r/sr=jst
Attachment #106832 -
Flags: review?(jst) → review+
Assignee | ||
Comment 23•22 years ago
|
||
Attachment #106832 -
Attachment is obsolete: true
Assignee | ||
Comment 24•22 years ago
|
||
Comment on attachment 107000 [details] [diff] [review] No "middle cache" in nsDocumentEncoder as by discussion. Boris, here's a new patch for review.
Attachment #107000 -
Flags: superreview?(bzbarsky)
![]() |
||
Comment 25•22 years ago
|
||
Comment on attachment 107000 [details] [diff] [review] No "middle cache" in nsDocumentEncoder as by discussion. looks great!
Attachment #107000 -
Flags: superreview?(bzbarsky) → superreview+
Assignee | ||
Comment 26•22 years ago
|
||
Fix checked in. The 'Z' number went down a couple of KB. I'm surprised that nsCOMPtrs generate that much more code than normal pointers.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Target Milestone: mozilla1.2beta → mozilla1.3alpha
![]() |
||
Comment 27•22 years ago
|
||
There's a lot of inlining going on (of the do_GetService calls, etc, etc).
![]() |
||
Updated•22 years ago
|
Attachment #106832 -
Flags: superreview?(bzbarsky)
Component: DOM: Core → DOM: Core & HTML
QA Contact: stummala → general
You need to log in
before you can comment on or make changes to this bug.
Description
•