Qt
  1. Qt
  2. QTBUG-59549

No caching in QWindowsFontEngine::getGlyphBearings

    Details

    • Type: Bug Bug
    • Status: Reported
    • Priority: P3: Somewhat important P3: Somewhat important
    • Resolution: Unresolved
    • Affects Version/s: 5.6.0
    • Fix Version/s: None
    • Component/s: GUI: Font handling
    • Labels:
      None
    • Environment:

      Windows

      Description

      The function QWindowsFontEngine::getGlyphBearings in qwindowsfontengine.cpp calls GetCharABCWidthsI to get information about the font a lot. However there is no caching at all, which results in a performance bottleneck (especially when calling functions like QPainter::drawText often).

      When hooking GetCharABCWidthsI to manually cache based on the HFONT pointer, the bottleneck disappears. See here for my (very crude) implementation.

      Some numbers (after running my application for about 20 seconds):

      HGDIOBJ: 3B0A22E9
      count: 4, hits: 2, misses: 2
      
      HGDIOBJ: A70A1E93
      count: 1374, hits: 1348, misses: 26
      
      HGDIOBJ: 000A1F1B
      count: 140039, hits: 139925, misses: 114
      
      HGDIOBJ: 7C0A2302
      count: 581, hits: 550, misses: 31
      

      Hotspots before:

      Hotspots after:

      No reviews matched the request. Check your Options in the drop-down menu of this sections header.

        Activity

        Hide
        Eskil Abrahamsen Blomfeldt added a comment -

        Could you share a code example that demonstrates this?

        The getGlyphBearings() function should only be called during the layout of word wrapped text, and only at locations where the the current glyph advance minus minimum right bearing (which is negative, and cached) would cause the glyph to be too wide for the layout. That is: It shouldn't happen often, which is why we didn't consider it a bottleneck.

        I am interested in seeing where this logic is flawed in your use case.

        Show
        Eskil Abrahamsen Blomfeldt added a comment - Could you share a code example that demonstrates this? The getGlyphBearings() function should only be called during the layout of word wrapped text, and only at locations where the the current glyph advance minus minimum right bearing (which is negative, and cached) would cause the glyph to be too wide for the layout. That is: It shouldn't happen often, which is why we didn't consider it a bottleneck. I am interested in seeing where this logic is flawed in your use case.
        Hide
        Duncan Ogilvie added a comment -

        Most of the calls come from here (Qt::TextBypassShaping doesn't appear to influence the performance): https://github.com/x64dbg/x64dbg/blob/development/src/gui/Src/Utils/RichTextPainter.cpp#L45

        I am aware that calling QPainter::drawText often is not particularly smart to print custom text highlighting but it is the easiest and the fact that it is slow because there is no caching for something that could/should definitely be cached is why I reported this as a bug.

        A very small part comes from within Qt:

        Show
        Duncan Ogilvie added a comment - Most of the calls come from here (Qt::TextBypassShaping doesn't appear to influence the performance): https://github.com/x64dbg/x64dbg/blob/development/src/gui/Src/Utils/RichTextPainter.cpp#L45 I am aware that calling QPainter::drawText often is not particularly smart to print custom text highlighting but it is the easiest and the fact that it is slow because there is no caching for something that could/should definitely be cached is why I reported this as a bug. A very small part comes from within Qt:
        Hide
        Eskil Abrahamsen Blomfeldt added a comment -

        Unless that text changes every frame, try using QStaticText, or at least a QTextLayout with caching enabled. Calling drawText() and requiring the same text to be itemized, shaped and laid out every frame is extremely inefficient, even if we introduced caching of the bearing of all the glyphs.

        Show
        Eskil Abrahamsen Blomfeldt added a comment - Unless that text changes every frame, try using QStaticText, or at least a QTextLayout with caching enabled. Calling drawText() and requiring the same text to be itemized, shaped and laid out every frame is extremely inefficient, even if we introduced caching of the bearing of all the glyphs.
        Hide
        Duncan Ogilvie added a comment -

        The text can be changed every frame. I tried using QStaticText for individual words, but it is much, much slower than calling drawText directly.

        QTextLayout is also slower (probably because I draw single words with different colors and backgrounds and not whole paragraphs).

        I don't really see the argument against adding a cache though. The 144 glyphs cached hardly take any memory and the performance is increased greatly.

        Show
        Duncan Ogilvie added a comment - The text can be changed every frame. I tried using QStaticText for individual words, but it is much, much slower than calling drawText directly. QTextLayout is also slower (probably because I draw single words with different colors and backgrounds and not whole paragraphs). I don't really see the argument against adding a cache though. The 144 glyphs cached hardly take any memory and the performance is increased greatly.
        Hide
        Eskil Abrahamsen Blomfeldt added a comment -

        It wasn't an argument against it, I just wanted to point out that for text which doesn't update every frame, you will be wasting huge amounts of resources by using drawText() here, and the only reason to avoid keeping QTextLayouts around would be for you to save a little bit of memory. QTextLayout is what will be used by the drawText() internally, so there is no way that will be slower.

        If the text is updated often, the cache would still be useful of course. I don't want to cache every glyph in the font, though, since some CJK fonts are huge. So we would need to think of some caching strategy that only caches the most used subset of 256 glyphs or something like that.

        Show
        Eskil Abrahamsen Blomfeldt added a comment - It wasn't an argument against it, I just wanted to point out that for text which doesn't update every frame, you will be wasting huge amounts of resources by using drawText() here, and the only reason to avoid keeping QTextLayouts around would be for you to save a little bit of memory. QTextLayout is what will be used by the drawText() internally, so there is no way that will be slower. If the text is updated often, the cache would still be useful of course. I don't want to cache every glyph in the font, though, since some CJK fonts are huge. So we would need to think of some caching strategy that only caches the most used subset of 256 glyphs or something like that.
        Hide
        Duncan Ogilvie added a comment - - edited

        Alright Basically what I did when trying QTextLayout was copying (as far as possible, some things were private) the implementation of QPainter::drawText and in all my measurements this resulted in a slowdown and not a speedup. However I'm far from a Qt expert and I threw away all my code after I concluded it wouldn't be worth it so I could be wrong.

        In my use case the text could update every frame (even though it doesn't usually) the issue is that text is drawn in a row-based system where it is not possible to lay out and draw a lot of vertical space at once. It is my expectation that when rewriting the table code to work column-based it will be possible to achieve a great speedup but for now I'm stuck with QPainter::drawText.

        As for the cache, we developed a cache for QFontMetrics::width(QString) (this was showing up in earlier benchmarks as a bottleneck) and it uses 0x10000 - 0xE000 + 0xD800 = 63488 bytes of memory (to store the width of the most common characters in the font). You can find the code here: https://github.com/x64dbg/x64dbg/blob/development/src/gui/Src/Utils/CachedFontMetrics.h

        Another idea would be to simply cache everything that reaches a certain hit/miss ratio and simply rotate out low-ratio/cold entries when memory limits are reached...

        Show
        Duncan Ogilvie added a comment - - edited Alright Basically what I did when trying QTextLayout was copying (as far as possible, some things were private) the implementation of QPainter::drawText and in all my measurements this resulted in a slowdown and not a speedup. However I'm far from a Qt expert and I threw away all my code after I concluded it wouldn't be worth it so I could be wrong. In my use case the text could update every frame (even though it doesn't usually) the issue is that text is drawn in a row-based system where it is not possible to lay out and draw a lot of vertical space at once. It is my expectation that when rewriting the table code to work column-based it will be possible to achieve a great speedup but for now I'm stuck with QPainter::drawText. As for the cache, we developed a cache for QFontMetrics::width(QString) (this was showing up in earlier benchmarks as a bottleneck) and it uses 0x10000 - 0xE000 + 0xD800 = 63488 bytes of memory (to store the width of the most common characters in the font). You can find the code here: https://github.com/x64dbg/x64dbg/blob/development/src/gui/Src/Utils/CachedFontMetrics.h Another idea would be to simply cache everything that reaches a certain hit/miss ratio and simply rotate out low-ratio/cold entries when memory limits are reached...
        Hide
        Eskil Abrahamsen Blomfeldt added a comment -

        In that case, I recommend that you create the QTextLayouts outside the loop and only update them then the text changes. You should be able to gain a lot of performance from this. Make sure you set the cache to be enabled on the QTextLayout. QStaticText should be even faster, but there are some gotchas there which could cause it to fall back to regular drawText(), so maybe you are hitting some of those.

        Show
        Eskil Abrahamsen Blomfeldt added a comment - In that case, I recommend that you create the QTextLayouts outside the loop and only update them then the text changes. You should be able to gain a lot of performance from this. Make sure you set the cache to be enabled on the QTextLayout. QStaticText should be even faster, but there are some gotchas there which could cause it to fall back to regular drawText(), so maybe you are hitting some of those.
        Hide
        Duncan Ogilvie added a comment -

        Thanks for your advice! I will try creating QTextLayouts to speed up the drawing process and report back on the performance here...

        Show
        Duncan Ogilvie added a comment - Thanks for your advice! I will try creating QTextLayouts to speed up the drawing process and report back on the performance here...

          People

          • Assignee:
            Eskil Abrahamsen Blomfeldt
            Reporter:
            Duncan Ogilvie
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:

              Gerrit Reviews

              There are no open Gerrit changes