230603 - Impedance mismatch between nsILanguageAtom and nsIDeviceContext

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Boris Zbarsky [:bzbarsky]]

nsIDeviceContext wants a language group atom.  Layout seems to have an
nsILanguageAtom more often than not.  The resulting GetLanguageGroup() calls
show up in profiles as one of the main traffickers in nsCOMPtr.

Is there anything we can do with these APIs to make them a little better suited
to each other?  Is there a real reason for nsILanguageAtom?

Comment 1

[Boris Zbarsky [:bzbarsky]]

To be precise, the profile I am looking at has 32 total hits under
nsCOMPtr_base::~nsCOMPtr_base, of which 8 are under AtomImpl::Release (and 17
are directly inside nsCOMPtr_base::~nsCOMPtr_base).

Of the 32 calls into nsCOMPtr_base::~nsCOMPtr_base that are represented, 11
nsCOMPtr<nsIAtom> resulting from this nsILanguageAtom stuff.  So if we assume
that the ratios sorta hold, something like 6 of the 8 AtomImpl::Release hits
come from this one cause.  On this profile (view-source of a medium-sized page)
that's about 0.5% of pageload.

Comment 2

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

See also bug 105859.

Comment 3

[Brian Ryner (not reading)]

I'm seeing this in profiles as well.

It looks to me like the purpose of nsILanguageAtom is to represent a (language
string, language group nsIAtom) pair.  Certainly the only public part of the
language group (at least, the only one any callers use) is its language group
nsIAtom.

nsILanguageAtomService allows you to look up an nsILanguageAtom in various ways,
such as by language string, by charset (which determines a language string and
then looks it up that way), or the application-default language.

So, it seems that the nsILanguageAtom interface can go away entirely.  The
lookup methods on nsILanguageAtomService can simply return the language group
nsIAtom, since that's the only thing anyone cares about.  The current array of
nsLanguageAtom objects could be replaced by a (string,nsIAtom) hash table that
maps language strings to language group atoms.

I'll get started on this unless anyone thinks I'm missing something.

Comment 4

[Brian Ryner (not reading)]

Attachment: patch

Comment 5

[Brian Ryner (not reading)]

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

Boris, can you review, or r+sr if you feel comfortable doing so?

Comment 6

[Brian Ryner (not reading)]

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

or dbaron, please r+sr if you want.

Comment 7

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Were any existing callers testing language atom equality (rather than getting
the lang group and testing lang group equality)?  If so, this could change
behavior.  (But if they were using the result of LookupCharSet for
such things they could be in a mess already.)
                                                                                
The nsFontMetricsXlib change needs an addref.
                                                                                
Should ~nsLanguageAtomService be NS_HIDDEN too?
                                                                                
The error handling near the end of nsLanguageAtomService::LookupLanguage seems
unnecessary -- the atom will be the same whether it's in the hashtable or not,
so there's no need to do error handling for mLangs.Put. 

Comment 8

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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

r+sr=dbaron, since I'm pretty confident they were never used in pointer
comparisons.  (They were only stored two places outside of the service --
nsPresContext and nsStyleVisibility -- and bryner changed the name in both
places.)

Comment 9

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

(In reply to comment #7)
> The nsFontMetricsXlib change needs an addref.

bryner points out that this was an nsCOMPtr.  That means the old code was
leaking (double-addref), and there's still a double-addref leak right below,
which should be fixed by changing NS_NewAtom to do_GetAtom.

Comment 10

[Roland Mainz]

David Baron
wrote:                                                                         
> the nsFontMetricsXlib change needs an addref.

If I recall it correctly I have turned the whole Xlib/Xprint gfx over to
nsCOMPtr usage - are you sure the addref is needed ?

Comment 11

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

The patch on bug 187125 introduced a whole bunch of double-addref leaks.  I
commented there.

Comment 12

[Brian Ryner (not reading)]

checked in.