Bug 170842 – Add performance test for FontCache::systemFallbackForCharacters()

• Component: New Bugs
• Status: RESOLVED FIXED

[Myles C. Maxfield]

Add performance test for FontCache::systemFallbackForCharacters()

[Myles C. Maxfield]

Created attachment 307075 [details]
Patch

[Tim Horton]

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!

[Myles C. Maxfield]

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.

[Tim Horton]

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

[Build Bot]

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

[Build Bot]

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

[Myles C. Maxfield]

Committed r215366: <http://trac.webkit.org/changeset/215366>

[Ryan Haddad]

(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

[Ryan Haddad]

Reverted r215366 for reason:

This test is failing on performance bots.

Committed r215417: <http://trac.webkit.org/changeset/215417>

[Myles C. Maxfield]

How can a performance test fail?

[Chris Dumez]

(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

[Chris Dumez]

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

[Chris Dumez]

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

[Myles C. Maxfield]

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

[Myles C. Maxfield]

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

[Myles C. Maxfield]

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

[Myles C. Maxfield]

Committed r215737: <http://trac.webkit.org/changeset/215737>

[Carlos Alberto Lopez Perez]

(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

[Carlos Alberto Lopez Perez]

Committed r215745: <http://trac.webkit.org/changeset/215745>

[Konstantin Tokarev]

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

[Carlos Alberto Lopez Perez]

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

[Carlos Alberto Lopez Perez]

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

[Myles C. Maxfield]

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

[Myles C. Maxfield]

Committed r215752: <http://trac.webkit.org/changeset/215752>

[Carlos Alberto Lopez Perez]

Committed r215754: <http://trac.webkit.org/changeset/215754>

[Myles C. Maxfield]

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

[Carlos Alberto Lopez Perez]

(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