Daniel Bratell Assignee
	Description • 22 years ago

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.

	Daniel Bratell Assignee
	Comment 1 • 22 years ago

Taking bug

	Daniel Bratell Assignee
	Comment 2 • 22 years ago

Looking for some r= and sr=. jst?

	Johnny Stenback (: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.

	Daniel Bratell Assignee
	Comment 4 • 22 years ago

How is proper caching done?

	Boris Zbarsky [:bzbarsky]
	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).

	Daniel Bratell 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?

	Peter Van der Beken [:peterv]
	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.

	Boris Zbarsky [:bzbarsky]
	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

	Johnny Stenback (:jst)
	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.

	Daniel Bratell 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.

	Boris Zbarsky [:bzbarsky]
	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.

	Johnny Stenback (:jst)
	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.

	Daniel Bratell 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.

	Boris Zbarsky [:bzbarsky]
	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....

	Daniel Bratell 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.

	Boris Zbarsky [:bzbarsky]
	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.

	Daniel Bratell 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.

	Boris Zbarsky [:bzbarsky]
	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.

	Daniel Bratell 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?

	Boris Zbarsky [:bzbarsky]
	Comment 21 • 22 years ago

Yes.  ;)

	Johnny Stenback (:jst)
	Comment 22 • 22 years ago

Comment on attachment 106832 [details] [diff] [review]
Convert content over to using the new utility function

r/sr=jst

	Daniel Bratell 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.

	Boris Zbarsky [:bzbarsky]
	Comment 25 • 22 years ago

Comment on attachment 107000 [details] [diff] [review]
No "middle cache" in nsDocumentEncoder as by discussion.

looks great!

	Daniel Bratell 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.

	Boris Zbarsky [:bzbarsky]
	Comment 27 • 22 years ago

There's a lot of inlining going on (of the do_GetService calls, etc, etc).