Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
885903002:
Make the glyph array entries inline. (Closed)
DescriptionMake the glyph array entries inline.
Perf Reports
bin/c --match nytimes --config 8888
desk_nytimes.skp_1_mpd 1.41ms -> 1.38ms 0.98x
desk_nytimes.skp_1 1.94ms -> 1.87ms 0.97x
bin/c --match nytimes --config gpu
desk_nytimes.skp_1_mpd 1.63ms -> 1.59ms 0.97x
desk_nytimes.skp_1 1.56ms -> 1.5ms 0.97x
Here are results from mac instruments:
--match nytimes --config gpu --samples 10000 --skps /Users/herb/src/skia/skps
Inline:
Total Samples Running Time Self Symbol Name
94335 94335.0ms 98.3% 0.0 start
2365 2365.0ms 2.4% 2365.0 SkGlyphCache::getGlyphIDMetrics(unsigned short, int, int)
975 975.0ms 1.0% 975.0 SkGlyphCache::lookupMetrics(unsigned int, SkGlyphCache::MetricsType)
Clean:
Total Samples Running Time Self Symbol Name
96833 96833.0ms 97.3% 0.0 start
3418 3418.0ms 3.4% 3418.0 SkGlyphCache::getGlyphIDMetrics(unsigned short, int, int)
1961 1961.0ms 1.9% 1961.0 SkGlyphCache::lookupMetrics(unsigned int, SkGlyphCache::MetricsType)
BUG=skia:
Committed: https://skia.googlesource.com/skia/+/4c08f16b252a55e438a61f26e5581394ed177da1
Committed: https://skia.googlesource.com/skia/+/b4c29ac173e6f8844327338687248b98bc94132d
Committed: https://skia.googlesource.com/skia/+/e70de9e4f0b7bf73f7cd1a20dbabcb233ffbb7f1
Patch Set 1 #Patch Set 2 : Fix tracking problem. #Patch Set 3 : Move to using indexes. #Patch Set 4 : Add hash index adjustment #
Total comments: 2
Patch Set 5 : Change to sentinal for invalid #Patch Set 6 : Debugged sentinal font cache #Patch Set 7 : Fix spelling. #Patch Set 8 : Add parens as suggested by build #
Total comments: 3
Patch Set 9 : Fix asserts for checking code in range #
Total comments: 1
Patch Set 10 : Fix change from uint16 to in. #Patch Set 11 : Fix calling the wrong function for glyphs #Patch Set 12 : new sync #Patch Set 13 : Win 7 font scaler fixed #Patch Set 14 : Move sentinal to ~0 as id #Patch Set 15 : Rebase onto SkGlyph changes. #Patch Set 16 : Fix comments remove old code #
Messages
Total messages: 53 (12 generated)
herb@google.com changed reviewers: + mtklein@google.com
Using indexes, but not adjusting the caches during inserts. How does this approach look?
Added index adjustment. skdiff shows no diffs.
reed@google.com changed reviewers: + reed@google.com
slight confusion on names: - maintain makes some sense from the impl p.o.v., but not much from the callers. find or lookup or get all seem more natural for the caller. - maintainGlyph takes an ID, but maintainChar takes a charcode. This diff surprised me, as I thought they both took their "name" (i.e. maintainGlyph would take a glyph code). https://codereview.chromium.org/885903002/diff/60001/src/core/SkGlyphCache.cpp File src/core/SkGlyphCache.cpp (right): https://codereview.chromium.org/885903002/diff/60001/src/core/SkGlyphCache.cp... src/core/SkGlyphCache.cpp:70: // init to 0 so that all of the pointers will be null comment (0) disagrees with the code (0xFF) I'm guessing 0xFFFF is your invalid glyph index? Can we store a glyph at slot 0 that never matches (e.g. glyphindex == 0xFFFF) to void the double check?
Fixed names and added sentinels. https://codereview.chromium.org/885903002/diff/60001/src/core/SkGlyphCache.cpp File src/core/SkGlyphCache.cpp (right): https://codereview.chromium.org/885903002/diff/60001/src/core/SkGlyphCache.cp... src/core/SkGlyphCache.cpp:70: // init to 0 so that all of the pointers will be null On 2015/01/30 13:57:38, reed1 wrote: > comment (0) disagrees with the code (0xFF) > > I'm guessing 0xFFFF is your invalid glyph index? > Can we store a glyph at slot 0 that never matches (e.g. glyphindex == 0xFFFF) to > void the double check? Done.
The CQ bit was checked by herb@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/885903002/120001
Note for Reviewers: The CQ is waiting for an approval. If you believe that the CL is not ready yet, or if you would like to L-G-T-M with comments then please uncheck the CQ checkbox. Waiting for LGTM from valid reviewer(s) till 2015-02-01 11:15 UTC
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Build-Ubuntu13.10-GCC4.8-Arm7-Debug-Android-Trybot on client.skia.compile (http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu13.10-GC...) Build-Ubuntu13.10-GCC4.8-x86_64-Release-Trybot on client.skia.compile (http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu13.10-GC...)
The CQ bit was checked by herb@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/885903002/130001
Note for Reviewers: The CQ is waiting for an approval. If you believe that the CL is not ready yet, or if you would like to L-G-T-M with comments then please uncheck the CQ checkbox. Waiting for LGTM from valid reviewer(s) till 2015-02-01 11:49 UTC
The CQ bit was unchecked by commit-bot@chromium.org
No LGTM from a valid reviewer yet. Please ask for an LGTM from a full Skia committer from https://skia.googlesource.com/skia/+/master/CQ_COMMITTERS
https://codereview.chromium.org/885903002/diff/130001/src/core/SkGlyph.h File src/core/SkGlyph.h (right): https://codereview.chromium.org/885903002/diff/130001/src/core/SkGlyph.h#newc... src/core/SkGlyph.h:132: static uint32_t MakeID(unsigned code) { can we add? SkASSERT(0 == (code & kCodeMask));
https://codereview.chromium.org/885903002/diff/130001/src/core/SkGlyph.h File src/core/SkGlyph.h (right): https://codereview.chromium.org/885903002/diff/130001/src/core/SkGlyph.h#newc... src/core/SkGlyph.h:132: static uint32_t MakeID(unsigned code) { On 2015/02/02 11:49:36, reed1 wrote: > can we add? > > SkASSERT(0 == (code & kCodeMask)); This assert doesn't make sense to me. Do you mean (code + 1) <= kCodeMask?
PTAL https://codereview.chromium.org/885903002/diff/130001/src/core/SkGlyph.h File src/core/SkGlyph.h (right): https://codereview.chromium.org/885903002/diff/130001/src/core/SkGlyph.h#newc... src/core/SkGlyph.h:132: static uint32_t MakeID(unsigned code) { On 2015/02/02 11:49:36, reed1 wrote: > can we add? > > SkASSERT(0 == (code & kCodeMask)); Done. I think your intent was to check the pre-condition that code is in the 24-bit range. Add assert to do this.
lgtm w/ nits https://codereview.chromium.org/885903002/diff/150001/src/core/SkGlyphCache.h File src/core/SkGlyphCache.h (right): https://codereview.chromium.org/885903002/diff/150001/src/core/SkGlyphCache.h... src/core/SkGlyphCache.h:231: void adjustCaches(int16_t insertion_index); nit: skia usually doesn't use "contricted" types for parameter types, as it will hide overflows. In this case, it would take an "int", but if we had to assign it to a shorter type (e.g. in16_t) we could use SkToS16() which checks for overflow in debug-builds.
The CQ bit was checked by herb@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/885903002/170001
Message was sent while issue was closed.
Committed patchset #10 (id:170001) as https://skia.googlesource.com/skia/+/4c08f16b252a55e438a61f26e5581394ed177da1
Message was sent while issue was closed.
A revert of this CL (patchset #10 id:170001) has been created in https://codereview.chromium.org/897463004/ by bsalomon@google.com. The reason for reverting is: I suspect this is causing the off-by-one character issues that show up in gold.skia.org as of now. All the errors I've seen are on a Win7 bot. Example: good: https://gold.skia.org/img/images/35396bb0d299b81c0031dc0632a019d4.png bad: https://gold.skia.org/img/images/484e511f9e696d95031cd25aeae59da0.png .
Message was sent while issue was closed.
I have found the bug causing the regression. Do I need to start a new CL?
Message was sent while issue was closed.
On 2015/02/07 01:58:48, herb1 wrote: > I have found the bug causing the regression. Do I need to start a new CL? It's usally fine to do as you did here, just re-upload to the original CL. I'd only start a new CL if you're closer to starting from scratch than to the previous attempt. If you click "Edit Issue", you can untick the "Closed" box, then use trybots, resubmit, etc.
The CQ bit was checked by herb@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/885903002/190001
Message was sent while issue was closed.
Committed patchset #11 (id:190001) as https://skia.googlesource.com/skia/+/b4c29ac173e6f8844327338687248b98bc94132d
Message was sent while issue was closed.
On 2015/02/09 14:38:33, I haz the power (commit-bot) wrote: > Committed patchset #11 (id:190001) as > https://skia.googlesource.com/skia/+/b4c29ac173e6f8844327338687248b98bc94132d Looks like the bug's still there. Want to take a look at gold.skia.org before we revert? As far as I can see, the problem is Win7-bot only (maybe Win8 haven't finished yet) and again seems to be promoting glyph images by 1, e.g. 'm' draws as 'n', but the metrics seem to be from the correct glyph, so the 'n' is spaced as if it were as wide as an 'n'.
Message was sent while issue was closed.
On 2015/02/09 15:39:13, mtklein wrote: > On 2015/02/09 14:38:33, I haz the power (commit-bot) wrote: > > Committed patchset #11 (id:190001) as > > https://skia.googlesource.com/skia/+/b4c29ac173e6f8844327338687248b98bc94132d > > Looks like the bug's still there. Want to take a look at http://gold.skia.org before > we revert? As far as I can see, the problem is Win7-bot only (maybe Win8 > haven't finished yet) and again seems to be promoting glyph images by 1, e.g. > 'm' draws as 'n', but the metrics seem to be from the correct glyph, so the 'n' > is spaced as if it were as wide as an 'n'. Actually, we're going to revert this now. I've just learned how to query gold.skia.org for the effects of past CLs, so there's no need to keep the tree broken: - visit https://gold.skia.org/#/triage?head=0 - paste b4c29ac173e6f8844327338687248b98bc94132d into both sides of the commit range filter - press update It should take you to a URL that looks like this, which will show images produced by the bots when your CL was the latest CL: https://gold.skia.org/#/triage?ce=b4c29ac173e6f8844327338687248b98bc94132d&cs... Unfortunately, it doesn't look like that URL works when you go to it directly.
Message was sent while issue was closed.
A revert of this CL (patchset #11 id:190001) has been created in https://codereview.chromium.org/911513003/ by mtklein@google.com. The reason for reverting is: Still broken..
Message was sent while issue was closed.
A revert of this CL (patchset #11 id:190001) has been created in https://codereview.chromium.org/910803002/ by jvanverth@google.com. The reason for reverting is: Causing many text errors in gold images. Appears to be mainly Test-Win7-ShuttleA-HD2000-x86-*-GDI-Trybot,.
Message was sent while issue was closed.
PTAL Running bin/c has some variation. The ones digit can be +/- 2, so maybe 5% variation. We should probably make fID in SkGlyph private after this CL.
Message was sent while issue was closed.
On 2015/02/13 20:44:20, herb1 wrote:
> PTAL
>
> Running bin/c has some variation. The ones digit can be +/- 2, so maybe 5%
> variation.
>
> We should probably make fID in SkGlyph private after this CL.
Sorry, I can see this is confusing, but I think your GPU results are not
comparing what you'd expect:
bin/c --match nytimes --config 8888
desk_nytimes.skp_1 2.25ms -> 1.87ms 0.83x
desk_nytimes.skp_1_mpd 2.41ms -> 1.29ms 0.54x
bin/c --match nytimes --config gpu
desk_nytimes.skp_1 2.25ms -> 1.53ms 0.68x
desk_nytimes.skp_1_mpd 2.41ms -> 1.6ms 0.66x
Looks suspiciously like we didn't rerun the baseline for GPU. bin/c doesn't
rerun the baseline if it sees its logfile is present (for faster iteration), but
that means that you have to manually remove it when you change what you're
testing.
Message was sent while issue was closed.
> We should probably make fID in SkGlyph private after this CL. Can we make this private this CL, or too invasive? Anyway, timing and privacy aside, this LGTM for another try at the correctness bots.
Yeah, that matches my latest intuitions around this a lot better.
The CQ bit was checked by herb@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mtklein@google.com, reed@google.com Link to the patchset: https://codereview.chromium.org/885903002/#ps290001 (title: "Fix comments remove old code")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/885903002/290001
Message was sent while issue was closed.
This issue passed the CQ. To commit it, remove "COMMIT=false" from the description and try again.
The performance numbers are the same as reported before. PTAL
The CQ bit was checked by mtklein@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/885903002/290001
Message was sent while issue was closed.
This issue passed the CQ. To commit it, remove "COMMIT=false" from the description and try again.
The CQ bit was checked by mtklein@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/885903002/290001
Message was sent while issue was closed.
Committed patchset #16 (id:290001) as https://skia.googlesource.com/skia/+/e70de9e4f0b7bf73f7cd1a20dbabcb233ffbb7f1
Message was sent while issue was closed.
possible perf regression associated with this change... https://perf.skia.org/#2875 any chance this CL is responsible?
Message was sent while issue was closed.
On 2015/02/27 19:27:39, reed1 wrote: > possible perf regression associated with this change... > > https://perf.skia.org/#2875 > > any chance this CL is responsible? The effect spans across gpu, 565, and 8888, so I think we can rule out anything Ganesh-y. But the effect is also visible on SKPs that have no text, like desk_tigersvg.skp. I wouldn't think that this CL could affect that. What about https://skia.googlesource.com/skia/+/a9061de9ff8d5b9545e5d1ef7a6e4054e1907c95 ? I don't see anything bad there jump out at me, but a SkChunkAlloc regression could certainly affect a bunch of different configs.
Message was sent while issue was closed.
adding author of chunkalloc change
Message was sent while issue was closed.
On 2015/02/27 19:39:29, reed1 wrote: > adding author of chunkalloc change Looking harder, these are almost all Nexus 5 lines. This might just be an alert telling us the Nexus 5 was underclocked for heat this run. Let's keep an eye on at as we get more runs in... might not have anything to do with any of those CLs. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
