Bug 183285 – [GTK] Switch to use always complex text code path

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Carlos Garcia Campos]

Now that we have branched for 2.20, it's a good time to try using complex text path always. We can simply force it for GTK+ port and see how it works for the whole release cycle and if we don't notice any issues or performance regressions we release 2.22 with complex text path forced. We can add a debug env variable to switch back to auto without having to recompile. We should also keep the auto mode for the layout tests to avoid having to rebaseline a lot of tests. After 2.22 is released we can make a final decision and remove the env variable.

[Carlos Garcia Campos]

Created attachment 334892 [details]
Patch

[Michael Catanzaro]

Comment on attachment 334892 [details]
Patch

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

> Source/WebKit/UIProcess/gtk/WebProcessPoolGtk.cpp:96
> +    const char* forceComplexText = getenv("WEBKIT_FORCE_COMPLEX_TEXT");

The semantics of this are backwards.

Should be either WEBKIT_FORCE_SIMPLE_TEXT=1 or WEBKIT_DISABLE_COMPLEX_TEXT=1.

Let's not check for 0.

> Tools/ChangeLog:8
> +        Keep the auto mode for the layout tests to avoid having to rebaseline a lot of tests.

I don't think this is a great idea... now we are not testing the code that users are actually running anymore. I would do a rebaseline at the same time.

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #2)
> Comment on attachment 334892 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=334892&action=review
> 
> > Source/WebKit/UIProcess/gtk/WebProcessPoolGtk.cpp:96
> > +    const char* forceComplexText = getenv("WEBKIT_FORCE_COMPLEX_TEXT");
> 
> The semantics of this are backwards.
> 
> Should be either WEBKIT_FORCE_SIMPLE_TEXT=1 or WEBKIT_DISABLE_COMPLEX_TEXT=1.

This doesn't force simple text and it doesn't disable complex text either. It either forces complex text or leaves the current auto where complex or simple is used depending on the context.

> Let's not check for 0.

I don't see why.

> > Tools/ChangeLog:8
> > +        Keep the auto mode for the layout tests to avoid having to rebaseline a lot of tests.
> 
> I don't think this is a great idea... now we are not testing the code that
> users are actually running anymore. I would do a rebaseline at the same time.

The default is auto, so complex test is still tested when required. Most of the changes that we will have to rebaseline are in text that doesn't affect the test at all, like text explaining what the test does, for example. The idea is to try it our during the next release cycle in the unstable releases, and once we make a decision we do the rebaseline. I don't want to waste the time doing a huge rebaseline to roll out the patch tomorrow.

[Carlos Garcia Campos]

Note that my main concern is performance, and our layout tests don't help with that, but real usage does.

[Carlos Garcia Campos]

This is not different than GTK_OVERLAY_SCROLLING, for example, that defaults to enabled when not present or != "0" is used. You need to export GTK_OVERLAY_SCROLLING=0 to disable it.

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #3)
> This doesn't force simple text and it doesn't disable complex text either.
> It either forces complex text or leaves the current auto where complex or
> simple is used depending on the context.

Good point.

Other options:

WEBKIT_DO_NOT_FORCE_COMPLEX_TEXT=1
WEBKIT_ALLOW_SIMPLE_TEXT=1

> > Let's not check for 0.
> 
> I don't see why.

It's very confusing to have an environment variable that only works when set to 0.

> Most of
> the changes that we will have to rebaseline are in text that doesn't affect
> the test at all, like text explaining what the test does, for example. The
> idea is to try it our during the next release cycle in the unstable
> releases, and once we make a decision we do the rebaseline. I don't want to
> waste the time doing a huge rebaseline to roll out the patch tomorrow.

Fair enough, as long as you're confident you won't forget to complete this before 2.22.

One more thing. The most likely reason to revert this change would be if we notice performance problems. But we are not actually going to notice performance problems, not without real perf testing. We do have a perf bot that I never look at, but it won't be able to test this change because the environment variable will be set in the test runner. How do you plan to evaluate the performance impact?

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #5)
> This is not different than GTK_OVERLAY_SCROLLING, for example, that defaults
> to enabled when not present or != "0" is used. You need to export
> GTK_OVERLAY_SCROLLING=0 to disable it.

True, but that seems bad to me. Let's not copy that example, when it is *simple* to avoid. ;) WEBKIT_ALLOW_SIMPLE_TEXT=1 is fine, right?

It's not a big deal, since we don't plan to keep this environment variable all the way until 2.22, but it would be nicer IMO.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #6)
> How do you plan to evaluate the performance impact?

Myles needs to do similar testing in https://bugs.webkit.org/show_bug.cgi?id=178960#c11, so we could ask him for advice.

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #6)
> (In reply to Carlos Garcia Campos from comment #3)
> > This doesn't force simple text and it doesn't disable complex text either.
> > It either forces complex text or leaves the current auto where complex or
> > simple is used depending on the context.
> 
> Good point.
> 
> Other options:
> 
> WEBKIT_DO_NOT_FORCE_COMPLEX_TEXT=1
> WEBKIT_ALLOW_SIMPLE_TEXT=1
> 
> > > Let's not check for 0.
> > 
> > I don't see why.
> 
> It's very confusing to have an environment variable that only works when set
> to 0.

I don't thin it's confusing and it's consistent with other env vars like force AC where we also check for 0, the only difference is that in this case the default is disabled instead of enabled.
 
> > Most of
> > the changes that we will have to rebaseline are in text that doesn't affect
> > the test at all, like text explaining what the test does, for example. The
> > idea is to try it our during the next release cycle in the unstable
> > releases, and once we make a decision we do the rebaseline. I don't want to
> > waste the time doing a huge rebaseline to roll out the patch tomorrow.
> 
> Fair enough, as long as you're confident you won't forget to complete this
> before 2.22.

Sure.

> One more thing. The most likely reason to revert this change would be if we
> notice performance problems. But we are not actually going to notice
> performance problems, not without real perf testing. We do have a perf bot
> that I never look at, but it won't be able to test this change because the
> environment variable will be set in the test runner. How do you plan to
> evaluate the performance impact?

I'm not worried about a performance regression, it's not a problem if complex text is a bit slower. My concern is only if performance i really noticeable and affects real usage. If we think a particular website is too slow, we can try disabling the complex text force and see if there's any significant difference.

[Carlos Garcia Campos]

We can run the perf tests with complex text forced, note that WTR doesn't override the variable if present.

[Carlos Garcia Campos]

Committed r230559: <https://trac.webkit.org/changeset/230559>