Add performance test for FontCache::systemFallbackForCharacters()
Created attachment 307075 [details] Patch
Comment on attachment 307075 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=307075&action=review > Source/WebCore/platform/graphics/FontCascade.h:264 > + static bool treatAsZeroWidthSpace(UChar c) { return c < 0x20 || (c >= 0x7F && c < 0xA0) || c == softHyphen || c == zeroWidthSpace || (c >= 0x200e && c <= 0x200f) || (c >= 0x202a && c <= 0x202e) || c == zeroWidthNoBreakSpace || c == objectReplacementCharacter || c == zeroWidthNonJoiner || c == zeroWidthJoiner; } How is this part not a behavior change? And, if it's just a performance change, you should explain in the changelog!
Comment on attachment 307075 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=307075&action=review >> Source/WebCore/platform/graphics/FontCascade.h:264 >> + static bool treatAsZeroWidthSpace(UChar c) { return c < 0x20 || (c >= 0x7F && c < 0xA0) || c == softHyphen || c == zeroWidthSpace || (c >= 0x200e && c <= 0x200f) || (c >= 0x202a && c <= 0x202e) || c == zeroWidthNoBreakSpace || c == objectReplacementCharacter || c == zeroWidthNonJoiner || c == zeroWidthJoiner; } > > How is this part not a behavior change? And, if it's just a performance change, you should explain in the changelog! It's not a behavior change. I just inlined treatAsZeroWidthSpaceInComplexScript() since it only has a single caller. There's no effect since the compiler inlines the calls already.
(In reply to Myles C. Maxfield from comment #3) > Comment on attachment 307075 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=307075&action=review > > >> Source/WebCore/platform/graphics/FontCascade.h:264 > >> + static bool treatAsZeroWidthSpace(UChar c) { return c < 0x20 || (c >= 0x7F && c < 0xA0) || c == softHyphen || c == zeroWidthSpace || (c >= 0x200e && c <= 0x200f) || (c >= 0x202a && c <= 0x202e) || c == zeroWidthNoBreakSpace || c == objectReplacementCharacter || c == zeroWidthNonJoiner || c == zeroWidthJoiner; } > > > > How is this part not a behavior change? And, if it's just a performance change, you should explain in the changelog! > > It's not a behavior change. I just inlined > treatAsZeroWidthSpaceInComplexScript() since it only has a single caller. > There's no effect since the compiler inlines the calls already. Ahh! I see what I was missing.
Comment on attachment 307075 [details] Patch Attachment 307075 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/3532832 New failing tests: webrtc/multi-video.html
Created attachment 307108 [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.11.6
Committed r215366: <http://trac.webkit.org/changeset/215366>
(In reply to Myles C. Maxfield from comment #7) > Committed r215366: <http://trac.webkit.org/changeset/215366> This test is failing on the El Capitan perf bot: Running Layout/word-joiner.html (132 of 167) ERROR: FAILED Finished: 7.317369 s https://build.webkit.org/builders/Apple%20El%20Capitan%20Release%20WK2%20(Perf)/builds/202
Reverted r215366 for reason: This test is failing on performance bots. Committed r215417: <http://trac.webkit.org/changeset/215417>
How can a performance test fail?
(In reply to Myles C. Maxfield from comment #10) > How can a performance test fail? Ryosuke, do you know? I would assume at least the following: - Exceeded timeout - Printed things on stderr - Crashed
(In reply to Chris Dumez from comment #11) > (In reply to Myles C. Maxfield from comment #10) > > How can a performance test fail? > > Ryosuke, do you know? I would assume at least the following: > - Exceeded timeout > - Printed things on stderr > - Crashed In this case, it prints "Error: " which is printed by: for line in re.split('\n', output.text): description_match = self._description_regex.match(line) if description_match: self._description = description_match.group('description') continue metric_match = self._metrics_regex.match(line) if not metric_match: _log.error('ERROR: ' + line) return False in Tools/Scripts/webkitpy/performance_tests/perftest.py so the _metrics_regex fails the match the printed line in the test output.
(In reply to Chris Dumez from comment #12) > (In reply to Chris Dumez from comment #11) > > (In reply to Myles C. Maxfield from comment #10) > > > How can a performance test fail? > > > > Ryosuke, do you know? I would assume at least the following: > > - Exceeded timeout > > - Printed things on stderr > > - Crashed > > In this case, it prints "Error: " which is printed by: > for line in re.split('\n', output.text): > description_match = self._description_regex.match(line) > if description_match: > self._description = description_match.group('description') > continue > > metric_match = self._metrics_regex.match(line) > if not metric_match: > _log.error('ERROR: ' + line) > return False > > in Tools/Scripts/webkitpy/performance_tests/perftest.py > > so the _metrics_regex fails the match the printed line in the test output. _metrics_regex = re.compile(r'^(?P<subtest>[A-Za-z0-9\(\[].+?)?:(?P<metric>[A-Z][A-Za-z]+)(:(?P<aggregator>[A-Z][A-Za-z]+))? -> \[(?P<values>(\d+(\.\d+)?)(, \d+(\.\d+)?)+)\] (?P<unit>[a-z/]+)?$') It expects a metric to be printed but it seems the test printed an empty line.
(In reply to Chris Dumez from comment #13) > (In reply to Chris Dumez from comment #12) > > (In reply to Chris Dumez from comment #11) > > > (In reply to Myles C. Maxfield from comment #10) > > > > How can a performance test fail? > > > > > > Ryosuke, do you know? I would assume at least the following: > > > - Exceeded timeout > > > - Printed things on stderr > > > - Crashed > > > > In this case, it prints "Error: " which is printed by: > > for line in re.split('\n', output.text): > > description_match = self._description_regex.match(line) > > if description_match: > > self._description = description_match.group('description') > > continue > > > > metric_match = self._metrics_regex.match(line) > > if not metric_match: > > _log.error('ERROR: ' + line) > > return False > > > > in Tools/Scripts/webkitpy/performance_tests/perftest.py > > > > so the _metrics_regex fails the match the printed line in the test output. > > _metrics_regex = > re.compile(r'^(?P<subtest>[A-Za-z0-9\(\[].+?)?:(?P<metric>[A-Z][A-Za-z]+)(: > (?P<aggregator>[A-Z][A-Za-z]+))? -> \[(?P<values>(\d+(\.\d+)?)(, > \d+(\.\d+)?)+)\] (?P<unit>[a-z/]+)?$') > > It expects a metric to be printed but it seems the test printed an empty > line. My test has the exact same output as PerformanceTests/Layout/chapter-reflow.html. Confusing...
(In reply to Myles C. Maxfield from comment #14) > (In reply to Chris Dumez from comment #13) > > (In reply to Chris Dumez from comment #12) > > > (In reply to Chris Dumez from comment #11) > > > > (In reply to Myles C. Maxfield from comment #10) > > > > > How can a performance test fail? > > > > > > > > Ryosuke, do you know? I would assume at least the following: > > > > - Exceeded timeout > > > > - Printed things on stderr > > > > - Crashed > > > > > > In this case, it prints "Error: " which is printed by: > > > for line in re.split('\n', output.text): > > > description_match = self._description_regex.match(line) > > > if description_match: > > > self._description = description_match.group('description') > > > continue > > > > > > metric_match = self._metrics_regex.match(line) > > > if not metric_match: > > > _log.error('ERROR: ' + line) > > > return False > > > > > > in Tools/Scripts/webkitpy/performance_tests/perftest.py > > > > > > so the _metrics_regex fails the match the printed line in the test output. > > > > _metrics_regex = > > re.compile(r'^(?P<subtest>[A-Za-z0-9\(\[].+?)?:(?P<metric>[A-Z][A-Za-z]+)(: > > (?P<aggregator>[A-Z][A-Za-z]+))? -> \[(?P<values>(\d+(\.\d+)?)(, > > \d+(\.\d+)?)+)\] (?P<unit>[a-z/]+)?$') > > > > It expects a metric to be printed but it seems the test printed an empty > > line. > > My test has the exact same output as > PerformanceTests/Layout/chapter-reflow.html. Confusing... The test is failing because the web process is writing to stderr. It is writing ERROR: Could not create a sandbox extension for '/Users/mmaxfield/src/WebKit/OpenSource/LayoutTests/PerformanceTests/Layout/word-joiner.html' SandboxExtension::createHandleWithoutResolvingPath() shows that we are trying to create a sandbox exception for the local file so that we can open it in the network process, but we are being denied. It's unclear why my test is being denied but the other tests are being allowed.
Running the tests myself using DYLD_FRAMEWORK_PATH=/Users/mmaxfield/Build/Products/Debug ~/Build/Products/Debug/WebKitTestRunner "file:///Users/mmaxfield/src/WebKit/OpenSource/PerformanceTests/Layout/word-joiner.html" and DYLD_FRAMEWORK_PATH=/Users/mmaxfield/Build/Products/Debug ~/Build/Products/Debug/DumpRenderTree "file:///Users/mmaxfield/src/WebKit/OpenSource/PerformanceTests/Layout/word-joiner.html" seems to work just fine, and not print to stderr
Committed r215737: <http://trac.webkit.org/changeset/215737>
(In reply to Myles C. Maxfield from comment #17) > Committed r215737: <http://trac.webkit.org/changeset/215737> This broke the GTK+ build (as the EWS showed) ./../Source/WebCore/platform/graphics/harfbuzz/HarfBuzzShaper.cpp: In function ‘void WebCore::normalizeCharacters(const WebCore::TextRun&, UChar*, unsigned int)’: ../../Source/WebCore/platform/graphics/harfbuzz/HarfBuzzShaper.cpp:182:18: error: ‘treatAsZeroWidthSpaceInComplexScript’ is not a member of ‘WebCore::FontCascade’ else if (FontCascade::treatAsZeroWidthSpaceInComplexScript(character)) ^ https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release%20Debian%20Stable%20%28Build%29/builds/1090
Committed r215745: <http://trac.webkit.org/changeset/215745>
(In reply to Carlos Alberto Lopez Perez from comment #19) > Committed r215745: <http://trac.webkit.org/changeset/215745> It seems to me that the correct compilation fix would be to revert removal of treatAsZeroWidthSpaceInComplexScript. Otherwise behavior of HarfBuzzShaper changes, and after r215745 it converts 0x200c and 0x200d to zeroWidthSpace, while previously it didn't. Maybe it's a progression though, I'm not an expert on this.
(In reply to Konstantin Tokarev from comment #20) > (In reply to Carlos Alberto Lopez Perez from comment #19) > > Committed r215745: <http://trac.webkit.org/changeset/215745> > > It seems to me that the correct compilation fix would be to revert removal > of treatAsZeroWidthSpaceInComplexScript. Otherwise behavior of > HarfBuzzShaper changes, and after r215745 it converts 0x200c and 0x200d to > zeroWidthSpace, while previously it didn't. > That two characters are the ZERO WIDTH JOINER and the ZERO WIDTH NON-JOINER http://www.unicodemap.org/details/0x200C/index.html http://www.unicodemap.org/details/0x200D/index.html Checking the history I see that treatAsZeroWidthSpaceInComplexScript() was added in r111589 ( bug 79961 ) with test fast/text/zero-width-characters-complex-script.html and that the code using that function in HarfBuzzShaper was added in r122562 ( bug 90951 ) with tests fast/text/shaping/shaping-script-order.html and fast/text/shaping/shaping-selection-rect.html I have checked that this 3 tests pass after and before 215745. > Maybe it's a progression though, I'm not an expert on this. I'm neither an expert. It was an speculative fix. And you are right that there is a behaviour change here that was neither intended by me, neither by Miles as per comment https://bugs.webkit.org/show_bug.cgi?id=170842#c3 (he overlooked to see that the GTK port was calling on that function) Perhaps the safest thing to do here is to revert r215745 and the changes that r215737 did on Source/WebCore/platform/graphics/FontCascade.h Any opinion?
(In reply to Carlos Alberto Lopez Perez from comment #21) > Perhaps the safest thing to do here is to revert r215745 and the changes > that r215737 did on Source/WebCore/platform/graphics/FontCascade.h > > Any opinion? I will do that. This change of behaviour shouldn't be part of this bug (adding a performance test). If someone wants to propose removing treatAsZeroWidthSpaceInComplexScript in favour of just treatAsZeroWidthSpace, then please open a new bug.
(In reply to Konstantin Tokarev from comment #20) > (In reply to Carlos Alberto Lopez Perez from comment #19) > > Committed r215745: <http://trac.webkit.org/changeset/215745> > > It seems to me that the correct compilation fix would be to revert removal > of treatAsZeroWidthSpaceInComplexScript. Otherwise behavior of > HarfBuzzShaper changes, and after r215745 it converts 0x200c and 0x200d to > zeroWidthSpace, while previously it didn't. > > Maybe it's a progression though, I'm not an expert on this. Whoops. I did this in the patch which got rolled out, but forgot to do it in the roll-back-in. I'll fix it.
Committed r215752: <http://trac.webkit.org/changeset/215752>
Committed r215754: <http://trac.webkit.org/changeset/215754>
(In reply to Carlos Alberto Lopez Perez from comment #25) > Committed r215754: <http://trac.webkit.org/changeset/215754> Didn't I already fix this in r215752?
(In reply to Myles C. Maxfield from comment #26) > (In reply to Carlos Alberto Lopez Perez from comment #25) > > Committed r215754: <http://trac.webkit.org/changeset/215754> > > Didn't I already fix this in r215752? Yes. I reverted my previous fix: http://trac.webkit.org/changeset/215745