Bug 205787 – Add --force-optimization-level option to Tools/Scripts/set-webkit-configuration.

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Lam]

Usage:
    --forceOpt=<opt>        Force optimization: O3, O2, O1, O0, Os, Ofast, or none

This can be used to force debug builds to be built with a higher level optimization so that tests can run to completion faster.

It can also be useful as a simple way to force release builds to be built with different optimization levels for performance comparison.

[Mark Lam]

Created attachment 386817 [details]
proposed patch.

[Alexey Proskuryakov]

Comment on attachment 386817 [details]
proposed patch.

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

> Tools/ChangeLog:9
> +            --forceOpt=<opt>    Force optimization: O3, O2, O1, O0, Os, Ofast, or none

I don't see a reason to abbreviate "optimization level".

> Tools/ChangeLog:12
> +        This can be used to force debug builds to be built with a higher level optimization
> +        so that tests can run to completion faster.

This should just be -Og by default, anywhere where it's supported. I think that we have a bug about this somewhere, but can't find it now.

> Tools/ChangeLog:15
> +        It can also be useful as a simple way to force release builds to be built with
> +        different optimization levels for performance comparison.

Not sure if this is useful enough to have a feature. Modifying multiple xcconfigs is annoying, but seems OK if we do that once in a decade or so.

> Tools/Scripts/webkitdirs.pm:885
> +    push @options, ("-xcconfig", baseProductDir() . "/force-opt.xcconfig") if $forceOptIsEnabled;

Would it be enough to have the option on command line, like WK_LTO_MODE below?

[Mark Lam]

Comment on attachment 386817 [details]
proposed patch.

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

>> Tools/ChangeLog:9
>> +            --forceOpt=<opt>    Force optimization: O3, O2, O1, O0, Os, Ofast, or none
> 
> I don't see a reason to abbreviate "optimization level".

=(

>> Tools/ChangeLog:12
>> +        so that tests can run to completion faster.
> 
> This should just be -Og by default, anywhere where it's supported. I think that we have a bug about this somewhere, but can't find it now.

Debug builds are currently built with -O0 by default.  This patch does not affect that.

>> Tools/ChangeLog:15
>> +        different optimization levels for performance comparison.
> 
> Not sure if this is useful enough to have a feature. Modifying multiple xcconfigs is annoying, but seems OK if we do that once in a decade or so.

I don't expect it to be commonly used.  This is just a side benefit of having this facility.  With this, we can now do a quick perf comparison test without having to jump through the hoops of modifying al the xcconfigs.  If it does turn out to be profitable, we can then justify invest in modifying the xcconfigs.

>> Tools/Scripts/webkitdirs.pm:885
>> +    push @options, ("-xcconfig", baseProductDir() . "/force-opt.xcconfig") if $forceOptIsEnabled;
> 
> Would it be enough to have the option on command line, like WK_LTO_MODE below?

yes, that appears to work.  I'll redo the patch.

[Mark Lam]

Created attachment 386866 [details]
proposed patch.

[Saam Barati]

Comment on attachment 386817 [details]
proposed patch.

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

>>> Tools/ChangeLog:12
>>> +        so that tests can run to completion faster.
>> 
>> This should just be -Og by default, anywhere where it's supported. I think that we have a bug about this somewhere, but can't find it now.
> 
> Debug builds are currently built with -O0 by default.  This patch does not affect that.

Out of curiosity, what's "Og"?

>>> Tools/ChangeLog:15
>>> +        different optimization levels for performance comparison.
>> 
>> Not sure if this is useful enough to have a feature. Modifying multiple xcconfigs is annoying, but seems OK if we do that once in a decade or so.
> 
> I don't expect it to be commonly used.  This is just a side benefit of having this facility.  With this, we can now do a quick perf comparison test without having to jump through the hoops of modifying al the xcconfigs.  If it does turn out to be profitable, we can then justify invest in modifying the xcconfigs.

This actually seems like the perfect option for EWS to use on debug builds to make debug testing go faster. EWS doesn't need O0 for stepping through with a debugger.

> Tools/ChangeLog:18
> +        Setting --forceOpt=none restores the default optimization levels.  Of course,
> +        the build targets need to be rebuilt for this to take effect.

why not --no-forceOpt for consistency with other options?

[Saam Barati]

Comment on attachment 386817 [details]
proposed patch.

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

>> Tools/ChangeLog:18
>> +        the build targets need to be rebuilt for this to take effect.
> 
> why not --no-forceOpt for consistency with other options?

I see that this is consistent with LTO options.

[Saam Barati]

Comment on attachment 386866 [details]
proposed patch.

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

> Tools/Scripts/set-webkit-configuration:41
> +  --forceOptimizationLevel=<level>   Optimization level: O3, O2, O1, O0, Os, Ofast, Og, or none

why is this camel case? shouldn't this be "--force-optimization-level" to be consistent with other options?

> Tools/Scripts/webkitdirs.pm:429
> +    return if defined $forceOptimizationLevel;

would this ever actually be defined in memory? What if it were "none" here? Shouldn't we just always do below?

> Tools/Scripts/webkitdirs.pm:435
> +        chomp $forceOptimizationLevel;

small nit: why write the newline to the file and then do this? Why not just omit "\n" from the file?

[Saam Barati]

Comment on attachment 386866 [details]
proposed patch.

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

> Tools/ChangeLog:3
> +        Add --forceOpt option to Tools/Scripts/set-webkit-configuration.

you should also update the name of this bug to be consistent with the option

[Mark Lam]

Comment on attachment 386866 [details]
proposed patch.

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

>> Tools/Scripts/set-webkit-configuration:41
>> +  --forceOptimizationLevel=<level>   Optimization level: O3, O2, O1, O0, Os, Ofast, Og, or none
> 
> why is this camel case? shouldn't this be "--force-optimization-level" to be consistent with other options?

:( even more characters.  I'll change it.

>> Tools/Scripts/webkitdirs.pm:429
>> +    return if defined $forceOptimizationLevel;
> 
> would this ever actually be defined in memory? What if it were "none" here? Shouldn't we just always do below?

My understanding is that this is equivalent to init_once.  If $forceOptimizationLevel is already computed, we don't have to compute it again.  For the "none" case, I guess it will always redo this computation (same bug as the LTO case).  But since determineForceOptimizationLevel() isn't called that often (my estimate is that it's only called once), I'm going to just leave this as is.

>> Tools/Scripts/webkitdirs.pm:435
>> +        chomp $forceOptimizationLevel;
> 
> small nit: why write the newline to the file and then do this? Why not just omit "\n" from the file?

Because "cat ForceOptimizationLevel" can print a weird character after the optimization level if I don't terminate it with '\n'.  I think this is a small price to pay to have it behave nicer.

[Mark Lam]

Thanks for the review.  Landed in r254080: <http://trac.webkit.org/r254080>.

[Radar WebKit Bug Importer]

<rdar://problem/58351407>