Closed Bug 356235 Opened 16 years ago Closed 16 years ago

implement textrun cache

Categories

(Core :: Graphics, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: vlad, Assigned: vlad)

References

Details

Attachments

(3 files, 1 obsolete file)

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.
Attached patch run cache patch (obsolete) — Splinter Review
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.
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 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.
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?
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
Closed: 16 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...
Attached image 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 → ---
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
Closed: 16 years ago16 years ago
Resolution: --- → FIXED
Depends on: 356695
Attachment #241989 - Flags: superreview?(pavlov)
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
Depends on: 357779
You need to log in before you can comment on or make changes to this bug.