Status

()

RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: vlad, Assigned: vlad)

Tracking

Trunk
x86
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

Here's a textrun cache implementation that helps a ton, especially on the Mac.

* It requires that textruns be refcounted, though I can actually remove that requirement now that I think about it.
* It requires each textrun to keep the actual string; this could be optimized to just keep the glyphs or something.
* It requires an extra copy of the string to be kept around for the hash key.  I have code that just uses the string that's already in the textrun as a pointer, but because there's a distinction between UTF16 and ASCII, it doesn't work on Linux and Mac -- where we always store everything as UTF16, and we convert ASCII strings to UTF16 before making a textrun.  This sucks.
* The eviction algorithm could be much better

Nevertheless, it doesn't hurt perf on windows (improves it slightly), and it helps a ton on platforms with slow-measure, like Mac.
Created attachment 241895 [details] [diff] [review]
run cache patch

Initial patch; this just has the Mac bits, I have the windows bits on another computer, but they're not relevant for examining the code.
roc, stuart, could you guys take a look and give any feedback?
I'm surprised all those PR_Now()s aren't hurting performance.

Seems reasonable. I suspect it won't help when my code is ready, but it would be interesting to compare.
certainly a good idea for a stop-gap measure.
Created attachment 241989 [details] [diff] [review]
gfxTextRunCache patch

Ok, here's the final patch, with the Windows-specific changes.  Stuart is going to hate the string copying, but it's a net performance win.  The performance win could be made even greater by having the windows font back-end cache glyphs and other results from the "measure" computation instead of redoing them for draw.

Should we decide that we want to improve on this further even with roc's new stuff, there are a bunch of improvements we can make, but as it is, it should be a performance win.
Attachment #241895 - Attachment is obsolete: true
Attachment #241989 - Flags: superreview?(pavlov)
Attachment #241989 - Flags: review?(roc)
The hash functions aren't the greatest. I guess that doesn't matter much.

It's possible, although unlikely, that N seconds could elapse between creating a new textrun and calling Evict(). This could cause the new textrun to be evicted, leading to a crash.

If you have more than EVICT_MIN_COUNT in a cache and then there's no activity for a while, then we do a lookup, we will evict everything except for one textrun. That's a bit weird. I guess it's OK.
(In reply to comment #6)
> The hash functions aren't the greatest. I guess that doesn't matter much.

Suggestions appreciated, though as you say, it doesn't matter that much; I've never seen the hash lookup show up on a profile. 

> It's possible, although unlikely, that N seconds could elapse between creating
> a new textrun and calling Evict(). This could cause the new textrun to be
> evicted, leading to a crash.

Good point; I could call Evict() at the start of the function to solve this.

> If you have more than EVICT_MIN_COUNT in a cache and then there's no activity
> for a while, then we do a lookup, we will evict everything except for one
> textrun. That's a bit weird. I guess it's OK.

Yeah; I thought about evicting just the oldest half or something, but that complicates things.
Comment on attachment 241989 [details] [diff] [review]
gfxTextRunCache patch

r+ if you Evict() first.
Attachment #241989 - Flags: review?(roc) → review+
Comment on attachment 241989 [details] [diff] [review]
gfxTextRunCache patch

r+ if you Evict() first.
Attachment #241989 - Flags: superreview?(pavlov) → superreview+

Comment 10

12 years ago
Comment on attachment 241989 [details] [diff] [review]
gfxTextRunCache patch

ugh with the string copying. can you ifdef this code in and maybe avoid the constructor changes?
Attachment #241989 - Flags: superreview+ → superreview?(pavlov)
No, because we cache the textrun -- so the string inside it has to be long-lived, and not just a temporary borrowed pointer.

At some point I'll make it share the string with the string that's used as part of the key in the cache, but I don't think it should be an issue -- the string copy only happens for the first (cached) textrun, and doesn't happen for every subsequent use.

I checked this in but had to back it out, because I got screwed by our broken thebes linkage (see bug 351561).  I've been trying to fix the linkage all afternoon and evening, which has me oh-so-very-excited.

Comment 12

12 years ago
I admit I haven't spent much time looking over this patch (trying to really be on vacation...) but it seems like you could have the gfxTextRunCache thing own the string rather than making the textrun do it which would solve most of the problem, no?

Comment 13

12 years ago
there shouldn't be an issue where a textrun lives longer than the cache entry since nothing externally holds on to the textrun.
(In reply to comment #12)
> I admit I haven't spent much time looking over this patch (trying to really be
> on vacation...) but it seems like you could have the gfxTextRunCache thing own
> the string rather than making the textrun do it which would solve most of the
> problem, no?

Yeah, that probably would; I'd have to change the ATSUI and Pango code to take pointers.  I'll see if I can do that quickly, but it's probably worth doing that as a followup patch (especially because a lot of the string ownership in the textrun code is going to get changed around, with the new text frame stuff).
Finally checked in.  I didn't manage to keep the windows textrun still contain only a pointer; the problem was that the key can be copied by the hashtable, and so that changes the pointer value.  There's still a way around it, maybe, but I haven't done it yet.  Having the textrun hold the string and the hashtable key hold a pointer is probably the easiest; I'd just have to clean up atsui and pango.

I'll do that in a followup patch.  There are probably some wins to be made in the windows font code now, by aggressively caching the common parts of the MeasureOrDraw* functions -- e.g. caching the glyphs or similar.  Good fodder for future patches.
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Maybe, the Linux builds are broken by this check-in, can you look the regressions in your environments? On the FC5 that has the default settings for Japanese language, my build cannot render *all* pages correctly...
Created attachment 242276 [details]
screenshot

On my environment, we cannot use after 14-Oct builds...
I think that we need to clear the 'spacing' of gfxTextRun at re-using it.
I confirmed that the regression on Linux is caused by this check-in. I set the MOZ_GFX_NO_TEXT_CACHE=1 in my env, the regression is gone.
-> REOPEN for regression of Linux.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Created attachment 242281 [details] [diff] [review]
Patch for regression on Linux
Attachment #242281 - Flags: review?(vladimir)
I filed new bug(bug 356670) for the testers.
Comment on attachment 242281 [details] [diff] [review]
Patch for regression on Linux

Aiee, sorry!  I must not have updated my tree before testing on linux; I still had (old, I guess) code that did just one nsString.
Attachment #242281 - Flags: review?(vladimir) → review+
checked-in.
Status: REOPENED → RESOLVED
Last Resolved: 12 years ago12 years ago
Resolution: --- → FIXED

Updated

12 years ago
Depends on: 356695

Updated

12 years ago
Attachment #241989 - Flags: superreview?(pavlov)

Comment 25

12 years ago
I don't think anyone will care, but just as a note, this patch was the final strike for not being able to build cairo gfx with VC6 on Windows, see a relevant log here: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTest/1161182220.25619.gz&fulltext=1

Updated

12 years ago
Depends on: 357779
You need to log in before you can comment on or make changes to this bug.