1170069 - Use PLDHashTable2 in FontNameCache

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Nicholas Nethercote [inactive]]

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.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Use PLDHashTable2 in FontNameCache

Comment 2

[Andrew McCreight [:mccr8]]

Yikes...

Comment 3

[Nathan Froyd [:froydnj]]

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?)

Comment 4

[Nicholas Nethercote [inactive]]

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

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8f567d651c1a

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8f567d651c1a