Closed
Bug 356235
Opened 18 years ago
Closed 18 years ago
implement textrun cache
Categories
(Core :: Graphics, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: vlad, Assigned: vlad)
References
Details
Attachments
(3 files, 1 obsolete file)
29.25 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
43.36 KB,
image/png
|
Details | |
1.94 KB,
patch
|
vlad
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•18 years ago
|
||
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.
Assignee | ||
Comment 2•18 years ago
|
||
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.
Assignee | ||
Comment 5•18 years ago
|
||
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.
Assignee | ||
Comment 7•18 years ago
|
||
(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•18 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)
Assignee | ||
Comment 11•18 years ago
|
||
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•18 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•18 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.
Assignee | ||
Comment 14•18 years ago
|
||
(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).
Assignee | ||
Comment 15•18 years ago
|
||
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: 18 years ago
Resolution: --- → FIXED
Comment 16•18 years ago
|
||
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...
Comment 17•18 years ago
|
||
On my environment, we cannot use after 14-Oct builds...
Comment 18•18 years ago
|
||
I think that we need to clear the 'spacing' of gfxTextRun at re-using it.
Comment 19•18 years ago
|
||
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.
Comment 20•18 years ago
|
||
-> REOPEN for regression of Linux.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 21•18 years ago
|
||
Attachment #242281 -
Flags: review?(vladimir)
Comment 22•18 years ago
|
||
I filed new bug(bug 356670) for the testers.
Assignee | ||
Comment 23•18 years ago
|
||
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+
Comment 24•18 years ago
|
||
checked-in.
Status: REOPENED → RESOLVED
Closed: 18 years ago → 18 years ago
Resolution: --- → FIXED
Updated•18 years ago
|
Attachment #241989 -
Flags: superreview?(pavlov)
![]() |
||
Comment 25•18 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
You need to log in
before you can comment on or make changes to this bug.
Description
•