Bug 178750 - Cache glyphs (using display lists) when painting at high frequency
Summary: Cache glyphs (using display lists) when painting at high frequency
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Said Abou-Hallawa
URL:
Keywords: InRadar
Depends on: 184182 184311 184336
Blocks:
  Show dependency treegraph
 
Reported: 2017-10-24 14:22 PDT by Dave Hyatt
Modified: 2018-04-11 13:29 PDT (History)
13 users (show)

See Also:


Attachments
Work in progress (33.92 KB, patch)
2017-10-24 14:23 PDT, Dave Hyatt
no flags Details | Formatted Diff | Diff
Patch (34.18 KB, patch)
2017-10-26 09:33 PDT, Dave Hyatt
no flags Details | Formatted Diff | Diff
Patch (34.17 KB, patch)
2017-10-26 10:58 PDT, Dave Hyatt
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews123 for ios-simulator-wk2 (11.06 MB, application/zip)
2017-10-26 13:01 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-elcapitan-wk2 (1.54 MB, application/zip)
2017-10-26 18:00 PDT, Build Bot
no flags Details
Patch (49.21 KB, patch)
2017-10-30 11:40 PDT, Dave Hyatt
no flags Details | Formatted Diff | Diff
Patch (not for review) (68.29 KB, patch)
2017-11-08 13:47 PST, Dave Hyatt
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews104 for mac-elcapitan-wk2 (1.75 MB, application/zip)
2017-11-08 14:38 PST, Build Bot
no flags Details
Archive of layout-test-results from ews103 for mac-elcapitan (2.69 MB, application/zip)
2017-11-08 14:42 PST, Build Bot
no flags Details
Archive of layout-test-results from ews126 for ios-simulator-wk2 (1.74 MB, application/zip)
2017-11-08 15:08 PST, Build Bot
no flags Details
Archive of layout-test-results from ews116 for mac-elcapitan (3.61 MB, application/zip)
2017-11-08 15:25 PST, Build Bot
no flags Details
Patch (78.84 KB, patch)
2017-11-08 17:47 PST, Dave Hyatt
no flags Details | Formatted Diff | Diff
Patch (52.28 KB, patch)
2018-03-27 11:50 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (53.70 KB, patch)
2018-03-27 12:47 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (57.18 KB, patch)
2018-03-28 11:12 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (59.09 KB, patch)
2018-03-30 13:14 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
run-perf-tests results (Mac) (179.33 KB, application/zip)
2018-04-02 11:31 PDT, Said Abou-Hallawa
no flags Details
Patch (60.28 KB, patch)
2018-04-02 13:20 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (60.24 KB, patch)
2018-04-02 14:30 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (36.03 KB, patch)
2018-04-08 16:41 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (34.70 KB, patch)
2018-04-09 18:11 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews112 for mac-sierra (2.93 MB, application/zip)
2018-04-09 21:11 PDT, Build Bot
no flags Details
Patch (34.71 KB, patch)
2018-04-10 11:16 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (34.73 KB, patch)
2018-04-10 13:35 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews106 for mac-sierra-wk2 (1001.39 KB, application/zip)
2018-04-10 14:39 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews103 for mac-sierra (2.56 MB, application/zip)
2018-04-10 14:50 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews125 for ios-simulator-wk2 (2.39 MB, application/zip)
2018-04-10 15:08 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews116 for mac-sierra (3.17 MB, application/zip)
2018-04-10 15:25 PDT, Build Bot
no flags Details
Patch (34.49 KB, patch)
2018-04-10 15:28 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (33.39 KB, patch)
2018-04-10 17:54 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dave Hyatt 2017-10-24 14:22:17 PDT
[Experiment] Cache glyphs when painting at high frequency
Comment 1 Dave Hyatt 2017-10-24 14:23:09 PDT
Created attachment 324721 [details]
Work in progress
Comment 2 Dave Hyatt 2017-10-26 09:33:22 PDT
Created attachment 325018 [details]
Patch
Comment 3 Build Bot 2017-10-26 09:35:10 PDT
Attachment 325018 [details] did not pass style-queue:


ERROR: Source/WebCore/rendering/InlineTextBox.h:26:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/rendering/InlineTextBox.h:43:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/rendering/TextPainter.cpp:102:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 3 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Dave Hyatt 2017-10-26 10:58:49 PDT
Created attachment 325031 [details]
Patch
Comment 5 Radar WebKit Bug Importer 2017-10-26 11:10:54 PDT
<rdar://problem/35201729>
Comment 6 Simon Fraser (smfr) 2017-10-26 11:37:39 PDT
I think we need to consider how this fits in with display lists. You could imagine that this cache is a cache of DrawGlyphs items. I think we're going to start moving towards using DisplayList items for subsets of drawing.
Comment 7 Build Bot 2017-10-26 13:01:07 PDT
Comment on attachment 325031 [details]
Patch

Attachment 325031 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/5001314

New failing tests:
fast/text/simple-line-layout-hyphenation-limit-lines-accross-words.html
Comment 8 Build Bot 2017-10-26 13:01:09 PDT
Created attachment 325045 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6
Comment 9 Build Bot 2017-10-26 18:00:12 PDT
Comment on attachment 325031 [details]
Patch

Attachment 325031 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/5005294

New failing tests:
fast/text/simple-line-layout-hyphenation-limit-lines-accross-words.html
Comment 10 Build Bot 2017-10-26 18:00:13 PDT
Created attachment 325089 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 11 Dave Hyatt 2017-10-27 08:52:41 PDT
(In reply to Simon Fraser (smfr) from comment #6)
> I think we need to consider how this fits in with display lists. You could
> imagine that this cache is a cache of DrawGlyphs items. I think we're going
> to start moving towards using DisplayList items for subsets of drawing.

I tried using DrawGlyphs, but the problem is a single GlyphBuffer can ultimately result in a bunch of drawGlyphs calls, any time a different font in the cascade is used. This means you'd have to cache a list of DrawGlyphs items

I'm not convinced that's better than just holding on to a GlyphBuffer for this particular optimization.
Comment 12 Dave Hyatt 2017-10-27 09:24:42 PDT
Will implement it just to see how it compares though.
Comment 13 Dave Hyatt 2017-10-30 11:40:50 PDT
Created attachment 325363 [details]
Patch
Comment 14 Simon Fraser (smfr) 2017-10-30 11:51:18 PDT
Comment on attachment 325363 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=325363&action=review

> Source/WebCore/platform/graphics/CachedGlyphDrawInfo.h:37
> +    : m_textOrigin(origin)

indent this

> Source/WebCore/platform/graphics/FontCascade.cpp:340
> +    CachedGlyphDrawInfo* glyphDrawInfo = new CachedGlyphDrawInfo(point);

This needs to be a unique_ptr<>, as does the return value.

> Source/WebCore/platform/graphics/FontCascade.cpp:353
> +        if (context.isRecording())
> +            context.displayListRecorder()->recordDisplayListItem(item.copyRef());

Don't really like going straight to the recorder; we'll have to decide how we want to expose this. Maybe just make a new GraphicsContext with a recording backend?

> Source/WebCore/platform/graphics/FontCascade.h:51
> -
> +    

Whitespace

> Source/WebCore/platform/graphics/GraphicsContext.cpp:664
> +    

whitespace

> Source/WebCore/platform/graphics/GraphicsContext.h:248
> + 

whitespace

> Source/WebCore/platform/graphics/GraphicsContext.h:270
> +    

whitespace

> Source/WebCore/rendering/InlineTextBox.cpp:67
> +    

WS

> Source/WebCore/rendering/InlineTextBox.cpp:531
> +    if (!gTextBoxesWithGlyphs)
> +        gTextBoxesWithGlyphs = new InlineTextBoxGlyphsMap;

This should be a NEVER_DESTROYED and be returned by a helper function.

> Source/WebCore/rendering/RenderLayer.cpp:4310
> +    bool paintingFrequently = false;
> +    if (shouldPaintContent) {
> +        m_paintFrequencyInfo.paintedCachedResource = false;
> +        if (m_paintFrequencyInfo.totalPaintsSinceFirst > 0) {
> +            MonotonicTime now = MonotonicTime::now();
> +            if ((now - m_paintFrequencyInfo.lastPaintTime).seconds() > PAINT_FREQUENCY_SECONDS_IDLE_THRESHOLD) {
> +                m_paintFrequencyInfo.firstPaintTime = m_paintFrequencyInfo.lastPaintTime = MonotonicTime();
> +                m_paintFrequencyInfo.totalPaintsSinceFirst = 0;
> +            } else if ((m_paintFrequencyInfo.totalPaintsSinceFirst / (now - m_paintFrequencyInfo.firstPaintTime).seconds()) > PAINT_FREQUENCY_FPS_THRESHOLD)
> +                paintingFrequently = true;
> +        }
> +    }

Maybe split this into a separate patch

> Source/WebCore/rendering/RenderLayer.h:1033
> +        unsigned totalPaintsSinceFirst { 0 };

Isn't "total" always "since first"?

> Source/WebCore/rendering/TextPainter.h:92
> +    const CachedGlyphDrawInfo* m_drawGlyphsInfo { nullptr };

Is this an owning reference?
Comment 15 Dave Hyatt 2017-11-08 13:47:42 PST
Created attachment 326363 [details]
Patch (not for review)
Comment 16 Build Bot 2017-11-08 13:49:56 PST
Attachment 326363 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/FontCascade.h:52:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/rendering/SimpleLineLayout.h:41:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/graphics/FontCascade.cpp:320:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 3 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 17 Build Bot 2017-11-08 14:38:43 PST
Comment on attachment 326363 [details]
Patch (not for review)

Attachment 326363 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/5152981

Number of test failures exceeded the failure limit.
Comment 18 Build Bot 2017-11-08 14:38:44 PST
Created attachment 326379 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 19 Build Bot 2017-11-08 14:42:32 PST
Comment on attachment 326363 [details]
Patch (not for review)

Attachment 326363 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/5152964

New failing tests:
imported/blink/fast/canvas/canvas-state-stack-simple.html
css3/filters/backdrop/add-remove-add-backdrop-filter.html
imported/blink/http/tests/webfont/animation-assert.html
fast/repaint/fixed.html
fast/frames/iframe-translucent-background.html
Comment 20 Build Bot 2017-11-08 14:42:33 PST
Created attachment 326381 [details]
Archive of layout-test-results from ews103 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 21 Build Bot 2017-11-08 15:08:09 PST
Comment on attachment 326363 [details]
Patch (not for review)

Attachment 326363 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/5153337

Number of test failures exceeded the failure limit.
Comment 22 Build Bot 2017-11-08 15:08:10 PST
Created attachment 326390 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6
Comment 23 Build Bot 2017-11-08 15:25:12 PST
Comment on attachment 326363 [details]
Patch (not for review)

Attachment 326363 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/5153262

New failing tests:
imported/blink/fast/canvas/canvas-state-stack-simple.html
css3/filters/backdrop/add-remove-add-backdrop-filter.html
imported/blink/http/tests/webfont/animation-assert.html
fast/frames/iframe-translucent-background.html
Comment 24 Build Bot 2017-11-08 15:25:13 PST
Created attachment 326393 [details]
Archive of layout-test-results from ews116 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 25 Dave Hyatt 2017-11-08 17:47:13 PST
Created attachment 326414 [details]
Patch
Comment 26 Simon Fraser (smfr) 2017-11-08 18:17:14 PST
Comment on attachment 326414 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=326414&action=review

> Source/WebCore/platform/graphics/FontCascade.cpp:319
> +    // FIXME: Use the fast code path once it handles partial runs with kerning and ligatures. See http://webkit.org/b/100050
> +    CodePath codePathToUse = codePath(run);
> +    if (codePathToUse != Complex && (enableKerning() || requiresShaping()) && (from || destination != run.length()))
> +        codePathToUse = Complex;

Is this copying an existing FIXME? Can we share code?

> Source/WebCore/platform/graphics/displaylists/DisplayListFragment.cpp:57
> +    for (size_t i = 0; i < numItems; ++i) {
> +        auto& item = m_displayList.list()[i].get();
> +        
> +        if (is<DrawingItem>(item)) {
> +            DrawingItem& drawingItem = downcast<DrawingItem>(item);
> +            drawingItem.move(delta);
> +        }
> +    }

Hmm, I wonder if we could just wrap the list in a translate/untranslate. Then all the elements don't need move().

> Source/WebCore/platform/graphics/displaylists/DisplayListFragment.h:42
> +    Fragment(const FloatPoint&);

Would be nice to name the argument here.

> Source/WebCore/platform/graphics/displaylists/DisplayListFragment.h:46
> +    FloatPoint origin() const { return m_origin; }
> +    void setOrigin(const FloatPoint&);

What's this origin relative to?

> Source/WebCore/platform/graphics/displaylists/DisplayListReplayer.cpp:69
> +            recorder->recordDisplayListItem(item);

It's pretty confusing that a Replayer is recording!

> Source/WebCore/rendering/InlineTextBox.cpp:547
> +                auto glyphsPtr = font.generateDisplayListFragment(context, textRun, textOrigin, 0, length, FontCascade::CustomFontNotReadyAction::DoNotPaintIfFontNotReady);

glyphsPtr isn't a pointer any more, it's a fragment.

> Source/WebCore/rendering/SimpleLineLayout.h:100
> +    void setDisplayListFragment(const Run*, std::unique_ptr<DisplayList::Fragment>) const;

This should be a std::unique_ptr<DisplayList::Fragment>&& I think

> Source/WebCore/rendering/SimpleLineLayoutFunctions.cpp:143
> +        auto glyphs = layout.displayListFragment(&run.simpleRun());
> +        if (paintInfo.enclosingSelfPaintingLayer()) {
> +            if (paintInfo.enclosingSelfPaintingLayer()->paintingFrequently()) {
> +                if (!glyphs) {
> +                    auto glyphPtr = style.fontCascade().generateDisplayListFragment(paintInfo.context(), textRun, textOrigin, 0, run.text().length(), FontCascade::CustomFontNotReadyAction::DoNotPaintIfFontNotReady);
> +                    if (glyphPtr) {
> +                        layout.setDisplayListFragment(&run.simpleRun(), WTFMove(glyphPtr));
> +                        glyphs = glyphPtr.get();
> +                    }
> +                }
> +            } else if (glyphs) {
> +                layout.setDisplayListFragment(&run.simpleRun(), nullptr);
> +                glyphs = nullptr;
> +            }
> +            paintInfo.enclosingSelfPaintingLayer()->paintingCacheableResource();
> +        }
> +        if (glyphs)
> +            textPainter.setDisplayListFragment(glyphs);

It's a shame that the call to style.fontCascade().generateDisplayListFragment() is so far removed from the call to getGlyphsAndAdvances.... Ideally these would be next to each other (you either have a cached glyph run or compute a new one).

I guess you don't want to record everything that happens under textPainter.paint(), because that's things other than just text?
Comment 27 Dave Hyatt 2017-11-09 11:00:42 PST
Yeah TextPainter does lots of other stuff besides just draw glyphs. Trying to limit the scope of the caching to just the draw glyphs commands.
Comment 28 Said Abou-Hallawa 2018-03-27 11:50:07 PDT
Created attachment 336601 [details]
Patch
Comment 29 Said Abou-Hallawa 2018-03-27 11:53:36 PDT
I verified that replaying the DisplayList of the glyph cache happens very often while resizing the window and scrolling the page (~50,000 times in few seconds).
Comment 30 Said Abou-Hallawa 2018-03-27 12:00:36 PDT
I ran the Design MotionMark test with the attached patch on different hardware. I got between 2-21% progression. The highest progression was on the more memory limited hardware.
Comment 31 Said Abou-Hallawa 2018-03-27 12:24:23 PDT
When opening nytimes.com on 2560x1440 display and scrolling the page and resizing the window:

The maximum size of the glyph cache HashMap = 846 entries
The maximum size of the glyph cache HashMap (in bytes) = 2,142,320 bytes
Comment 32 Said Abou-Hallawa 2018-03-27 12:47:32 PDT
Created attachment 336612 [details]
Patch
Comment 33 Brent Fulgham 2018-03-27 12:49:49 PDT
(In reply to Said Abou-Hallawa from comment #30)
> I ran the Design MotionMark test with the attached patch on different
> hardware. I got between 2-21% progression. The highest progression was on
> the more memory limited hardware.

Wow! Nice! :-)
Comment 34 zalan 2018-03-27 12:57:23 PDT
Comment on attachment 336612 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=336612&action=review

> Source/WebCore/rendering/SimpleLineLayout.cpp:986
> +Layout::~Layout()
> +{
> +    for (unsigned i = 0; i < runCount(); ++i)
> +        TextPainter::glyphRunDisplayListCache().remove(&runAt(i));
> +}

This is a line breaking class. It should not be aware of any text painting related functionality at all.
Comment 35 zalan 2018-03-27 13:19:44 PDT
I think this cache should be hooked up with the MemoryPressureHandler and purge under memory pressure.
Comment 36 Said Abou-Hallawa 2018-03-28 11:12:13 PDT
Created attachment 336679 [details]
Patch
Comment 37 Said Abou-Hallawa 2018-03-28 11:13:48 PDT
Comment on attachment 336612 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=336612&action=review

>> Source/WebCore/rendering/SimpleLineLayout.cpp:986
>> +}
> 
> This is a line breaking class. It should not be aware of any text painting related functionality at all.

I moved this code to SimpleLineLayout::simpleLineLayoutWillBeDeleted() in SImpleLineLayouFunctions.cpp.
Comment 38 Daniel Bates 2018-03-29 00:12:47 PDT
Comment on attachment 336679 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=336679&action=review

> Source/WebCore/rendering/InlineTextBox.h:194
> +    TextRun createTextRun(const String&) const;

Please do not make this change without fixing the lifetime issue with TextRun. Taking a non const lvalue reference is intentional and a crude and indirect way to ensure that the lifetime of the passed string will live as long as the TextRun. TextRun is effectively a StringView: it does not own the passed string. We need to fix this lifetime issue with TextRun and then we can change this signature to take a const lvalue reference. Until we fix the lifetime issue we should add a FIXME to explain that it is intentional that this function takes a non-const lvalue reference. We should consider filing a bug as well and reference the bug in the FIXME comment.
Comment 39 Daniel Bates 2018-03-29 00:18:23 PDT
Comment on attachment 336679 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=336679&action=review

> Source/WebCore/rendering/TextPainter.cpp:102
> +    static GlyphRunDisplayListCache* cache = new GlyphRunDisplayListCache;

NeverDestroyed<> instead of heap allocating?
Comment 40 zalan 2018-03-29 07:27:50 PDT
Could you check the (potential) performance regression when the cache is not utilized at all? -resize case, when each layout frame generates a different set of runs. (Similar to what we do in PerformanceTests/Layout/simple-line-layout-*.html)
Comment 41 Said Abou-Hallawa 2018-03-30 13:14:00 PDT
Created attachment 336884 [details]
Patch
Comment 42 Said Abou-Hallawa 2018-03-30 15:06:42 PDT
Comment on attachment 336679 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=336679&action=review

>> Source/WebCore/rendering/InlineTextBox.h:194
>> +    TextRun createTextRun(const String&) const;
> 
> Please do not make this change without fixing the lifetime issue with TextRun. Taking a non const lvalue reference is intentional and a crude and indirect way to ensure that the lifetime of the passed string will live as long as the TextRun. TextRun is effectively a StringView: it does not own the passed string. We need to fix this lifetime issue with TextRun and then we can change this signature to take a const lvalue reference. Until we fix the lifetime issue we should add a FIXME to explain that it is intentional that this function takes a non-const lvalue reference. We should consider filing a bug as well and reference the bug in the FIXME comment.

A comment was added and a bug was filed https://bugs.webkit.org/show_bug.cgi?id=184182

>> Source/WebCore/rendering/TextPainter.cpp:102
>> +    static GlyphRunDisplayListCache* cache = new GlyphRunDisplayListCache;
> 
> NeverDestroyed<> instead of heap allocating?

Done.
Comment 43 Said Abou-Hallawa 2018-04-02 09:29:24 PDT
(In reply to zalan from comment #40)
> Could you check the (potential) performance regression when the cache is not
> utilized at all? -resize case, when each layout frame generates a different
> set of runs. (Similar to what we do in
> PerformanceTests/Layout/simple-line-layout-*.html)

If I understand your request correctly, I think you want to measure the performance when the glyph run drawing is recorded, cached, replayed back and never used again. But how can this help judging whether approach of this patch is good enough or not?

Caching in general has its cost. If the caching strategy is good enough, this cost is remedied by the difference between using the cache and not using it.

The caching strategy here is very strict. The glyph run caching happens only if

- The glyph tun has been painted at least 30 times.
- The frame rate to paint this glyph tun has been at least 31.25 FPS.

So it is expected that a cached entry will be reused at least few times before it is removed from the cache. I could have proved the use of the cache and I could have proved caching is faster than not caching by the MotionMark Design progression.

I think it is more informative if we measure the performance of  PerformanceTests/Layout/ with and without this patch. This is more practical measurement than measuring a hypothetical scenario (caching elements but never use them).

I've never run-perf-tests PerformanceTests/Layout/ before and I don't how much these tests are reliable but I will try.
Comment 44 Said Abou-Hallawa 2018-04-02 11:30:37 PDT
I ran "run-perf-tests PerformanceTests/Layout/" and I got the attached results. It is a mix of progressions and regressions but most of the regressions are in the noise range. The number of progressed tests is 32. The number of regressed test is 18. Results and comparison are attached.
Comment 45 Said Abou-Hallawa 2018-04-02 11:31:19 PDT
Created attachment 337006 [details]
run-perf-tests results (Mac)
Comment 46 Said Abou-Hallawa 2018-04-02 11:36:17 PDT
Can any one in the cc-list review this patch?
Comment 47 zalan 2018-04-02 11:43:43 PDT
(In reply to Said Abou-Hallawa from comment #46)
> Can any one in the cc-list review this patch?
Please add cache purging support first (see WidthCache).
Comment 48 Said Abou-Hallawa 2018-04-02 11:52:41 PDT
(In reply to zalan from comment #47)
> (In reply to Said Abou-Hallawa from comment #46)
> > Can any one in the cc-list review this patch?
> Please add cache purging support first (see WidthCache).

Do you mean the check isUnderMemoryPressure()? If you yes, I already added it in GlyphRunDisplayListCache::ensure(). If not, can you please elaborate what do you mean by cache purging support?
Comment 49 zalan 2018-04-02 12:00:10 PDT
(In reply to Said Abou-Hallawa from comment #48)
> (In reply to zalan from comment #47)
> > (In reply to Said Abou-Hallawa from comment #46)
> > > Can any one in the cc-list review this patch?
> > Please add cache purging support first (see WidthCache).
> 
> Do you mean the check isUnderMemoryPressure()? If you yes, I already added
> it in GlyphRunDisplayListCache::ensure(). If not, can you please elaborate
> what do you mean by cache purging support?
GlyphRunDisplayListCache::ensure() takes care of purging the cache when it is actually being used (and not sure why it is iOS only). You also need to respond to memory pressure when the cache is not actively used. See WidthCache and releaseNoncriticalMemory().
Comment 50 Said Abou-Hallawa 2018-04-02 13:20:03 PDT
Created attachment 337020 [details]
Patch
Comment 51 Said Abou-Hallawa 2018-04-02 13:21:18 PDT
(In reply to zalan from comment #49)
> (In reply to Said Abou-Hallawa from comment #48)
> > (In reply to zalan from comment #47)
> > > (In reply to Said Abou-Hallawa from comment #46)
> > > > Can any one in the cc-list review this patch?
> > > Please add cache purging support first (see WidthCache).
> > 
> > Do you mean the check isUnderMemoryPressure()? If you yes, I already added
> > it in GlyphRunDisplayListCache::ensure(). If not, can you please elaborate
> > what do you mean by cache purging support?
> GlyphRunDisplayListCache::ensure() takes care of purging the cache when it
> is actually being used (and not sure why it is iOS only). You also need to
> respond to memory pressure when the cache is not actively used. See
> WidthCache and releaseNoncriticalMemory().

The following call was added to releaseNoncriticalMemory:

TextPainter::glyphRunDisplayListCache().clear();
Comment 52 Said Abou-Hallawa 2018-04-02 14:30:24 PDT
Created attachment 337031 [details]
Patch
Comment 53 Antti Koivisto 2018-04-04 10:32:49 PDT
Comment on attachment 337031 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=337031&action=review

> Source/WebCore/rendering/GlyphRunDisplayListCache.h:45
> +    DisplayList::DisplayList* ensure(PaintInfo& paintInfo, const void* key, Functor&& createDisplayList)

Use of void* as a key is not very nice and WebKit-like. There are two possible key types with unrelated types. Can you come up with a better solution?

> Source/WebCore/rendering/InlineTextBox.cpp:1025
> +    auto displayList = textPainter.glyphRunDisplayListCache().ensure(paintInfo, this, [&]() {

glyphRunDisplayListCache() is a static member of TextPainter, accessing it via an instance is confusing.

> Source/WebCore/rendering/PaintFrequencyTracker.h:34
> +class PaintFrequencyTracker {

Moving and refactoring PaintFrequencyTracker could be done in a separate patch. This would make things easier to review.

> Source/WebCore/rendering/TextPainter.h:81
> +    DisplayList::DisplayList*  m_glyphDisplayList { nullptr };

Extra space after *

What guarantees m_glyphDisplayList stays alive longer than the TextPainter?
Comment 54 Jon Lee 2018-04-04 10:42:04 PDT
Comment on attachment 337031 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=337031&action=review

> Source/WebCore/rendering/TextPainter.cpp:110
> +    if (emphasisMark.isEmpty()) {

Nit: this function would read nicer if the condition checked were reversed.

> Source/WebCore/rendering/TextPainter.cpp:111
> +        if (!m_context.paintingDisabled() && !startOffset && endOffset == textRun.length() && m_glyphDisplayList) {

Nit: Ditto.
Comment 55 zalan 2018-04-06 09:24:29 PDT
Comment on attachment 337031 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=337031&action=review

> Source/WebCore/ChangeLog:19
> +        PaintFrequencyTracker is a class which detects when a layer is painted 
> +        frequently. SinglePaintFrequencyTracking is used in conjunction with
> +        PaintFrequencyTracker to recored a single paint timing.

Any particular reason why it is done at RenderLayer level? It looks like only the text content is taking advantage of the PaintFrequencyTracking so unless it is going to be extended to other type of content, I am not sure why it is tied to RenderLayers.

> Source/WebCore/rendering/GlyphRunDisplayListCache.h:31
> +#include "RenderLayer.h"

Accessing RenderLayer from the run cache (or any cache) seems like a layer violation.
Comment 56 Said Abou-Hallawa 2018-04-08 16:41:14 PDT
Created attachment 337470 [details]
Patch
Comment 57 Said Abou-Hallawa 2018-04-08 17:00:38 PDT
Comment on attachment 337031 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=337031&action=review

>> Source/WebCore/ChangeLog:19
>> +        PaintFrequencyTracker to recored a single paint timing.
> 
> Any particular reason why it is done at RenderLayer level? It looks like only the text content is taking advantage of the PaintFrequencyTracking so unless it is going to be extended to other type of content, I am not sure why it is tied to RenderLayers.

I think the RenderLayer is a middle choice between the frame and the box renderers. Choosing the RenderLayer should limit the number of GlyphRunDisplayListCache created per page and at the same time can give a more accurate and scoped paint frequency than what the frame will give.

>> Source/WebCore/rendering/GlyphRunDisplayListCache.h:31
>> +#include "RenderLayer.h"
> 
> Accessing RenderLayer from the run cache (or any cache) seems like a layer violation.

I made sure GlyphRunDisplayListCache.h is not included any header file.

>> Source/WebCore/rendering/GlyphRunDisplayListCache.h:45
>> +    DisplayList::DisplayList* ensure(PaintInfo& paintInfo, const void* key, Functor&& createDisplayList)
> 
> Use of void* as a key is not very nice and WebKit-like. There are two possible key types with unrelated types. Can you come up with a better solution?

Like the original patch that Dave Hyatt wrote, I made two caches for the two kinds of runs: InlineTextBox and SimpleLineLayout::Run.

>> Source/WebCore/rendering/InlineTextBox.cpp:1025
>> +    auto displayList = textPainter.glyphRunDisplayListCache().ensure(paintInfo, this, [&]() {
> 
> glyphRunDisplayListCache() is a static member of TextPainter, accessing it via an instance is confusing.

Fixed.

>> Source/WebCore/rendering/PaintFrequencyTracker.h:34
>> +class PaintFrequencyTracker {
> 
> Moving and refactoring PaintFrequencyTracker could be done in a separate patch. This would make things easier to review.

Done. This was landed in <https://trac.webkit.org/changeset/230274>.

>> Source/WebCore/rendering/TextPainter.cpp:110
>> +    if (emphasisMark.isEmpty()) {
> 
> Nit: this function would read nicer if the condition checked were reversed.

Done.

>> Source/WebCore/rendering/TextPainter.cpp:111
>> +        if (!m_context.paintingDisabled() && !startOffset && endOffset == textRun.length() && m_glyphDisplayList) {
> 
> Nit: Ditto.

Done.

>> Source/WebCore/rendering/TextPainter.h:81
>> +    DisplayList::DisplayList*  m_glyphDisplayList { nullptr };
> 
> Extra space after *
> 
> What guarantees m_glyphDisplayList stays alive longer than the TextPainter?

Fixed.
Comment 58 zalan 2018-04-08 22:27:59 PDT
(In reply to Said Abou-Hallawa from comment #57)
> Comment on attachment 337031 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=337031&action=review
> 
> >> Source/WebCore/ChangeLog:19
> >> +        PaintFrequencyTracker to recored a single paint timing.
> > 
> > Any particular reason why it is done at RenderLayer level? It looks like only the text content is taking advantage of the PaintFrequencyTracking so unless it is going to be extended to other type of content, I am not sure why it is tied to RenderLayers.
> 
> I think the RenderLayer is a middle choice between the frame and the box
> renderers. Choosing the RenderLayer should limit the number of
> GlyphRunDisplayListCache created per page and at the same time can give a
> more accurate and scoped paint frequency than what the frame will give.
There are better ways of managing the number of GlyphRunDisplayListCache instances (and the number of cache entries) than just having them on RenderLayers. It's fine for prototyping, but I think it should be much smarter than this.    

> 
> >> Source/WebCore/rendering/GlyphRunDisplayListCache.h:31
> >> +#include "RenderLayer.h"
> > 
> > Accessing RenderLayer from the run cache (or any cache) seems like a layer violation.
> 
> I made sure GlyphRunDisplayListCache.h is not included any header file.
Including GlyphRunDisplayListCache.h is fine. The layer violation is the other way around; it is about using the RenderLayer from the cache.   

r=me (as a first version) but Antti might have some follow up questions.
Comment 59 Said Abou-Hallawa 2018-04-09 18:11:02 PDT
Created attachment 337567 [details]
Patch
Comment 60 Said Abou-Hallawa 2018-04-09 18:13:13 PDT
Comment on attachment 337031 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=337031&action=review

>>>> Source/WebCore/rendering/GlyphRunDisplayListCache.h:31
>>>> +#include "RenderLayer.h"
>>> 
>>> Accessing RenderLayer from the run cache (or any cache) seems like a layer violation.
>> 
>> I made sure GlyphRunDisplayListCache.h is not included any header file.
> 
> Including GlyphRunDisplayListCache.h is fine. The layer violation is the other way around; it is about using the RenderLayer from the cache.   
> 
> r=me (as a first version) but Antti might have some follow up questions.

With the new patch neither RenderLayer.h nor PaintInfo.h is included in GlyphRunDisplayListCache.h. I just replaced the calls to PaintInfo and RenderLayer I used to do in GlyphRunDisplayListCache::ensure() by passing a single boolean variable named "shouldRemove".
Comment 61 Build Bot 2018-04-09 21:11:11 PDT
Comment on attachment 337567 [details]
Patch

Attachment 337567 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/7262719

New failing tests:
imported/w3c/web-platform-tests/workers/name-property.html
Comment 62 Build Bot 2018-04-09 21:11:13 PDT
Created attachment 337585 [details]
Archive of layout-test-results from ews112 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 63 Daniel Bates 2018-04-09 22:20:16 PDT
Comment on attachment 337567 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=337567&action=review

> Source/WebCore/rendering/GlyphRunDisplayListCache.h:41
> +    DisplayList::DisplayList* ensure(const RunType* run, bool shouldRemove, Functor&& createDisplayList)

Typically we name functions “ensure” when they are guaranteed to return a non-null object and hence we have them return a Ref<> or reference. This function goes against this convention because it can return a nullptr. Can we come up with a more appropriate name for this function?
Comment 64 Said Abou-Hallawa 2018-04-10 11:16:57 PDT
Created attachment 337623 [details]
Patch
Comment 65 Said Abou-Hallawa 2018-04-10 11:17:57 PDT
Comment on attachment 337585 [details]
Archive of layout-test-results from ews112 for mac-sierra

imported/w3c/web-platform-tests/workers/name-property.html is a flaky test.
Comment 66 zalan 2018-04-10 11:18:24 PDT
(In reply to Daniel Bates from comment #63)
> Comment on attachment 337567 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=337567&action=review
> 
> > Source/WebCore/rendering/GlyphRunDisplayListCache.h:41
> > +    DisplayList::DisplayList* ensure(const RunType* run, bool shouldRemove, Functor&& createDisplayList)
> 
> Typically we name functions “ensure” when they are guaranteed to return a
> non-null object and hence we have them return a Ref<> or reference. This
> function goes against this convention because it can return a nullptr. Can
> we come up with a more appropriate name for this function?
The "shouldRemove" makes this function/logic a bit odd. Is there a way to handle the remove case separately?
Also, I don't mind having the "Functor&& createDisplayList" but not sure why it is better than making it all internal to GlyphRunDisplayListCache by passing in the font and the run.
Comment 67 Said Abou-Hallawa 2018-04-10 13:35:49 PDT
Created attachment 337629 [details]
Patch
Comment 68 Said Abou-Hallawa 2018-04-10 13:39:52 PDT
Comment on attachment 337567 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=337567&action=review

>>> Source/WebCore/rendering/GlyphRunDisplayListCache.h:41
>>> +    DisplayList::DisplayList* ensure(const RunType* run, bool shouldRemove, Functor&& createDisplayList)
>> 
>> Typically we name functions “ensure” when they are guaranteed to return a non-null object and hence we have them return a Ref<> or reference. This function goes against this convention because it can return a nullptr. Can we come up with a more appropriate name for this function?
> 
> The "shouldRemove" makes this function/logic a bit odd. Is there a way to handle the remove case separately?
> Also, I don't mind having the "Functor&& createDisplayList" but not sure why it is better than making it all internal to GlyphRunDisplayListCache by passing in the font and the run.

Fixed. 

1. Function was renamed to get().
2. shouldRemove was removed
3. "Functor&& createDisplayList" was replaced by "const FontCascade& font, GraphicsContext& context, const TextRun& textRun"

I was trying to make creating the DisplayList in the caller side so I would not have to include the header files: FontCascade.h and TextRun.h
Comment 69 zalan 2018-04-10 13:59:44 PDT
Comment on attachment 337629 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=337629&action=review

> Source/WebCore/platform/graphics/FontCascade.cpp:309
> +std::unique_ptr<DisplayList::DisplayList> FontCascade::recordGlyphDisplayList(GraphicsContext& context, const TextRun& run, unsigned from, std::optional<unsigned> to, CustomFontNotReadyAction customFontNotReadyAction) const

I think "displayListForTextRun" (or something along those lines) would be more accurate (the word "record" certainly has no added value here, but again I am not great at coming up with names)

> Source/WebCore/platform/graphics/FontCascade.cpp:312
> +    if (context.paintingDisabled())
> +        return nullptr;

I think this should be an ASSERT instead. Callsites should (and I think they do atm) ensure that we don't call into recording when the painting is disabled.

> Source/WebCore/rendering/GlyphDisplayListCache.h:42
> +    DisplayList::DisplayList* get(const LayoutRun* run, const FontCascade& font, GraphicsContext& context, const TextRun& textRun)

Any reason why LayoutRun is not &?

> Source/WebCore/rendering/GlyphDisplayListCache.h:61
> +    void remove(const LayoutRun* run)

&?

> Source/WebCore/rendering/InlineTextBox.cpp:1028
> +    if (paintInfo.context().paintingDisabled() || !paintInfo.enclosingSelfPaintingLayer() || !paintInfo.enclosingSelfPaintingLayer()->paintingFrequently())
> +        TextPainter::removeGlyphDisplayList(this);
> +    else
> +        displayList = TextPainter::getGlyphDisplayList(this, font, context, textRun);
> +
> +    textPainter.setGlyphDisplayList(displayList);

else {
    auto* displayList = TextPainter::getGlyphDisplayList(this, font, context, textRun);
    if (displayList)    
       textPainter.setGlyphDisplayList(*displayList);
}
?

> Source/WebCore/rendering/SimpleLineLayoutFunctions.cpp:131
> +        DisplayList::DisplayList* displayList = nullptr;
> +        if (paintInfo.context().paintingDisabled() || !paintInfo.enclosingSelfPaintingLayer() || !paintInfo.enclosingSelfPaintingLayer()->paintingFrequently())
> +            TextPainter::removeGlyphDisplayList(&run.simpleRun());
> +        else
> +            displayList = TextPainter::getGlyphDisplayList(&run.simpleRun(), style.fontCascade(), paintInfo.context(), textRun);
> +
> +        textPainter.setGlyphDisplayList(displayList);

Same as above. Also, instead of duplicating a logic for not initiating a display list, could you move it to a static function somewhere?

> Source/WebCore/rendering/TextPainter.h:72
> +    static void removeGlyphDisplayList(const LayoutRun* run) { return GlyphDisplayListCache<LayoutRun>().remove(run); }

return?
Comment 70 Build Bot 2018-04-10 14:39:03 PDT
Comment on attachment 337629 [details]
Patch

Attachment 337629 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/7272347

Number of test failures exceeded the failure limit.
Comment 71 Build Bot 2018-04-10 14:39:05 PDT
Created attachment 337638 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 72 Build Bot 2018-04-10 14:50:38 PDT
Comment on attachment 337629 [details]
Patch

Attachment 337629 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/7272464

New failing tests:
inspector/canvas/recording-webgl.html
fast/events/drag-and-drop.html
fast/events/dropzone-001.html
http/tests/misc/acid3.html
Comment 73 Build Bot 2018-04-10 14:50:40 PDT
Created attachment 337640 [details]
Archive of layout-test-results from ews103 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 74 Build Bot 2018-04-10 15:08:03 PDT
Comment on attachment 337629 [details]
Patch

Attachment 337629 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/7272479

Number of test failures exceeded the failure limit.
Comment 75 Build Bot 2018-04-10 15:08:05 PDT
Created attachment 337642 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4
Comment 76 Build Bot 2018-04-10 15:25:30 PDT
Comment on attachment 337629 [details]
Patch

Attachment 337629 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/7272597

New failing tests:
fast/events/dropzone-001.html
fast/events/drag-and-drop.html
Comment 77 Build Bot 2018-04-10 15:25:32 PDT
Created attachment 337644 [details]
Archive of layout-test-results from ews116 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 78 Said Abou-Hallawa 2018-04-10 15:28:51 PDT
Created attachment 337646 [details]
Patch
Comment 79 Said Abou-Hallawa 2018-04-10 16:57:49 PDT
Comment on attachment 337629 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=337629&action=review

> Source/WebCore/rendering/TextPainter.h:68
> +        return GlyphDisplayListCache<LayoutRun>().get(run, font, context, textRun);

This is the cause of the crashes in the EWS layout tests. We should call glyphDisplayListCache<LayoutRun>() not GlyphDisplayListCache<LayoutRun>(). The first is a call to the static method which ensures the GlyphDisplayListCache is never destroyed. But the second creates a temporary GlyphDisplayListCache, adds an entry to it, and then deletes the GlyphDisplayListCache. This will return a freed DisplayList pointer because the single entry in the HahMap is deleted.
Comment 80 Said Abou-Hallawa 2018-04-10 17:54:59 PDT
Created attachment 337664 [details]
Patch
Comment 81 Said Abou-Hallawa 2018-04-10 18:03:33 PDT
Comment on attachment 337629 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=337629&action=review

>> Source/WebCore/platform/graphics/FontCascade.cpp:309
>> +std::unique_ptr<DisplayList::DisplayList> FontCascade::recordGlyphDisplayList(GraphicsContext& context, const TextRun& run, unsigned from, std::optional<unsigned> to, CustomFontNotReadyAction customFontNotReadyAction) const
> 
> I think "displayListForTextRun" (or something along those lines) would be more accurate (the word "record" certainly has no added value here, but again I am not great at coming up with names)

Names was changed to displayListForTextRun().

>> Source/WebCore/platform/graphics/FontCascade.cpp:312
>> +        return nullptr;
> 
> I think this should be an ASSERT instead. Callsites should (and I think they do atm) ensure that we don't call into recording when the painting is disabled.

Condition was replaced by an assertion.

>> Source/WebCore/rendering/GlyphDisplayListCache.h:42
>> +    DisplayList::DisplayList* get(const LayoutRun* run, const FontCascade& font, GraphicsContext& context, const TextRun& textRun)
> 
> Any reason why LayoutRun is not &?

Argument was changed from a pointer to a reference.

>> Source/WebCore/rendering/GlyphDisplayListCache.h:61
>> +    void remove(const LayoutRun* run)
> 
> &?

Argument was changed from a pointer to a reference.

>> Source/WebCore/rendering/InlineTextBox.cpp:1028
>> +    textPainter.setGlyphDisplayList(displayList);
> 
> else {
>     auto* displayList = TextPainter::getGlyphDisplayList(this, font, context, textRun);
>     if (displayList)    
>        textPainter.setGlyphDisplayList(*displayList);
> }
> ?

Your code and mine are not exactly equivalent. This was the only place that resets the raw pointer textPainter.m_glyphDisplayList to nullptr. But I did the following:

1. I put all the logic here a function named TextPainter::setGlyphDisplayListIfNeeded().
2. I moved the condition which initiates the DisplayList to a new function named TextPainter::shouldUseGlyphDisplayList(). 
3. I added reseting the textPainter.m_glyphDisplayList to TextPainter::paintTextOrEmphasisMarks(). textPainter.m_glyphDisplayList is valid for one paint only. It has to be set again from the cache if we decide to playback the DisplayList.

>> Source/WebCore/rendering/TextPainter.h:72
>> +    static void removeGlyphDisplayList(const LayoutRun* run) { return GlyphDisplayListCache<LayoutRun>().remove(run); }
> 
> return?

The return keyword was removed.
Comment 82 Antti Koivisto 2018-04-11 11:26:47 PDT
Comment on attachment 337664 [details]
Patch

r=me
Comment 83 WebKit Commit Bot 2018-04-11 13:29:54 PDT
Comment on attachment 337664 [details]
Patch

Clearing flags on attachment: 337664

Committed r230544: <https://trac.webkit.org/changeset/230544>
Comment 84 WebKit Commit Bot 2018-04-11 13:29:57 PDT
All reviewed patches have been landed.  Closing bug.