Remove the (redundant) nsTextFrameTextRunCache.

RESOLVED FIXED in Firefox 48

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jfkthame, Assigned: jfkthame)

Tracking

unspecified
mozilla48
Points:
---

Firefox Tracking Flags

(firefox48 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

As far as I can see, this is a vestige of the long-dead word-caching scheme we used to use, and no longer serves any purpose. nsTextFrame code still puts things into this cache, but nobody ever looks anything up there, so it's just a waste of time and space. So I propose to kill it.
With gTextRuns out of the picture, it's easier to see that switching mTextRunsToDelete to be an array of UniquePtr<>s, as Jeff suggested in bug 1265459 comment 5, is a reasonable thing to do.
Attachment #8742701 - Flags: review?(mats)
(In reply to Jonathan Kew (:jfkthame) from comment #0)
> As far as I can see, this is a vestige of the long-dead word-caching scheme
> we used to use, and no longer serves any purpose. nsTextFrame code still
> puts things into this cache, but nobody ever looks anything up there, so
> it's just a waste of time and space. So I propose to kill it.

Hmm, on looking again, I'm not entirely sure this is true; we may still depend on the cache's expiration behavior to delete uninflated runs stashed in the textframe's UninflatedTextRunProperty(). Clearing r? for now, until I'm more confident what's going on there.
Attachment #8742697 - Flags: review?(mats)
Attachment #8742701 - Flags: review?(mats)
I *think* we're not shipping font inflation anywhere anymore, so it may be safe to start letting it bitrot.
We still have font-inflation reftests in the tree, AFAICS; and as far as tryserver can tell, they don't seem to be leaking textruns with the patches here,[1] so I think my concern in comment 3 may have been unfounded.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=de46a1a7a32f
OK, this doesn't lead to us leaking anything; it's simply old code that we no longer need, as we don't attempt to cache shaped words in terms of ranges of other textruns. We can also remove the related gtest file (which was disabled 'on suspicion' some time ago anyhow, and then forgotten about AFAICT).
Attachment #8742697 - Attachment is obsolete: true
Attachment #8742997 - Flags: review?(mats)
Attachment #8742701 - Flags: review?(mats)
Comment on attachment 8742997 [details] [diff] [review]
Remove the global nsTextFrameTextRunCache, as it no longer serves any useful purpose

Nice!
Attachment #8742997 - Flags: review?(mats) → review+
Attachment #8742701 - Flags: review?(mats) → review+
https://hg.mozilla.org/mozilla-central/rev/77ba0dcb977a
https://hg.mozilla.org/mozilla-central/rev/20956a16ffb8
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.