Closed
Bug 230603
Opened 21 years ago
Closed 20 years ago
Impedance mismatch between nsILanguageAtom and nsIDeviceContext
Categories
(Core :: Layout, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: bzbarsky, Unassigned)
Details
Attachments
(1 file)
31.40 KB,
patch
|
dbaron
:
review+
dbaron
:
superreview+
|
Details | Diff | Splinter Review |
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?
Reporter | ||
Comment 1•21 years ago
|
||
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.
See also bug 105859.
Comment 3•20 years ago
|
||
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•20 years ago
|
||
Comment 5•20 years ago
|
||
Comment on attachment 150099 [details] [diff] [review] patch Boris, can you review, or r+sr if you feel comfortable doing so?
Attachment #150099 -
Flags: review?(bzbarsky)
Comment 6•20 years ago
|
||
Comment on attachment 150099 [details] [diff] [review] patch or dbaron, please r+sr if you want.
Attachment #150099 -
Flags: superreview?(dbaron)
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 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.)
Attachment #150099 -
Flags: superreview?(dbaron)
Attachment #150099 -
Flags: superreview+
Attachment #150099 -
Flags: review?(bzbarsky)
Attachment #150099 -
Flags: review+
(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•20 years ago
|
||
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 ?
The patch on bug 187125 introduced a whole bunch of double-addref leaks. I commented there.
Comment 12•20 years ago
|
||
checked in.
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Updated•6 years ago
|
Product: Core → Core Graveyard
Assignee | ||
Updated•6 years ago
|
Component: Layout: Misc Code → Layout
Product: Core Graveyard → Core
You need to log in
before you can comment on or make changes to this bug.
Description
•