Closed Bug 559315 Opened 10 years ago Closed 10 years ago

[HTML5] nsContentSink::Init call is slow

Categories

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

x86
All
defect

Tracking

()

RESOLVED FIXED

People

(Reporter: smaug, Assigned: smaug)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

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.
Blocks: domperf
The prefs really ought to use nsContentUtils::AddBoolPrefVarCache
Priority: -- → P3
Assignee: nobody → Olli.Pettay
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #440495 - Flags: review?(hsivonen)
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 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.
Attachment #440495 - Flags: review?(hsivonen) → review+
Attached patch static members (obsolete) — Splinter Review
Attachment #440732 - Flags: review?(jonas)
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.
Attachment #440732 - Flags: review?(jonas) → review-
(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.
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.
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.
Perhaps I shouldn't call the last parameter aDefault, but aInitial.
Would that be ok enough?
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.
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."
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.
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.
Ah. I thought almost understood what you mean, but got it wrong after all :)
Attached patch v3Splinter Review
Attachment #440495 - Attachment is obsolete: true
Attachment #440732 - Attachment is obsolete: true
Attachment #441658 - Flags: review?(jonas)
http://hg.mozilla.org/mozilla-central/rev/592ab8ae5518
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Did this change the content.notify.ontimer default value from true to false?
Uh, yes. Mistake.
I'll fix.
You need to log in before you can comment on or make changes to this bug.