Bug 178750 – Cache glyphs (using display lists) when painting at high frequency

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Dave Hyatt]

[Experiment] Cache glyphs when painting at high frequency

[Dave Hyatt]

Created attachment 324721 [details]
Work in progress

[Dave Hyatt]

Created attachment 325018 [details]
Patch

[Build Bot]

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.

[Dave Hyatt]

Created attachment 325031 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/35201729>

[Simon Fraser (smfr)]

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.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Dave Hyatt]

(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.

[Dave Hyatt]

Will implement it just to see how it compares though.

[Dave Hyatt]

Created attachment 325363 [details]
Patch

[Simon Fraser (smfr)]

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?

[Dave Hyatt]

Created attachment 326363 [details]
Patch (not for review)

[Build Bot]

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.

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Dave Hyatt]

Created attachment 326414 [details]
Patch

[Simon Fraser (smfr)]

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?

[Dave Hyatt]

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.

[Said Abou-Hallawa]

Created attachment 336601 [details]
Patch

[Said Abou-Hallawa]

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).

[Said Abou-Hallawa]

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.

[Said Abou-Hallawa]

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

[Said Abou-Hallawa]

Created attachment 336612 [details]
Patch

[Brent Fulgham]

(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! :-)

[zalan]

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.

[zalan]

I think this cache should be hooked up with the MemoryPressureHandler and purge under memory pressure.

[Said Abou-Hallawa]

Created attachment 336679 [details]
Patch

[Said Abou-Hallawa]

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.

[Daniel Bates]

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.

[Daniel Bates]

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?

[zalan]

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)

[Said Abou-Hallawa]

Created attachment 336884 [details]
Patch

[Said Abou-Hallawa]

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.

[Said Abou-Hallawa]

(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.

[Said Abou-Hallawa]

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.

[Said Abou-Hallawa]

Created attachment 337006 [details]
run-perf-tests results (Mac)

[Said Abou-Hallawa]

Can any one in the cc-list review this patch?

[zalan]

(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).

[Said Abou-Hallawa]

(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?

[zalan]

(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().

[Said Abou-Hallawa]

Created attachment 337020 [details]
Patch

[Said Abou-Hallawa]

(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();

[Said Abou-Hallawa]

Created attachment 337031 [details]
Patch

[Antti Koivisto]

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?

[Jon Lee]

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.

[zalan]

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.

[Said Abou-Hallawa]

Created attachment 337470 [details]
Patch

[Said Abou-Hallawa]

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.

[zalan]

(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.

[Said Abou-Hallawa]

Created attachment 337567 [details]
Patch

[Said Abou-Hallawa]

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".

[EWS Watchlist]

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

[EWS Watchlist]

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

[Daniel Bates]

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?

[Said Abou-Hallawa]

Created attachment 337623 [details]
Patch

[Said Abou-Hallawa]

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.

[zalan]

(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.

[Said Abou-Hallawa]

Created attachment 337629 [details]
Patch

[Said Abou-Hallawa]

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

[zalan]

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?

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Said Abou-Hallawa]

Created attachment 337646 [details]
Patch

[Said Abou-Hallawa]

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.

[Said Abou-Hallawa]

Created attachment 337664 [details]
Patch

[Said Abou-Hallawa]

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.

[Antti Koivisto]

Comment on attachment 337664 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 337664 [details]
Patch

Clearing flags on attachment: 337664

Committed r230544: <https://trac.webkit.org/changeset/230544>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.