Bug 238119 – Enable PGO when building for release and production

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Wenson Hsieh]

.

[Wenson Hsieh]

Created attachment 455180 [details]
Test attachment

[Wenson Hsieh]

Created attachment 455202 [details]
Test attachment

[Wenson Hsieh]

rdar://90182309

[Wenson Hsieh]

Created attachment 455208 [details]
Test attachment

[Saam Barati]

r=me. (Patch review isn't actually letting me set the r+ flag though b/c the patch size)

[Wenson Hsieh]

(In reply to Saam Barati from comment #5)
> r=me. (Patch review isn't actually letting me set the r+ flag though b/c the
> patch size)

Thanks for the review!

In lieu of EWS, I ran a (large) subset of the layout tests on macOS locally, and confirmed that it doesn't break anything 🤞🏻.

I'll commit this manually, and will also be keeping an eye out for the post-commit bots (and EWS).

[Wenson Hsieh]

Committed r291558 (248660@trunk): <https://commits.webkit.org/248660@trunk>

[Jonathan Bedard]

Committed r291559 (248661@trunk): <https://commits.webkit.org/248661@trunk>

[Aakash Jain]

For what it's worth, EWS also has a limit of 100 MB (for total patch size) as set in https://github.com/WebKit/WebKit/blob/main/Tools/CISupport/ews-build/master.cfg#L26

That's why EWS wasn't run on this patch.

[Geoffrey Garen]

r=me on "Don't limit to ARM Macs"

Not sure how to set r+, or how to land this patch, with the 100MB file

[Saam Barati]

(In reply to Aakash Jain from comment #9)
> For what it's worth, EWS also has a limit of 100 MB (for total patch size)
> as set in
> https://github.com/WebKit/WebKit/blob/main/Tools/CISupport/ews-build/master.
> cfg#L26
> 
> That's why EWS wasn't run on this patch.

Can we remove that limit?

[Wenson Hsieh]

Reopening to attach new patch.

[Wenson Hsieh]

Created attachment 455314 [details]
Test attachment

[Wenson Hsieh]

(In reply to Wenson Hsieh from comment #13)
> Created attachment 455314 [details]
> v2

(This version of the patch includes only a tarballs of the raw profiling data, which brings the in-repo size down from 300 MB to about 44 MB).

[Wenson Hsieh]

Created attachment 455386 [details]
v3

[Wenson Hsieh]

(In reply to Wenson Hsieh from comment #15)
> Created attachment 455386 [details]
> v3

This latest iteration of the patch does not include the JavaScriptCore/WebCore/WebKit profiling data at all, and instead only adds build system support for incorporating compressed .profdata files when building for Release/Production configurations. If the appropriate data can't be found, we simply fall back to a tiny, (mostly) empty profdata file.

On the appropriate build configurations, profiling data will be downloaded prior to building the WebKit stack instead of falling back to the empty profiling data.

[Wenson Hsieh]

Comment on attachment 455386 [details]
v3

This (hopefully)

[Geoffrey Garen]

Comment on attachment 455386 [details]
v3

r=me

Seems like a fine approach.

[Alexey Proskuryakov]

Comment on attachment 455386 [details]
v3

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

I like this approach.

> Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:12132
> +				"$(SRCROOT)/../../Tools/Profiling/Empty.profdata",

This file won't be available in production builds, which I think results in the behavior we want (failing the build).

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:38745
> +			shellScript = "if [ \"${ACTION}\" = \"installhdrs\" -o \"${ACTION}\" = \"installapi\" ]; then\n    exit 0;\nfi\n\nif [ -z \"${PROFILE_DATA_FLAGS}\" ]; then\n    exit 0;\nfi\n\nif [ -f \"${SRCROOT}/WebCore.profdata.tar.gz\" ]; then\n    echo \"Extracting ${SRCROOT}/WebCore.profdata.tar.gz\";\n    tar -xzvf ${SRCROOT}/WebCore.profdata.tar.gz -C ${BUILT_PRODUCTS_DIR}/DerivedSources/WebCore;\n    exit 0;\nfi\n\necho \"${SRCROOT}/WebCore.profdata.tar.gz does not exist - falling back to empty profiling data\";\ncp ${SRCROOT}/../../Tools/Profiling/Empty.profdata ${BUILT_PRODUCTS_DIR}/DerivedSources/WebCore/WebCore.profdata;\n";

I don't quite understand how git-lfs works, but won't ${SRCROOT}/WebCore.profdata.tar.gz actually exist as a stub?

Longer term, we probably need multiple profdata files (there's no way iOS and macOS profiles are the same, for example).

[Wenson Hsieh]

Comment on attachment 455386 [details]
v3

Thank you for taking a look!

After offline discussion with Geoff, Alexey, and (many) others, I think we've all agreed on a way forward here — to:

• Upload the profiling data to WebKitAdditions using `git lfs`, and copy into the Apple internal SDK.
• Make build scripts look for the profiling data in the build directory (or SDK, if it's not in the build directory).
• If the profiling data can't be found for a (non-production) release build, then fall back to Empty.profdata in Tools.
• If the profiling data can't be found for a production build, then exit with a nonzero error code (and a descriptive message).

I'll upload a v4 of patch that does this, momentarily!

[Wenson Hsieh]

Created attachment 455539 [details]
v4

[Wenson Hsieh]

(In reply to Wenson Hsieh from comment #21)
> Created attachment 455539 [details]
> v4

(The .pbxproj format makes it kind of annoying to read the build phase script, so I put it here as well, for reviewers' convenience)

```
if [ "${ACTION}" = "installhdrs" -o "${ACTION}" = "installapi" ]; then
    exit 0;
fi

if [ -z "${PROFILE_DATA_FLAGS}" ]; then
    exit 0;
fi

RELATIVE_PROFILE_DATA_PATH="usr/local/include/WebKitAdditions/Profiling/JavaScriptCore.profdata.tar.gz";
ABSOLUTE_PROFILE_DATA_PATH="${BUILT_PRODUCTS_DIR}/${RELATIVE_PROFILE_DATA_PATH}";

if [ ! -f "${ABSOLUTE_PROFILE_DATA_PATH}" ]; then
    ABSOLUTE_PROFILE_DATA_PATH="${SDK_DIR}/${RELATIVE_PROFILE_DATA_PATH}";
fi

if [ ! -f "${ABSOLUTE_PROFILE_DATA_PATH}" ] || [ $(wc -c <${ABSOLUTE_PROFILE_DATA_PATH}) -lt 4096 ]; then
    if [ "${CONFIGURATION}" = "Production" ]; then
        echo "Error: production build missing profiling data at ${ABSOLUTE_PROFILE_DATA_PATH}";
        exit 1;
    fi
    echo "Missing or invalid profiling data at ${ABSOLUTE_PROFILE_DATA_PATH} - falling back to empty file";
    cp ${SRCROOT}/../../Tools/Profiling/Empty.profdata ${BUILT_PRODUCTS_DIR}/DerivedSources/JavaScriptCore/JavaScriptCore.profdata;
else
    echo "Copying profiling data at ${ABSOLUTE_PROFILE_DATA_PATH}";
    tar -xzvf ${ABSOLUTE_PROFILE_DATA_PATH} -C ${BUILT_PRODUCTS_DIR}/DerivedSources/JavaScriptCore;
fi
```

[Alexey Proskuryakov]

>        echo "Error: production build missing profiling data at ${ABSOLUTE_PROFILE_DATA_PATH}";

This error seems slightly misleading, should be saying that it's missing at both RELATIVE_PROFILE_DATA_PATH and ABSOLUTE_PROFILE_DATA_PATH.

The plan looks good to me overall. Please build various configurations before landing (iOS, watchOS, macCatalyst release, debug and production come to mind).

It's not great that names of the profdata files are so generic - there is no affordance to make them CPU architecture specific, or different between AppKit and macCatalyst.

[Alexey Proskuryakov]

Comment on attachment 455539 [details]
v4

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

> Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:12131
> +				"$(BUILT_PRODUCTS_DIR)/usr/local/include/WebKitAdditions/Profiling/JavaScriptCore.profdata.tar.gz",

This is only one of the two paths the script is looking at. I think that we need both as inputs (same in other projects).

> Tools/ChangeLog:9
> +        Add an empty profiling data file that can be used as a fallback for production and release builds, when the

The ChangeLog is stale, as we discussed. Empty.profdata can't be used as a fallback for production builds.

[Wenson Hsieh]

(In reply to Alexey Proskuryakov from comment #23)
> >        echo "Error: production build missing profiling data at ${ABSOLUTE_PROFILE_DATA_PATH}";
> 
> This error seems slightly misleading, should be saying that it's missing at
> both RELATIVE_PROFILE_DATA_PATH and ABSOLUTE_PROFILE_DATA_PATH.

Sounds good to me! Will tweak the error message to mention that the profiling data is missing at both "${BUILT_PRODUCTS_DIR}/${RELATIVE_PROFILE_DATA_PATH}" and the current value of "${ABSOLUTE_PROFILE_DATA_PATH}".

> 
> The plan looks good to me overall. Please build various configurations
> before landing (iOS, watchOS, macCatalyst release, debug and production come
> to mind).

Will do! (..and will also coordinate with ops folks to make sure this change doesn't break any of our test and perf automation).

> 
> It's not great that names of the profdata files are so generic - there is no
> affordance to make them CPU architecture specific, or different between
> AppKit and macCatalyst.

While generic, our experiments have shown that this profiling data significantly improves performance across all architectures and platforms where this patch enables PGO. I think in the future, we should do further experiments to see if we can get more wins out of crafting PGO files specific to architecture and platform (at which point we could then add the affordances necessary to maintain and deploy the relevant PGO file, given something like the target triple, or perhaps just architecture + platform).

[Wenson Hsieh]

(In reply to Alexey Proskuryakov from comment #24)
> Comment on attachment 455539 [details]
> v4
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=455539&action=review
> 
> > Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:12131
> > +				"$(BUILT_PRODUCTS_DIR)/usr/local/include/WebKitAdditions/Profiling/JavaScriptCore.profdata.tar.gz",
> 
> This is only one of the two paths the script is looking at. I think that we
> need both as inputs (same in other projects).

Good catch — will add the SDK WKA paths here as well.

> 
> > Tools/ChangeLog:9
> > +        Add an empty profiling data file that can be used as a fallback for production and release builds, when the
> 
> The ChangeLog is stale, as we discussed. Empty.profdata can't be used as a
> fallback for production builds.

Whoops! Will update the ChangeLog to reflect the actual behavior.

[Wenson Hsieh]

Created attachment 455559 [details]
v4 (address feedback)

[Alexey Proskuryakov]

> maintain and deploy the relevant PGO file, given something like the target triple, or perhaps just architecture + platform).

We don't need the platform here - WebKitAdditions can and should decide which platform to copy profiles for, because it's building for the same platform as WebKit. We only need to differentiate by file name when there are multiple in the same build (so, architecture and macCatalyst variant are the only ones that come to mind).

[Wenson Hsieh]

(In reply to Alexey Proskuryakov from comment #28)
> > maintain and deploy the relevant PGO file, given something like the target triple, or perhaps just architecture + platform).
> 
> We don't need the platform here - WebKitAdditions can and should decide
> which platform to copy profiles for, because it's building for the same
> platform as WebKit. We only need to differentiate by file name when there
> are multiple in the same build (so, architecture and macCatalyst variant are
> the only ones that come to mind).

I see — yes, that makes sense! (and also lets us avoid making the build script more complicated)

[Alexey Proskuryakov]

Comment on attachment 455559 [details]
v4 (address feedback)

Looks good to me. Marking cq- for now as a reminder to wait for bot updates, and to perform all of the builds for testing.

As this is new ground, others are more than welcome to chime in.

[Wenson Hsieh]

(In reply to Alexey Proskuryakov from comment #30)
> Comment on attachment 455559 [details]
> v4 (address feedback)
> 
> Looks good to me. Marking cq- for now as a reminder to wait for bot updates,
> and to perform all of the builds for testing.
> 
> As this is new ground, others are more than welcome to chime in.

Sounds good — thank you for the review!

(Adding Ryan and bot watchers as well, for visibility)

[Wenson Hsieh]

Created attachment 455674 [details]
Adjust decompression

[Wenson Hsieh]

Comment on attachment 455674 [details]
Adjust decompression

(Oops, I forgot to update the input file list for the build scripts — need to fix that)

[Wenson Hsieh]

Created attachment 455675 [details]
Adjust decompression

[Wenson Hsieh]

Created attachment 455686 [details]
Rebase on trunk

[Wenson Hsieh]

Created attachment 455693 [details]
Rebase (again)

[Wenson Hsieh]

Created attachment 455702 [details]
Fix production iOS builds

[Alexey Proskuryakov]

Comment on attachment 455702 [details]
Fix production iOS builds

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

> Source/JavaScriptCore/Configurations/JavaScriptCore.xcconfig:55
> +OTHER_CPLUSPLUSFLAGS = $(inherited) -fno-slp-vectorize --system-header-prefix=unicode/ $(PROFILE_DATA_FLAGS);

Just noticed this - why is it sufficient to only add the flag to C++ files?

[Wenson Hsieh]

(In reply to Alexey Proskuryakov from comment #38)
> Comment on attachment 455702 [details]
> Fix production iOS builds
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=455702&action=review
> 
> > Source/JavaScriptCore/Configurations/JavaScriptCore.xcconfig:55
> > +OTHER_CPLUSPLUSFLAGS = $(inherited) -fno-slp-vectorize --system-header-prefix=unicode/ $(PROFILE_DATA_FLAGS);
> 
> Just noticed this - why is it sufficient to only add the flag to C++ files?

This was not an intentional omission (though, given that the vast majority of our source files are C++ anyways, I wouldn't expect it to make much difference). I'll add it to `OTHER_CFLAGS` as well, with:

```
OTHER_CFLAGS = $(inherited) … $(PROFILE_DATA_FLAGS);
```

[Wenson Hsieh]

(In reply to Wenson Hsieh from comment #39)
> (In reply to Alexey Proskuryakov from comment #38)
> > Comment on attachment 455702 [details]
> > Fix production iOS builds
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=455702&action=review
> > 
> > > Source/JavaScriptCore/Configurations/JavaScriptCore.xcconfig:55
> > > +OTHER_CPLUSPLUSFLAGS = $(inherited) -fno-slp-vectorize --system-header-prefix=unicode/ $(PROFILE_DATA_FLAGS);
> > 
> > Just noticed this - why is it sufficient to only add the flag to C++ files?
> 
> This was not an intentional omission (though, given that the vast majority
> of our source files are C++ anyways, I wouldn't expect it to make much
> difference). I'll add it to `OTHER_CFLAGS` as well, with:
> 
> ```
> OTHER_CFLAGS = $(inherited) … $(PROFILE_DATA_FLAGS);
> ```

Oddly, enabling this in WebKit2 leads to:

```
In file included from /Users/whsieh/Build/Release/DerivedSources/WebKit/unified-sources/UnifiedSource13-mm.mm:1:
/Users/whsieh/work/OpenSource/Source/WebKit/Shared/Cocoa/SandboxExtensionCocoa.mm:64:9: error: use of undeclared identifier 'memset_s'
        memset_s(m_token, length, 0, length);
1 error generated.
```

It even happens if I simply add the following, without the -fprofile-* flags, so I think it's unrelated to profiling (and instead related to how the OTHER_CFLAGS build setting in WebKit2 is set...)

```
OTHER_CFLAGS = $(inherited);
```

Going to leave it out of there for now, and see what the bots think.

[Wenson Hsieh]

(In reply to Wenson Hsieh from comment #40)
> 
> Going to leave it out of there for now, and see what the bots think.

Actually, I think I figured it out. So in BaseTarget.xcconfig, OTHER_CPLUSPLUSFLAGS includes OTHER_CFLAGS already:

```
OTHER_CPLUSPLUSFLAGS = $(OTHER_CFLAGS) …;
```

...which seems to cause problems if WebKit.xcconfig then goes along and tries to change both OTHER_CFLAGS and OTHER_CPLUSPLUSFLAGS.

To fix this, I just moved the part that adds $(PROFILE_DATA_FLAGS) to OTHER_CFLAGS to Source/WebKit/Configurations/BaseTarget.xcconfig instead, and then OTHER_CPLUSPLUSFLAGS just gets it by virtue of including all of $(OTHER_CFLAGS).

[Wenson Hsieh]

Created attachment 455717 [details]
Add flag to OTHER_CFLAGS too

[Alexey Proskuryakov]

> moved the part that adds $(PROFILE_DATA_FLAGS) to OTHER_CFLAGS to Source/WebKit/Configurations/BaseTarget.xcconfig

This means that the profile will be in use when building all of WebKit project targets, not just the framework. Which is probably harmless, but does not feel quite right.

It is a little mysterious to me why we need to add System.framework via -isystem.

For some context, OTHER_CFLAGS is the default value for OTHER_CPLUSPLUSFLAGS, so if we don't define OTHER_CPLUSPLUSFLAGS as all, it matches. But once we start defining it, we need everything, they are not merged together.

[Wenson Hsieh]

(In reply to Alexey Proskuryakov from comment #43)
> > moved the part that adds $(PROFILE_DATA_FLAGS) to OTHER_CFLAGS to Source/WebKit/Configurations/BaseTarget.xcconfig
> 
> This means that the profile will be in use when building all of WebKit
> project targets, not just the framework. Which is probably harmless, but
> does not feel quite right.

I agree it is a bit odd. Is it fine here, or should I move this back to WebKit.xcconfig and just make it apply to `OTHER_CPLUSPLUSFLAGS`?

> 
> It is a little mysterious to me why we need to add System.framework via
> -isystem.
> 
> For some context, OTHER_CFLAGS is the default value for
> OTHER_CPLUSPLUSFLAGS, so if we don't define OTHER_CPLUSPLUSFLAGS as all, it
> matches. But once we start defining it, we need everything, they are not
> merged together.

So it looks like the -isystem was added in r213492 to fix the build after r213483. It does sound like we should be able to omit this line altogether if we add the profiling flags to OTHER_CFLAGS, since OTHER_CPLUSPLUSFLAGS will simply get both the profiling flags and the System.framework header includes from OTHER_CFLAGS.

I was able to still build internal iOS and macOS locally after removing this line — let's see what EWS says.

[Wenson Hsieh]

Created attachment 455779 [details]
For EWS

[Alexey Proskuryakov]

> I agree it is a bit odd. Is it fine here, or should I move this back to WebKit.xcconfig and just make it apply to `OTHER_CPLUSPLUSFLAGS`?

It would certainly be nicer and consistent with other projects if moved to WebKit.xcconfig. But I don't want to delay landing this.

[Wenson Hsieh]

(In reply to Alexey Proskuryakov from comment #46)
> > I agree it is a bit odd. Is it fine here, or should I move this back to WebKit.xcconfig and just make it apply to `OTHER_CPLUSPLUSFLAGS`?
> 
> It would certainly be nicer and consistent with other projects if moved to
> WebKit.xcconfig. But I don't want to delay landing this.

Sounds good — I've filed <https://webkit.org/b/238390> to track this. Thanks!

[Wenson Hsieh]

Committed r291889 (248888@trunk): <https://commits.webkit.org/248888@trunk>

[Wenson Hsieh]

Comment on attachment 455779 [details]
For EWS

Landed in: https://commits.webkit.org/r291889