96364 - CObserverService::RegisterObservers() called 400 times on startup

Status: VERIFIED FIXED

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

Description

[Suresh Duddi (gone)]

nsDTDUtils.cpp

CObserverService::RegisterObservers() is called about 400 times on startup.

It might be worth rethinking if we need to maintain all that data structure and
kill it just to do MetaTag notification. Registering directly with the some
parser observer service might be better.

Comment 1

[harishd]

Attachment: Patch v1.0 [ Note: This is not the final patch ]

Comment 2

[harishd]

Attachment: patch v1.1 [ Needs commenting, MLK / QFY runs ]

Comment 3

[Suresh Duddi (gone)]

Suggestion: I am not very particular about the names. You can do it if you think
this makes the code more readable.

* nsIContentSink::NotifyObservers -> NotifyTagObservers()
  Seems like a more intutive name.

* nsIElementObserver::Notify() -> NotifyTagObservers() or something like that

* nsIObserverEntry::GetObserverInterface() -> GetTopicObservers() ?

* COtherDTD.cpp
  +  static NS_DEFINE_IID(kParserServiceCID, NS_PARSERSERVICE_CID);
  why is this there ?

* nsObserverEntry::AddObserver()
  Why nsVoidArray(0) ? How about using nsAutoVoidArray ? mostly there are going
  to be less than 4 observers per topic right ? AutoVoidArray can save the
  allocs at the cost of memory.

* Add/Remove Observer() in nsObserverEntry() take nsISupports *
  Why ? Would it make sense to have them take an nsIElementObserver so you
  wont have to NS_STATIC_CAST() in your nsObserverEntry::Notify() method. Is
  there any specific benefit to keeping it nsISupports* ?

* Using voidArray * in nsObserverEntry::mObservers is good.

* If the number of people doing nsIParserService::RegisterObserver() is less and
  the number of Topics is less (4 right ?) I think not using a hash table is
  the right way.

* nsViewSourceHTML.cpp
  +static NS_DEFINE_IID(kParserServiceCID, NS_PARSERSERVICE_CID);
  do we need this ?

* I didnt' review the intl/

Comment 4

[harishd]

>I am not very particular about the names. You can do it if you think
>this makes the code more readable.

In fact I was thinking of renaming GetObserverInterface() to something else. But
, Notify() -> NotifyTagObservers() did not strike me. Though, NotifyTagObservers
is more intuitive. Will make that change.

>COtherDTD.cpp
> static NS_DEFINE_IID(kParserServiceCID, NS_PARSERSERVICE_CID);
> nsViewSourceHTML.cpp
> static NS_DEFINE_IID(kParserServiceCID, NS_PARSERSERVICE_CID);
> do we need this ?

We don't need these. I'll have to comment the code and will do the clean up then.

>nsObserverEntry::AddObserver()
> Why nsVoidArray(0) ? How about using nsAutoVoidArray ? mostly there are going
> to be less than 4 observers per topic right ? AutoVoidArray can save the
> allocs at the cost of memory.

Will replace nsVoidArray with nsAutoVoidArray.

>Add/Remove Observer() in nsObserverEntry() take nsISupports *
> Why ? Would it make sense to have them take an nsIElementObserver so you
> wont have to NS_STATIC_CAST() in your nsObserverEntry::Notify() method. Is
> there any specific benefit to keeping it nsISupports* ?

You're right. I was only particular in keeping parameters as nsISupports* at the
interface level. 

> If the number of people doing nsIParserService::RegisterObserver() is less and
> the number of Topics is less (4 right ?) I think not using a hash table is
> the right way.

Topics == mime type. There are lots of mime types. However, this mechanism is
primarily for "text/html", "text/xhtml", and "application/xhtml+xml" ( i.e.
flavors of html ).

Note: This mechanism would not work for xml documents because there are no
predefined tags in xml and therefore would require string comparisons
(expensive), and change in data structure, to work correctly. 

Comment 5

[harishd]

>Add/Remove Observer() in nsObserverEntry() take nsISupports *
> Why ? Would it make sense to have them take an nsIElementObserver so you
> wont have to NS_STATIC_CAST() in your nsObserverEntry::Notify() method. Is
> there any specific benefit to keeping it nsISupports* ?

I just realized that one cannot avoid casting when using nsVoidArray because the
elements are void*.

Comment 6

[Suresh Duddi (gone)]

Ah yes.

nsISupports* -> void* -> nsIElementObserver*
This using STATIC_CAST() might be buggy - depending on where in the inheritance
hierarchy nsIElementObserver is. To get this right you would have to do:
nsISupports -> void* -> nsISupports* -QI()-> nsIElementObserver*

nsIElementObserver* -> void* -> nsIElementObserver*
This is ok using STATIC_CAST().

Comment 7

[harishd]

QFY Run with my patch:
----------------------
Function     :	nsParserService::RegisterObserver
Calls        :	1
Function time:	0.09 usec (0.00% of Focus)
F+D time     :	49.00 usec (0.00% of Focus)
Avg F time   :	0.09 usec
Min F time   :	0.09 usec
Max F time   :	0.09 usec
Measurement  :	Function
-------------------------------------------------
Function     :nsObserverEntry::Notify
Calls        :166
Function time:12.05 usec (0.00% of Focus)
F+D time     :318.44 usec (0.00% of Focus)
Avg F time   :0.07 usec
Min F time   :0.07 usec
Max F time   :0.65 usece)
Measurement  :Function
--------------------------------------------------
Function     :HTMLContentSink::NotifyObservers
Calls        :166
Function time:5.02 usec (0.00% of Focus)
F+D time     :323.46 usec (0.00% of Focus)
Avg F time   :0.03 usec
Min F time   :0.03 usec
Max F time   :0.03 usec
Measurement  :Function
---------------------------------------------------
Function     :nsParserService::GetEntry
Calls        :4
Function time:0.22 usec (0.00% of Focus)
F+D time     :2.18 usec (0.00% of Focus)
Avg F time   :0.06 usec
Min F time   :0.05 usec
Max F time   :0.06 usec
Measurement  :Function
----------------------------------------------------
Function     :	nsParserService::GetObserverInterface
Calls        :	3
Function time:	0.09 usec (0.00% of Focus)
F+D time     :	2.23 usec (0.00% of Focus)
Avg F time   :	0.03 usec
Min F time   :	0.03 usec
Max F time   :	0.03 usec
Measurement  :	Function
-----------------------------------------------------

Comment 8

[harishd]

Attachment: Purify run [ No leaks found ]

Comment 9

[harishd]

Attachment: patch v1.2 [ includes dp's suggestions & a few editor code changes]

Comment 10

[harishd]

Calling Notify() from CNavDTD::WillHandleStartTag() with the patch:
--------------------------------------------------------------------

Function     :CNavDTD::WillHandleStartTag
Calls        :223
Function time:22.87 usec (0.00% of Focus)
F+D time     :488.88 usec (0.00% of Focus) <<<<< NOTICE THIS <<<<< 
Avg F time   :0.10 usec
Min F time   :0.09 usec
Max F time   :0.16 usec
Measurement  :Function

Calling Notify() from CNavDTD::WillHandleStartTag() without the patch:
-----------------------------------------------------------------------

Function     :CNavDTD::WillHandleStartTag
Calls        :231
Function time:29.40 usec (0.00% of Focus)
F+D time     :58,508.89 usec (0.00% of Focus)  <<<<< NOTICE THIS <<<<< 
Avg F time   :0.13 usec
Min F time   :0.11 usec
Max F time   :0.18 usec
Measurement  :Function

Comment 11

[Suresh Duddi (gone)]

Comment on attachment 50293 [details] [diff] [review]
patch v1.2 [ includes dp's suggestions & a few editor code changes]

r=dp

Comment 12

[rpotts (gone)]

Comment on attachment 50293 [details] [diff] [review]
patch v1.2 [ includes dp's suggestions & a few editor code changes]

sr=[e-mail]

Comment 13

[Shanjian Li]

Comment on attachment 50293 [details] [diff] [review]
patch v1.2 [ includes dp's suggestions & a few editor code changes]

r=shanjian (for i18n related change)

Comment 14

[Simon Fraser [no longer active]]

Comments:

+  // Observer mechanism
+  NS_IMETHOD RegisterObserver(const nsString& aTopic,
+                              nsIElementObserver* aObserver,
+                              const eHTMLTags* aTags = nsnull) = 0;

nsAString is the new, better string way, no?

Here:

+      PRInt32 theAttrCount =aNode->GetAttributeCount(); 
+      PRInt32 theObserversCount=theObservers->Count(); 
+      if(0<theObserversCount){

and elsewhere, you're using non-standard spacing. Convention is spaces around '=
', after ',', around '<' etc.

+}
\ No newline at end of file

fix this.

+nsParserService::~nsParserService()
+{
+  nsObserverEntry *entry = nsnull;
+  while(entry = NS_STATIC_CAST(nsObserverEntry*,mEntries.Pop())) {
+    NS_RELEASE(entry);
+  }
+}

Is it really OK for the parser service to hold refs until it goes away? Does it 
need to hold any refs at all? I fear that this might introduce a risk of cycles.

+class nsParserService : public nsIParserService {
etc.

It would be nice to see the nsIParserService interface use nsAStrings, so you can 
avoid this kind of stuff:

+  nsAutoString topic(NS_LITERAL_STRING("text/html"));

Fix those and r=sfraser

Comment 15

[harishd]

> nsAString is the new, better string way, no?
Didn't know that...will make it so if that's the case.

>Is it really OK for the parser service to hold refs until it goes away? Does it 
>need to hold any refs at all? I fear that this might introduce a risk of cycles.

ParserService holds a reference to the observer as long as the observer is
resgistered with the service. 

nsParserService::~nsParserService()
+{
+  nsObserverEntry *entry = nsnull;
+  while(entry = NS_STATIC_CAST(nsObserverEntry*,mEntries.Pop())) {
+    NS_RELEASE(entry);
+  }
+}

nsObserverEntry contains the data structure where the observers are stored. When 
the sink get's constructed it requests, the parser service, for a topic observer
interface through which the sink can notify observers. So, before handing over
the interface to the sink the parser service ADDREFs it to avoid accidental
deletion of the observer entry.

Comment 16

[harishd]

Attachment: patch v1.3 [ includes simon's suggestions ]

Comment 17

[harishd]

Fix is in. Marking FIXED.

Comment 18

[Moied]

verified fixed checked in cvs rev 3.115.