Closed Bug 97299 Opened 23 years ago Closed 4 years ago

Store font names in lowercase to avoid conversions

Categories

(Core :: Graphics: Text, defect, P4)

Product:
Core
Component:
Graphics: Text
Type:
defect

Tracking

()

Status:
RESOLVED WONTFIX

People

(Reporter: jesup, Assigned: jesup, NeedInfo)

References

Details

(Keywords: fonts, intl, perf)

Attachments

(1 file, 2 obsolete files)

Updated patch. nsFont changed to class from struct.
7.17 KB, patch
attinasi
: superreview+
Details | Diff | Splinter Review 
We should store font names in nsFont in lowercase to avoid constant conversions
when reflowing/rendering/etc in nsFont::Equals().  This shows up in jprofs of
loading large pages (like jprofs, or lxr).

I don't think this will affect anything, however it in theory could affect what
case users see fonts listed in, perhaps.


Assigned to me because nsFont.cpp hasn't had a change other than mass string
changes in years.
BTW, nsFont::Equals used 4.2% of the time spent in reflow loading a jprof.html
file.  Reflow used about 40% of the total CPU time spent in load.
Keywords: perf
Blocks: 71668
Patch available for review.  Note that I don't seem to have any fonts with
uppercase characters in them anyways; someone with a Win32 build system should
look at this patch.
Keywords: patch, qawanted
Target Milestone: --- → mozilla0.9.5
Status: NEW → ASSIGNED
Priority: -- → P4
looks good.
r=pavlov
Attachment #47328 - Flags: review+
What happened to the plan to make nsFonts into atoms? There was much talk about 
that this spring. 

I know nothing about that plan.  Bug #?  In any case, we should probably do
this; it's low-hanging fruit.
Comment on attachment 47328 [details] [diff] [review]
Patch to store font names in nsFont as lowercase

sr=attinasi BTW: if you want to replace the comments that say "name should be lower case" with assertions, that would be better I think
Attachment #47328 - Flags: superreview+
Marc: good call on the assertions.  This is a struct, not a class, and name was
therefore public, and getting set in other places than nsFont.cpp.  I'm revising
the patch.
have r=darin on netwerk/http portion via email
For uriloader portion via email: looks fine to me, r=darin
I don't remember the bug ¤. Something about nsFonts being expensive to create. I
think the bug might have been closed after hyatt's style rewrite because we
stopped creating tens, or hundred of thousands of nsFonts so the creation time
was no longer important. Anyway, I think it was Erik och rbs who talked about
it. Btw, rbs should probably see this as he is working a lot on fonts.
Apologies; those r=darin's were for bug 92576.

If there's someone working on Fonts, I didn't know it - nsFont.cpp hasn't had
any direct attention in years.
The work is done on a branch, FONT_20011307_BRANCH . See
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/gfx/src&command=DIFF_FRAMESET&file=nsFont.cpp&rev1=3.11&rev2=3.11.4.1&root=/cvsroot
for the last change. I don't know the status of it, but I think rbs can
contribute to your work atleast.
As acknowledged in the comments of 2001-09-07 15:04 the patch isn't safe at all.

>nsFont::Equals used 4.2%

Do you have any figure after the change (ignoring the unsafe nature of the 
current patch for a moment).
Keywords: fonts, intl
Of interest: bug 77942


This is an updated patch that catches a few other places where nsFonts can be
initialized; I caught them by converting the struct to a class and then
(temporarily) making 'name' protected.  In this patch I've left it a class;
however I have not made the members protected since that would require more
extensive changes.

I'm going to go back and measure with and without the patch to see what
difference this makes before asking for final reviews, but anyone interested
might want to try it now.  Note that I have DEBUG-only assertions in there to
try to catch non-lowercase font names.
Ok, measurements are in.  Hits for loading a specific 300K jprof went from ~4.4%
of reflow time to just under 2%.  (Reflow is about 40% of load time.)  This is
on a fresh pull, dual-450 PIII Linux RH7.1 gcc2.96.2 -O2 disable-debug build.

On the basis of that, requesting re-approval.  Pavlov/Marc?
Attachment #47328 - Attachment is obsolete: true
Comment on attachment 49161 [details] [diff] [review]
Updated patch.  nsFont changed to class from struct.

sr=attinasi - But, I think the default ctor for nsFont is unnecessary - it is empty. Why is it there?
Attachment #49161 - Flags: superreview+
I had been using it for breakpoints.  I can certainly remove it if you like
(makes sense).  Of course, why was the destructor there already?  It's not
virtual and is empty.  Should I remove both while I'm at it?
jesup:
Wanna patch Xlib gfx code too, please - or should I file an extra patch for that
?
Taking QA job...
Keywords: qawanted
QA Contact: tpreston → Roland.Mainz
r=pavlov
I'll check in after the tree opens (if I'm still around).  
is the code in?
Per CVS log: No.

I am going to file a Xlib port for the GTK+ part (need ~1hour...) ...
I missed tree closing this morning due to a meeting, and it hasn't opened
again.  Once it opens, I will commit the patch if I'm around.  I'm going home,
so perhaps I'll try again in the morning.

Comment on attachment 49999 [details] [diff] [review]
Patch for gfx/src/xlib/nsDeviceContextXlib.cpp (not part of default build)

Filed patch for Xlib toolkit.
this piece of code is not part of the default build, a simple r=jesup should be more than suffcient... :-)
Comment on attachment 49999 [details] [diff] [review]
Patch for gfx/src/xlib/nsDeviceContextXlib.cpp (not part of default build)

r=rjesup@wgate.com
Attachment #49999 - Flags: review+
Fix checked in
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Reopening. I get hundreds of these on my Mac trunk debug build (I stopped the
run after about 650):

###!!! ASSERTION: nsFont name wasn't lowercase: 'name.Equals(aOther.name)', file
nsFont.cpp, line 56

Please fix or back out.
Severity: minor → major
Status: RESOLVED → REOPENED
OS: All → Mac System 9.x
Hardware: PC → Macintosh
Resolution: FIXED → ---
Make this a smoketest blocker.  The app is completely unusable on my Macintosh 
debug build.  I want this fix backed out until the assertion isn't triggered.
Severity: major → blocker
Keywords: smoketest
Ok, let's back out and try another pass later.
Status: REOPENED → ASSIGNED
I'm seeing the assertions on Windows too.
Keywords: smoketest
OS: Mac System 9.x → All
Hardware: Macintosh → All
Target Milestone: mozilla0.9.5 → mozilla0.9.6
Severity: blocker → minor
Target Milestone: mozilla0.9.6 → mozilla0.9.7
Mozilla 0.9.7 is long gone. Randell, could you update the target milestone?
Target Milestone: mozilla0.9.7 → mozilla1.0
dunno how this got in the imagelib component
Component: ImageLib → GFX Compositor
jesup:
Are you still working on this bug ?
Product: Core → Core Graveyard
No longer valid - nsFont::Equals() no longer shows up in an 8MB jprof file load.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago → 13 years ago
Resolution: --- → INVALID
Correction: A higher-resolution jprof (and turning off debug) shows nsFont:Equals() is around 1.5% of a large jprof file load.  Of that, 0.9% nsAString_internal::Equals(); most of the rest is in nsFont::Equals().

Better, but still big enough to affect page-load times.
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
If this is still valid, it should be moved to the proper place.  Also CCing some of the current font people.
Status: REOPENED → ASSIGNED
Component: GFX → Graphics
Product: Core Graveyard → Core
QA Contact: roland.mainz → thebes
Target Milestone: mozilla1.0 → ---
Comment on attachment 49999 [details] [diff] [review]
Patch for gfx/src/xlib/nsDeviceContextXlib.cpp (not part of default build)

nsDeviceContextXlib no longer exists.
Attachment #49999 - Attachment is obsolete: true
When I last checked this earlier this year, it's still valid, though not showing up as a major performance hit on the simple tests I did (generic browsing).  It is silly to continually convert to lowercase.
This doesn't apply anymore?
Flags: needinfo?(rjesup)
The name comparison has moved to (it appears) gfxFontFamilyList.h:166 (a.mType == b.mType && a.mName == b.mName), which appears to be a case-ful comparison now.   I will note that CSS3 font-family name comparisons are *insensitive*, but I'm unsure as to where this is being done. 

David?

"User agents must match these names case insensitively, using the "Default Caseless Matching" algorithm outlined in the Unicode specification [UNICODE]."

https://drafts.csswg.org/css-fonts/#font-family-casing
Flags: needinfo?(rjesup) → needinfo?(dbaron)
(In reply to Randell Jesup [:jesup] from comment #46)
> The name comparison has moved to (it appears) gfxFontFamilyList.h:166
> (a.mType == b.mType && a.mName == b.mName), which appears to be a
> case-ful comparison now.   I will note that CSS3 font-family name
> comparisons are *insensitive*, but I'm unsure as to where this is
> being done. 
> 
> David?
> 
> "User agents must match these names case insensitively, using the "Default
> Caseless Matching" algorithm outlined in the Unicode specification
> [UNICODE]."
> 
> https://drafts.csswg.org/css-fonts/#font-family-casing

The name comparison there applies to the actual CSS name ==> font lookup. Fonts in the system fontlist and the user fontlist for a document are hashed using the lowercased name as the key, so this will be a case-insensitive name lookup. This isn't quite the same as the "Default Caseless Matching" described in the spec but it's in practice equivalent.

I don't think nsFont comparisons need to be case-insensitive, unless you think I'm overlooking something David?

We could lowercase the name when parsing but that would mean the name in devtools or in the computed value would be slightly different from what the author was using.

To summarize, I think the context of this bug is no longer valid. I suggest we resolve it.
Flags: needinfo?(jdaggett)
Component: Graphics → Graphics: Text
Status: ASSIGNED → RESOLVED
Closed: 13 years ago → 4 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: