Bug 120110 – [GTK] Skip FontConfig initialization in WebKitTestRunner if requested

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Zan Dobersek]

[GTK] Skip FontConfig initialization in WebKitTestRunner if requested

[Zan Dobersek]

Created attachment 209267 [details]
Patch

[Zan Dobersek]

As described in the change log, FontConfig initialization is pretty heavy computation-wise, and is as such unnecessarily shown as a bottleneck when reviewing the profile. This skews the whole profile and obstructs real problems as the FontConfig setup is not really necessary when not running layout tests.

[Martin Robinson]

It is necessary to produce appropriate test results though, right?

[Zan Dobersek]

When running layout tests, yes.

This shouldn't apply to the performace tests (i.e. when running run-perf-tests and using a profiler), at least at the moment there are no tests testing the performance of anything that strictly relies on specific fonts being present.

I'll think this through before pushing forward - in worst case, the FontConfig functions consuming substantial amount of time could be simply ignored when reviewing the profiles. Unfortunately the perf profiler doesn't provide any obvious mechanisms to ignore specific symbols at either record- or report-time.

[Zan Dobersek]

Comment on attachment 209267 [details]
Patch

I'll spin off the typo fix into a separate patch.

[Martin Robinson]

(In reply to comment #4) 
> This shouldn't apply to the performace tests (i.e. when running run-perf-tests and using a profiler), at least at the moment there are no tests testing the performance of anything that strictly relies on specific fonts being present.

I see. You make a good point. As long as it does not cause tests to start failing, I have no objection.

[Zan Dobersek]

Created attachment 221110 [details]
Patch

[Martin Robinson]

Comment on attachment 221110 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=221110&action=review

> Tools/WebKitTestRunner/InjectedBundle/gtk/ActivateFontsGtk.cpp:87
> +    if (g_getenv("WKTR_SKIP_FONTCONFIG_INITIALIZATION"))
> +        return;
> +

A "WEBKIT" prefix probably makes more sense here, since that's the one we traditionally use.

[Zan Dobersek]

Comment on attachment 221110 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=221110&action=review

>> Tools/WebKitTestRunner/InjectedBundle/gtk/ActivateFontsGtk.cpp:87
>> +
> 
> A "WEBKIT" prefix probably makes more sense here, since that's the one we traditionally use.

I'd then change the variable name to 'WEBKIT_SKIP_WKTR_FONTCONFIG_INITIALIZATION'. Sounds OK?

[Martin Robinson]

(In reply to comment #9)
> (From update of attachment 221110 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=221110&action=review
> 
> >> Tools/WebKitTestRunner/InjectedBundle/gtk/ActivateFontsGtk.cpp:87
> >> +
> > 
> > A "WEBKIT" prefix probably makes more sense here, since that's the one we traditionally use.
> 
> I'd then change the variable name to 'WEBKIT_SKIP_WKTR_FONTCONFIG_INITIALIZATION'. Sounds OK?

I'm not a big fan unnecessary abbreviation, so I prefer WEBKIT_SKIP_WEBKITTESTRUNNER_FONTCONFIG_INITIALIZATION.

[Zan Dobersek]

Committed r161990: <http://trac.webkit.org/changeset/161990>