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.
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.
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
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
(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 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?
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.
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 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
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
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 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
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 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?
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).
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.
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
(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 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 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 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.
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 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.
(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.
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.
(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).
(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?
(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().
(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 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 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 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 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.
(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 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".
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 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 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.
(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 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 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?
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
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
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
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 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 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.
2017-10-24 14:23 PDT, Dave Hyatt
2017-10-26 09:33 PDT, Dave Hyatt
2017-10-26 10:58 PDT, Dave Hyatt
2017-10-26 13:01 PDT, Build Bot
2017-10-26 18:00 PDT, Build Bot
2017-10-30 11:40 PDT, Dave Hyatt
2017-11-08 13:47 PST, Dave Hyatt
2017-11-08 14:38 PST, Build Bot
2017-11-08 14:42 PST, Build Bot
2017-11-08 15:08 PST, Build Bot
2017-11-08 15:25 PST, Build Bot
2017-11-08 17:47 PST, Dave Hyatt
2018-03-27 11:50 PDT, Said Abou-Hallawa
2018-03-27 12:47 PDT, Said Abou-Hallawa
2018-03-28 11:12 PDT, Said Abou-Hallawa
2018-03-30 13:14 PDT, Said Abou-Hallawa
2018-04-02 11:31 PDT, Said Abou-Hallawa
2018-04-02 13:20 PDT, Said Abou-Hallawa
2018-04-02 14:30 PDT, Said Abou-Hallawa
2018-04-08 16:41 PDT, Said Abou-Hallawa
2018-04-09 18:11 PDT, Said Abou-Hallawa
2018-04-09 21:11 PDT, EWS Watchlist
2018-04-10 11:16 PDT, Said Abou-Hallawa
2018-04-10 13:35 PDT, Said Abou-Hallawa
2018-04-10 14:39 PDT, EWS Watchlist
2018-04-10 14:50 PDT, EWS Watchlist
2018-04-10 15:08 PDT, EWS Watchlist
2018-04-10 15:25 PDT, EWS Watchlist
2018-04-10 15:28 PDT, Said Abou-Hallawa
2018-04-10 17:54 PDT, Said Abou-Hallawa