Bug 170842 - Add performance test for FontCache::systemFallbackForCharacters()
Summary: Add performance test for FontCache::systemFallbackForCharacters()
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-04-13 19:54 PDT by Myles C. Maxfield
Modified: 2017-04-17 16:21 PDT (History)
8 users (show)

See Also:


Attachments
Patch (7.89 KB, patch)
2017-04-13 19:58 PDT, Myles C. Maxfield
thorton: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews123 for ios-simulator-wk2 (105.40 MB, application/zip)
2017-04-14 02:19 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Myles C. Maxfield 2017-04-13 19:54:35 PDT
Add performance test for FontCache::systemFallbackForCharacters()
Comment 1 Myles C. Maxfield 2017-04-13 19:58:59 PDT
Created attachment 307075 [details]
Patch
Comment 2 Tim Horton 2017-04-13 20:14:53 PDT
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 3 Myles C. Maxfield 2017-04-13 20:35:44 PDT
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.
Comment 4 Tim Horton 2017-04-13 20:37:49 PDT
(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 5 Build Bot 2017-04-14 02:19:01 PDT
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
Comment 6 Build Bot 2017-04-14 02:19:06 PDT
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
Comment 7 Myles C. Maxfield 2017-04-14 10:57:31 PDT
Committed r215366: <http://trac.webkit.org/changeset/215366>
Comment 8 Ryan Haddad 2017-04-17 09:21:21 PDT
(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
Comment 9 Ryan Haddad 2017-04-17 09:37:31 PDT
Reverted r215366 for reason:

This test is failing on performance bots.

Committed r215417: <http://trac.webkit.org/changeset/215417>
Comment 10 Myles C. Maxfield 2017-04-17 16:04:18 PDT
How can a performance test fail?
Comment 11 Chris Dumez 2017-04-17 16:15:43 PDT
(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
Comment 12 Chris Dumez 2017-04-17 16:19:10 PDT
(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.
Comment 13 Chris Dumez 2017-04-17 16:21:32 PDT
(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.