559315 - [HTML5] nsContentSink::Init call is slow

Status: RESOLVED FIXED

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

Description

[Olli Pettay [:smaug][[e-mail]]]

Seems like https://bugzilla.mozilla.org/show_bug.cgi?id=559311#c0 is valid even after the patch in that bug. ::Init is really slow.

Comment 1

[Henri Sivonen (:hsivonen) (not reading bugmail while on leave)]

The prefs really ought to use nsContentUtils::AddBoolPrefVarCache

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

Would this be ok. Make most of the variables global, since that is what
they really are. Prefs shouldn't be changed too often.

I posted the patch to tryserver.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

On OSX the patch speeds up https://bug404385.bugzilla.mozilla.org/attachment.cgi?id=289857 ~8%, which is expected since things under ::Init
take ~10% without the patch and ~2% with the patch.

Comment 4

[Henri Sivonen (:hsivonen) (not reading bugmail while on leave)]

Comment on attachment 440495 [details] [diff] [review]
patch

Looks good to me, although I think using nsContentSink::sFoo instead of gFoo would be more proper. r=hsivonen with the globals changed to static members. However, I'm not officially qualified to review this, so you should in principle get an sr anyway. If the superreviewer prefers globals, I defer to the superreviewer.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Attachment: static members

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 440732 [details] [diff] [review]
static members

This won't work if the preferences are removed. I.e. we won't fall back to the default value in that case.

This is fine for these preferences, but I'm wary of having that behavior in general for these nsContentUtils functions.

Perhaps allocate a small object with default value and cache location, and use that pointer as closure.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #6)
> (From update of attachment 440732 [details] [diff] [review])
> This won't work if the preferences are removed. I.e. we won't fall back to the
> default value in that case.
Do we care? We don't care elsewhere where the API is used.
 
> Perhaps allocate a small object with default value and cache location, and use
> that pointer as closure.
I don't understand what you mean here.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

And in anyway, the "we won't fall back to the default value" would be a bug
in nsContentUtils. Perhaps worth to file a followup.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Er, ok I see you don't like the way I give default value as a parameter to
nsContentUtils methods.
But still, do we really care if the prefs are removed.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Perhaps I shouldn't call the last parameter aDefault, but aInitial.
Would that be ok enough?

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Like I said, I don't think we care for these specific parameters, but I have no idea if we'll care for future consumers of this API. I don't think changing the name of the argument will make much difference.

It's easy enough to make this API do the right thing so I don't see a reason not to.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

I still don't understand what you mean with
"Perhaps allocate a small object with default value and cache location, and use
 that pointer as closure."

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Something like:

struct PrefCacheData {
  void* cacheLocation;
  union {
    PRBool defaultValueBool;
    PRInt32 defaultValueInt;
  };
};


void
nsContentUtils::AddBoolPrefVarCache(const char *aPref,
                                    PRBool* aCache,
                                    PRBool aDefault)
{
  PrefCacheData* data = new PrefCacheData;
  data->cacheLocation = aCache;
  data->defaultValueBool = aDefault;
  sCacheData->AppendElement(data);

  RegisterPrefCallback(aPref, BoolVarChanged, data);
}


where sCacheData is an nsTArray<nsAutoPtr<PrefCacheData> >* which is deleted on shutdown.

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

If you do that, you could also stick the preference name in PrefCacheData, which could be used to solve the problem we have now that the prefix strings are actually prefixes. I.e. if you add any preference starting with the string name passed in we'll get notified and use that new preference.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Ah. I thought almost understood what you mean, but got it wrong after all :)

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v3

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 441658 [details] [diff] [review]
v3

Looks great

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

http://hg.mozilla.org/mozilla-central/rev/592ab8ae5518

Comment 19

[Dão Gottwald [:dao]]

Did this change the content.notify.ontimer default value from true to false?

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

Uh, yes. Mistake.
I'll fix.

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

http://hg.mozilla.org/mozilla-central/rev/3c5d45390f00