|
Description
Dave Hyatt
2017-10-24 14:22:17 PDT
Created attachment 324721 [details]
Work in progress
Created attachment 325018 [details]
Patch
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.
Created attachment 325031 [details]
Patch
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 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 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 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 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. Will implement it just to see how it compares though. Created attachment 325363 [details]
Patch
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? Created attachment 326363 [details]
Patch (not for review)
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 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. 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
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. 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
Created attachment 326414 [details]
Patch
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? 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. Created attachment 336601 [details]
Patch
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 Created attachment 336612 [details]
Patch
(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. I think this cache should be hooked up with the MemoryPressureHandler and purge under memory pressure. Created attachment 336679 [details]
Patch
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. 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? 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) Created attachment 336884 [details]
Patch
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. Created attachment 337006 [details]
run-perf-tests results (Mac)
Can any one in the cc-list review this patch? (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(). Created attachment 337020 [details]
Patch
(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(); Created attachment 337031 [details]
Patch
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. Created attachment 337470 [details]
Patch
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. Created attachment 337567 [details]
Patch
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 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 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? Created attachment 337623 [details]
Patch
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. Created attachment 337629 [details]
Patch
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? 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. 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 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 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 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. 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 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 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
Created attachment 337646 [details]
Patch
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. Created attachment 337664 [details]
Patch
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 on attachment 337664 [details]
Patch
r=me
Comment on attachment 337664 [details] Patch Clearing flags on attachment: 337664 Committed r230544: <https://trac.webkit.org/changeset/230544> All reviewed patches have been landed. Closing bug. |