Use PLDHashTable2 in FontNameCache

RESOLVED FIXED in Firefox 41

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla41
Points:
---

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
I tried to introduce PLDHashTable2 into FontNameCache in another bug but it
caused a Fennec start-up regression. I've now managed to find a variation that
avoids the regression.
(Assignee)

Comment 1

3 years ago
Created attachment 8613361 [details] [diff] [review]
Use PLDHashTable2 in FontNameCache
Attachment #8613361 - Flags: review?(nfroyd)
Yikes...
Comment on attachment 8613361 [details] [diff] [review]
Use PLDHashTable2 in FontNameCache

Review of attachment 8613361 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxFT2FontList.cpp
@@ +622,5 @@
>      {
> +        // HACK ALERT: it's weird to assign |mOps| after we passed a pointer to
> +        // it to |mMap|'s constructor. A more normal approach here would be to
> +        // have a static |sOps| member. Unfortunately, this mysteriously but
> +        // consistently makes Fennec start-up slower, so we take this

This is...awful.  But I am glad to see that it's so obvious in this patch what the key change was. ;)

The best alternative I have is to have:

  char mMap[sizeof(PLDHashTable2)]; // with some alignment magic

and use placement new-initialization on that with a static sOps in the constructor.  (Probably with a Map() getter to hide the necessary casting.)  I can't decide whether that's worse or better.

I wish we knew *why* this was the case, though; I'd expect the lone reference to global data to be no worse than having to initialize mOps for every single cache...Do you have any explanation for why this variant might be faster?  (Acquired, perhaps, through hours of yelling at the computer?)
Attachment #8613361 - Flags: review?(nfroyd) → review+
(Assignee)

Comment 4

3 years ago
> The best alternative I have is to have:
> 
>   char mMap[sizeof(PLDHashTable2)]; // with some alignment magic
> 
> and use placement new-initialization on that with a static sOps in the
> constructor.  (Probably with a Map() getter to hide the necessary casting.) 
> I can't decide whether that's worse or better.

Worse, IMO :)  Plus I suspect the regression involves the static member somehow, which is why I went for a hack that avoided creating one. 


> I wish we knew *why* this was the case, though; I'd expect the lone
> reference to global data to be no worse than having to initialize mOps for
> every single cache...Do you have any explanation for why this variant might
> be faster?  (Acquired, perhaps, through hours of yelling at the computer?)

No explanation, other than mysterious Android start-up performance regressions that don't affect other platforms are common. I've had a couple, glandium has battled them, etc. :(
https://hg.mozilla.org/mozilla-central/rev/8f567d651c1a
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.