Open Bug 742134 Opened 8 years ago Updated 8 years ago

Try to avoid doing IO during text drawing

Categories

(Core :: Graphics, defect)

x86
macOS
defect
Not set

Tracking

()

Tracking Status
blocking-fennec1.0 --- -

People

(Reporter: jrmuizel, Assigned: jrmuizel)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

We currently see read() calls during cairo_show_glyphs. It would be nice to not do this.
#0  0xafd0b450 in read () from /home/jrmuizel/src/gdb-root/moz-gdb/lib/353549C2A70D00EC/system/lib/libc.so
#1  0xafd19c18 in __sread () from /home/jrmuizel/src/gdb-root/moz-gdb/lib/353549C2A70D00EC/system/lib/libc.so
#2  0xafd1987a in __srefill () from /home/jrmuizel/src/gdb-root/moz-gdb/lib/353549C2A70D00EC/system/lib/libc.so
#3  0xafd18e22 in fseeko () from /home/jrmuizel/src/gdb-root/moz-gdb/lib/353549C2A70D00EC/system/lib/libc.so
#4  0xafd18e96 in fseek () from /home/jrmuizel/src/gdb-root/moz-gdb/lib/353549C2A70D00EC/system/lib/libc.so
#5  0x48eb2b1e in ft_ansi_stream_io () from /home/jrmuizel/mozilla/maple/objdir-droid/dist/bin/libxul.so
#6  0x48e6360a in FT_Stream_Seek () from /home/jrmuizel/mozilla/maple/objdir-droid/dist/bin/libxul.so
#7  0x48e91e22 in tt_face_goto_table () from /home/jrmuizel/mozilla/maple/objdir-droid/dist/bin/libxul.so
#8  0x48e69632 in tt_loader_init () from /home/jrmuizel/mozilla/maple/objdir-droid/dist/bin/libxul.so
#9  0x48e6979c in TT_Load_Glyph () from /home/jrmuizel/mozilla/maple/objdir-droid/dist/bin/libxul.so
#10 0x48e66696 in tt_glyph_load () from /home/jrmuizel/mozilla/maple/objdir-droid/dist/bin/libxul.so
#11 0x48e5d4f6 in FT_Load_Glyph () from /home/jrmuizel/mozilla/maple/objdir-droid/dist/bin/libxul.so
#12 0x48b89804 in _cairo_ft_scaled_glyph_init (abstract_font=0x4cbb6780, scaled_glyph=<optimized out>, info=CAIRO_SCALED_GLYPH_INFO_METRICS) at /home/jrmuizel/mozilla/maple/gfx/cairo/cairo/src/cairo-ft-font.c:2164
This happens when double-tap zooming out on nytimes.com the font in question is DroidSans.ttf
blocking-fennec1.0: --- → ?
This seems to imply that Freetype re-reads glyph data from the font file when it needs to rasterize new glyphs at a size that it hasn't previously rasterized (and cached). Depending on disk caching behavior, this might be able to re-use cached data from when the previous size was rasterized, or might hit the filesystem.

Avoiding this would presumably require the original glyph data for the face to be cached somewhere, so that it could be re-used to rasterize at different sizes without referring back to the file. Can we afford the memory to do that in all cases?

Maybe it would be helpful to mmap() the font file in gfxFT2FontEntry, and then use FT_New_Memory_Face instead of FT_New_Face to create the FT_Face we need. As long as the mapped region doesn't get paged out, this should avoid hitting the disk to rasterize a new size; but if memory is tight, it would fall back gracefully.

Oh, one other thought: do you only see this when zooming *out*, never when zooming *in*? If so, it might be that new glyphs are required that haven't previously be loaded at all. That'd be a different issue than re-reading the glyph data to rasterize a new size; it would just mean that Freetype doesn't eagerly read the data for *all* glyphs but only loads them on demand, which seems pretty reasonable. All the same, mmap()ing the font file might be a useful approach, rather than making FT go through stream-based i/o.
(In reply to Jonathan Kew (:jfkthame) from comment #3)
> This seems to imply that Freetype re-reads glyph data from the font file
> when it needs to rasterize new glyphs at a size that it hasn't previously
> rasterized (and cached). Depending on disk caching behavior, this might be
> able to re-use cached data from when the previous size was rasterized, or
> might hit the filesystem.

It feels like we might be thrashing our glyph cache. For the fully zoomed out case it feels like we should be able to keep all of those glyphs in cache even when we're at different zoom levels.

> Avoiding this would presumably require the original glyph data for the face
> to be cached somewhere, so that it could be re-used to rasterize at
> different sizes without referring back to the file. Can we afford the memory
> to do that in all cases?

Maybe, we'll have to measure the costs of rereading vs. caching.

> Maybe it would be helpful to mmap() the font file in gfxFT2FontEntry, and
> then use FT_New_Memory_Face instead of FT_New_Face to create the FT_Face we
> need. As long as the mapped region doesn't get paged out, this should avoid
> hitting the disk to rasterize a new size; but if memory is tight, it would
> fall back gracefully.

This sounds like it might be worth trying.

> Oh, one other thought: do you only see this when zooming *out*, never when
> zooming *in*? If so, it might be that new glyphs are required that haven't
> previously be loaded at all. That'd be a different issue than re-reading the
> glyph data to rasterize a new size; it would just mean that Freetype doesn't
> eagerly read the data for *all* glyphs but only loads them on demand, which
> seems pretty reasonable. All the same, mmap()ing the font file might be a
> useful approach, rather than making FT go through stream-based i/o.

I'll take a closer look tomorrow, but I saw this when zooming out to the full page. I saw it again the second time I did it.
Here's a quick hack to try and use mmap() instead of stream i/o for fonts used by the content process. It'd be interesting to see if this affects the behavior you're seeing.

There's a tryserver build with this patch at https://tbpl.mozilla.org/?tree=Try&rev=015ac859a3b4.

If we want to go this way, the patch wants a bit more love - e.g. it doesn't ever bother to unmap the fonts, even at shutdown. But it may be enough to get an idea of whether this will improve things.
blocking-fennec1.0: ? → beta+
Assignee: nobody → jfkthame
About the glyph cache:
Glyphs are cached using the scaled_font. i.e. a new scaled_font will rerasterize all of it's glyphs. There's also a scaled_font cache. First we should check that the scaled fonts we're creating when we zoom out are hitting the scaled_font cache. If they are, we should ensure that we still have the rasterized glyphs in the glyph cache.
Comment on attachment 612169 [details] [diff] [review]
WIP - use mmap to open fonts for freetype

Jeff, could you try running with this patch (see above for tryserver build, or make your own) and let me know if it's worth pursuing?
Attachment #612169 - Flags: feedback?(jmuizelaar)
(In reply to Jeff Muizelaar [:jrmuizel] from comment #6)
> About the glyph cache:
> Glyphs are cached using the scaled_font. i.e. a new scaled_font will
> rerasterize all of it's glyphs. There's also a scaled_font cache. First we
> should check that the scaled fonts we're creating when we zoom out are
> hitting the scaled_font cache. If they are, we should ensure that we still
> have the rasterized glyphs in the glyph cache.

During the zoom, is it possible/plausible that we trigger a reflow at some more-or-less random intermediate size, not just at the final fully-zoomed-out size? If so, it'd be very likely that we suddenly require glyphs at a size we haven't previously used.
(In reply to Jonathan Kew (:jfkthame) from comment #7)
> Comment on attachment 612169 [details] [diff] [review]
> WIP - use mmap to open fonts for freetype
> 
> Jeff, could you try running with this patch (see above for tryserver build,
> or make your own) and let me know if it's worth pursuing?

My initial investigation does not show any improvement with this patch. I'll try to get more concrete numbers.
I had a closer investigation and it looks like we're not hitting the scaled_font_cache because we end up with scaled font ctm's that are subtly different.

e.g.:

xx = 0.48969808220863342
xx = 0.48969796299934387
xx = 0.48969802260398865

Because of this we end up creating new scaled_fonts all the time and thus ignore our glyph cache.
This seems to be caused by us passing in slightly different values to nsDOMWindowUtils::SetResolution()
As far as I know passing in slightly different values to SetResolution is pretty much unavoidable. In other places where we do float comparisons, we take into account a fuzz amount, can we do that here too? e.g. Math.abs(valuea - valueb) < 1e-5 instead of valuea == valueb.
(In reply to Kartikaya Gupta (:kats) from comment #12)
> As far as I know passing in slightly different values to SetResolution is
> pretty much unavoidable. In other places where we do float comparisons, we
> take into account a fuzz amount, can we do that here too? e.g.
> Math.abs(valuea - valueb) < 1e-5 instead of valuea == valueb.

I'm seeing different SetResolution values for the fully zoomed out case. Shouldn't the fully zoomed out resolution always be the same?
No, because when we determine the resolution is based on one or more of: (1) the amount the user's fingers move during a pinch, which is variable, (2) the last page size we have, which itself is a function of the previous resolution we were at, (3) the size of the block we're zooming in to which again is a function of the previous resolution.

It's really only when the page first loads that we have a reliable resolution because then it's based on the device pixel resolution of the screen (fixed quantity) and the CSS viewport (reliable CSS pixel value).
Jonathan, can I assume you'll submit this upstream as well?
(In reply to Brad Lassey [:blassey] from comment #15)
> Jonathan, can I assume you'll submit this upstream as well?

Not relevant at this point; the patch I posted - which may not help significantly anyhow, as the real issue is that we're using fractionally-different scaling factors and hence re-rasterizing - doesn't touch upstream code.

(If we do something within cairo or freetype, that'd be different - but we may want to address the scaling issue within gecko code - maybe quantizing scale factors or font matrices or something could help.)
(In reply to Kartikaya Gupta (:kats) from comment #14)
> No, because when we determine the resolution is based on one or more of: (1)
> the amount the user's fingers move during a pinch, which is variable, (2)
> the last page size we have, which itself is a function of the previous
> resolution we were at, (3) the size of the block we're zooming in to which
> again is a function of the previous resolution.
> 
> It's really only when the page first loads that we have a reliable
> resolution because then it's based on the device pixel resolution of the
> screen (fixed quantity) and the CSS viewport (reliable CSS pixel value).

It seems to me that it ought to be possible to recognize the fully-zoomed-out case, as mentioned by Jeff in comment 13, and use a consistent resolution based on these factors, rather than something that's been computed iteratively through a series of unpredictable steps of zooming in and out. That would help us to hit the font caches more often instead of creating new scaled_fonts.
(In reply to Jonathan Kew (:jfkthame) from comment #17)
> (In reply to Kartikaya Gupta (:kats) from comment #14)
> > No, because when we determine the resolution is based on one or more of: (1)
> > the amount the user's fingers move during a pinch, which is variable, (2)
> > the last page size we have, which itself is a function of the previous
> > resolution we were at, (3) the size of the block we're zooming in to which
> > again is a function of the previous resolution.
> > 
> > It's really only when the page first loads that we have a reliable
> > resolution because then it's based on the device pixel resolution of the
> > screen (fixed quantity) and the CSS viewport (reliable CSS pixel value).
> 
> It seems to me that it ought to be possible to recognize the
> fully-zoomed-out case, as mentioned by Jeff in comment 13, and use a
> consistent resolution based on these factors, rather than something that's
> been computed iteratively through a series of unpredictable steps of zooming
> in and out. That would help us to hit the font caches more often instead of
> creating new scaled_fonts.

I have some plans about fixing this that basically involve us using the right units in more situations to avoid using slightly different zooms. I'll file some bugs about them.
Depends on: 744901
Depends on: 744916, 744939
Jeff is actually working on this.
Assignee: jfkthame → jmuizelaar
Jeff, should we beta+ your zoom factor patches instead of this bug?
blocking-fennec1.0: beta+ → -
You need to log in before you can comment on or make changes to this bug.