Bug 227333 – Build Default Metal library offline, and install to WebCore Resources

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGL

[Kyle Piddington]

Build Default Metal library offline, and install to WebCore Resources

[Kyle Piddington]

Created attachment 432120 [details]
Patch

[EWS Watchlist]

Note that there are important steps to take when updating ANGLE. See https://trac.webkit.org/wiki/UpdatingANGLE

[Dean Jackson]

Comment on attachment 432120 [details]
Patch

This looks ok to me, but it would be nice if other people familiar with the build process double checked it. cc-ing them.

[Kyle Piddington]

Created attachment 432133 [details]
Patch

[Tim Horton]

Comment on attachment 432133 [details]
Patch

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

> Source/ThirdParty/ANGLE/ANGLE.xcodeproj/project.pbxproj:6
> -	objectVersion = 46;
> +	objectVersion = 54;

I think you need to revert this to avoid breaking the project for people using older software. And you'll have to keep fighting xcode :(

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/DisplayMtl.mm:990
> +    NSBundle * webcoreBundle = [NSBundle bundleWithIdentifier:@"com.apple.WebCore"];

Nit: No space after the star (is WebKit style, but it looks like ANGLE style agrees), and WebCore is two words (`NSBundle *webCoreBundle`)

Technically this is a layering violation, but maybe it's OK because "where we put ANGLE bits on disk" is kind of irrelevant. Likely the *correct* thing to do is have the client pass you the path to the files, and do that in WebCore somewhere. But this isn't a regression from your patch, so it's fine.

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/DisplayMtl.mm:991
> +    if(webcoreBundle)

nit: space after the if (`if (webCoreBundle)`)

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/DisplayMtl.mm:993
> +    return [NSBundle bundleWithPath:@"/System/Library/PrivateFrameworks/WebCore.framework"];

This path is only accurate on iOS, on macOS it's a subframework inside WebKit.

Another way you can look up the bundle is by a ObjC class you know that comes from it (see e.g. the example of WebCoreBundleFinder in ImageMac, or other uses of bundleForClass: in the tree)

I'm kind of intrigued though -- in what case does bundleWithIdentifier fail? LocalizedStringsCocoa seems to use it without any fallback, so I'd expect that that is OK?

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/DisplayMtl.mm:1001
> +    NSURL * path = [NSURL fileURLWithPath:@"angle-default.metallib" relativeToURL: webcoreBundle.resourceURL ];

nit: again with the stars, also no spaces after colons in objc method invocations nor before the closing ]

[Kimmo Kinnunen]

Comment on attachment 432133 [details]
Patch

Seems overly error prone?

Is there a reason it cannot It be a normal binary blob symbol in the normal compilation and be loaded with something like
newLibraryWithData?
 https://developer.apple.com/documentation/metal/mtldevice/1433391-newlibrarywithdata ?

[Alexey Proskuryakov]

Comment on attachment 432133 [details]
Patch

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

> Source/ThirdParty/ANGLE/ANGLE.xcodeproj/project.pbxproj:3654
> +		FF007C932683E49A00E09E0D /* Move Metal Library */ = {

I have several comments about how this is implemented, but before any of that, is it possible to build into the correct location? Moving manually is trying to play against the Xcode build system.

Most of directory path logic is in xcconfigs, and it's very complex because everything has different paths across platforms and build styles (such as Safari standalone for old OS versions or STP). Doing it differently makes it hard to verify now, and hard to maintain in the future.

[Radar WebKit Bug Importer]

<rdar://problem/79994335>

[Kyle Piddington]

Created attachment 433554 [details]
Patch

[Kyle Piddington]

Created attachment 433557 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 433557 [details]
Patch

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

> Source/ThirdParty/ANGLE/.gitignore:31
> +/src/libANGLE/renderer/metal/shaders/compiled/mtl_default_shaders_compiled.inc

This should probably go into DerivedSources? Generating code in place is fraught with peril.

> Source/ThirdParty/ANGLE/ANGLE.xcodeproj/project.pbxproj:3693
> +				"$(BUILT_PRODUCTS_DIR)/ANGLEMetalLib.metallib",

I didn't examine this patch closely enough yet, is this actually an input, not an output?

This phase clearly depends on create_mtl_internal_shaders.py too. Does it depend on anything else?

> Source/ThirdParty/ANGLE/ANGLE.xcodeproj/project.pbxproj:3698
> +			outputPaths = (
> +			);

Please add the output path too, so that actions that depend on it would be ordered correctly.

> Source/ThirdParty/ANGLE/ANGLE.xcodeproj/project.pbxproj:4218
> +				ALWAYS_SEARCH_USER_PATHS = NO;
> +				CODE_SIGN_STYLE = Automatic;
> +				IPHONEOS_DEPLOYMENT_TARGET = 15.0;
> +				MTL_ENABLE_DEBUG_INFO = INCLUDE_SOURCE;
> +				MTL_FAST_MATH = YES;
> +				PRODUCT_NAME = "$(TARGET_NAME)";

Please put all build settings into xcconfigs.

> Source/ThirdParty/ANGLE/ANGLE.xcodeproj/project.pbxproj:4239
> +				IPHONEOS_DEPLOYMENT_TARGET = 15.0;

In an xcconfig, it will be easier to follow other xcconfigs and to avoid issues like this (obviously, we can't hardcode IPHONEOS_DEPLOYMENT_TARGET = 15.0).

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/DisplayMtl.mm:1007
> +    mtl::AutoObjCPtr<id<MTLLibrary>> mDefaultShaders = mtl::CreateShaderLibraryFromBinary(getMetalDevice(), compiled_shader_binary,
> +                                                                        compiled_shader_binary_len, &err);

Can this call fail? E.g. if the system runs out of file handles?

Even though conditions like that are "beyond repair", we get hard to investigate bugs when they are not handled explicitly.

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/shaders/create_mtl_internal_shaders.py:2
> +# Copyright 2021 The ANGLE Project Authors. All rights reserved.

I'm not certain about what the copyright header should look like, as it's obviously not a standard WebKit one - but then this is a 3rd party library.

[Kyle Piddington]

(In reply to Alexey Proskuryakov from comment #11)
> Comment on attachment 433557 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=433557&action=review
> 
> > Source/ThirdParty/ANGLE/.gitignore:31
> > +/src/libANGLE/renderer/metal/shaders/compiled/mtl_default_shaders_compiled.inc
> 
> This should probably go into DerivedSources? Generating code in place is
> fraught with peril.
> 
Done
> > Source/ThirdParty/ANGLE/ANGLE.xcodeproj/project.pbxproj:3693
> > +				"$(BUILT_PRODUCTS_DIR)/ANGLEMetalLib.metallib",
> 
> I didn't examine this patch closely enough yet, is this actually an input,
> not an output?
> 
Yes, the MetalLib is built in a separate dependency 
> This phase clearly depends on create_mtl_internal_shaders.py too. Does it
> depend on anything else?
> 
Just these files.
> > Source/ThirdParty/ANGLE/ANGLE.xcodeproj/project.pbxproj:3698
> > +			outputPaths = (
> > +			);
> 
> Please add the output path too, so that actions that depend on it would be
> ordered correctly.
> 
Added!
> > Source/ThirdParty/ANGLE/ANGLE.xcodeproj/project.pbxproj:4218
> > +				ALWAYS_SEARCH_USER_PATHS = NO;
> > +				CODE_SIGN_STYLE = Automatic;
> > +				IPHONEOS_DEPLOYMENT_TARGET = 15.0;
> > +				MTL_ENABLE_DEBUG_INFO = INCLUDE_SOURCE;
> > +				MTL_FAST_MATH = YES;
> > +				PRODUCT_NAME = "$(TARGET_NAME)";
> 
> Please put all build settings into xcconfigs.
> 
Done!
> > Source/ThirdParty/ANGLE/ANGLE.xcodeproj/project.pbxproj:4239
> > +				IPHONEOS_DEPLOYMENT_TARGET = 15.0;
> 
> In an xcconfig, it will be easier to follow other xcconfigs and to avoid
> issues like this (obviously, we can't hardcode IPHONEOS_DEPLOYMENT_TARGET =
> 15.0).

> > Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/DisplayMtl.mm:1007
> > +    mtl::AutoObjCPtr<id<MTLLibrary>> mDefaultShaders = mtl::CreateShaderLibraryFromBinary(getMetalDevice(), compiled_shader_binary,
> > +                                                                        compiled_shader_binary_len, &err);
> 
> Can this call fail? E.g. if the system runs out of file handles?
> 
> Even though conditions like that are "beyond repair", we get hard to
> investigate bugs when they are not handled explicitly.
This call can fail, but we should fail 'gracefully' (i.e. we won't crash, we just won't be able to make a context.)
> > Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/shaders/create_mtl_internal_shaders.py:2
> > +# Copyright 2021 The ANGLE Project Authors. All rights reserved.
> 
> I'm not certain about what the copyright header should look like, as it's
> obviously not a standard WebKit one - but then this is a 3rd party library.

This is the same header as other ANGLE generated code.

[Kyle Piddington]

Created attachment 433608 [details]
Patch

[Alexey Proskuryakov]

> This call can fail, but we should fail 'gracefully' (i.e. we won't crash, we
> just won't be able to make a context.)

What are the consequences of setting `mDefaultShadersAsyncInfo->compiled = true` even when it fails?

[Kyle Piddington]

(In reply to Alexey Proskuryakov from comment #14)
> > This call can fail, but we should fail 'gracefully' (i.e. we won't crash, we
> > just won't be able to make a context.)
> 
> What are the consequences of setting `mDefaultShadersAsyncInfo->compiled =
> true` even when it fails?

'Compiled' here is used as a flag for async-awaiting on the default library.
    if (!mDefaultShadersAsyncInfo->compiled)
    {
        // Wait for async compilation
        mDefaultShadersAsyncInfo->cv.wait(lg,
                                          [this] { return mDefaultShadersAsyncInfo->compiled; });
    }

By setting 'compiled', we continue, and then return a nil library. Any GL calls that rely on the default library will return failure.

[Kyle Piddington]

Created attachment 433631 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 433631 [details]
Patch

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

I have few enough comments that I don't need to see the next iteration (please do address these though).

Probably needs someone familiar with WebGL to give final review, and also to give another pass over Xcode configs. Even though I made a lot of comments, I'm not very confident that I didn't overlook anything.

> Source/ThirdParty/ANGLE/.gitignore:31
> +/src/libANGLE/renderer/metal/shaders/compiled/mtl_default_shaders_compiled.inc

This seems unnecessary now.

> Source/ThirdParty/ANGLE/ANGLE.xcodeproj/project.pbxproj:3693
> +				"$(BUILT_PRODUCTS_DIR)/ANGLEMetalLib.metallib",

Please do add create_mtl_internal_shaders.py.

> Source/ThirdParty/ANGLE/ANGLE.xcodeproj/project.pbxproj:3702
> +			shellScript = "echo python \"${SRCROOT}/src/libANGLE/renderer/metal/shaders/create_mtl_internal_shaders.py\" \"$SCRIPT_INPUT_FILE_0\" \"$SCRIPT_OUTPUT_FILE_0\"\npython \"${SRCROOT}/src/libANGLE/renderer/metal/shaders/create_mtl_internal_shaders.py\" \"$SCRIPT_INPUT_FILE_0\" \"$SCRIPT_OUTPUT_FILE_0\"\nrm \"${SCRIPT_INPUT_FILE_0}\"\n";

Why does this rm SCRIPT_INPUT_FILE_0 in the end? This means that incremental builds will always have to redo this work, and that's harmful to build speed.

If the problem is that the file goes into $(BUILT_PRODUCTS_DIR), perhaps that's what needs to change.

> Source/ThirdParty/ANGLE/Configurations/AngleMetalLib.xcconfig:21
> +DEAD_CODE_STRIPPING[config=Debug] = NO;

Why do we want dead code preserved in debug? It's only more harmful there, as it takes more space.

[Kenneth Russell]

Comment on attachment 433631 [details]
Patch

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

Alexey's review is more thorough than I can provide - I don't know Xcode well enough to review those changes - but conceptually this sounds fine. Happy to look at further iterations as well. r+

>> Source/ThirdParty/ANGLE/ANGLE.xcodeproj/project.pbxproj:3702
>> +			shellScript = "echo python \"${SRCROOT}/src/libANGLE/renderer/metal/shaders/create_mtl_internal_shaders.py\" \"$SCRIPT_INPUT_FILE_0\" \"$SCRIPT_OUTPUT_FILE_0\"\npython \"${SRCROOT}/src/libANGLE/renderer/metal/shaders/create_mtl_internal_shaders.py\" \"$SCRIPT_INPUT_FILE_0\" \"$SCRIPT_OUTPUT_FILE_0\"\nrm \"${SCRIPT_INPUT_FILE_0}\"\n";
> 
> Why does this rm SCRIPT_INPUT_FILE_0 in the end? This means that incremental builds will always have to redo this work, and that's harmful to build speed.
> 
> If the problem is that the file goes into $(BUILT_PRODUCTS_DIR), perhaps that's what needs to change.

Agreed - this should avoid overwriting the output file if nothing's changed. Take a look at Source/ThirdParty/ANGLE/adjust-angle-include-paths.py and the Xcode build rule which invokes it to see how it avoids redundant work. (It looks like it caches the file's contents in memory so it can compare with what's currently on disk.) I don't remember who most recently improved the incremental build support (krollin@ maybe?) and have been looking through the revision history of Source/ThirdParty/ANGLE/ANGLE.xcodeproj/project.pbxproj to see.

[Kyle Piddington]

(In reply to Alexey Proskuryakov from comment #17)
> Comment on attachment 433631 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=433631&action=review
> 
> I have few enough comments that I don't need to see the next iteration
> (please do address these though).
> 
> Probably needs someone familiar with WebGL to give final review, and also to
> give another pass over Xcode configs. Even though I made a lot of comments,
> I'm not very confident that I didn't overlook anything.
> 
> > Source/ThirdParty/ANGLE/.gitignore:31
> > +/src/libANGLE/renderer/metal/shaders/compiled/mtl_default_shaders_compiled.inc
> 
> This seems unnecessary now.
> 
Removed
> > Source/ThirdParty/ANGLE/ANGLE.xcodeproj/project.pbxproj:3693
> > +				"$(BUILT_PRODUCTS_DIR)/ANGLEMetalLib.metallib",
> 
> Please do add create_mtl_internal_shaders.py.
> 
Added, though I'm never sure how SCRIPT_INPUT_FILES works for determining build order.
> > Source/ThirdParty/ANGLE/ANGLE.xcodeproj/project.pbxproj:3702
> > +			shellScript = "echo python \"${SRCROOT}/src/libANGLE/renderer/metal/shaders/create_mtl_internal_shaders.py\" \"$SCRIPT_INPUT_FILE_0\" \"$SCRIPT_OUTPUT_FILE_0\"\npython \"${SRCROOT}/src/libANGLE/renderer/metal/shaders/create_mtl_internal_shaders.py\" \"$SCRIPT_INPUT_FILE_0\" \"$SCRIPT_OUTPUT_FILE_0\"\nrm \"${SCRIPT_INPUT_FILE_0}\"\n";
> 
> Why does this rm SCRIPT_INPUT_FILE_0 in the end? This means that incremental
> builds will always have to redo this work, and that's harmful to build speed.
> 
> If the problem is that the file goes into $(BUILT_PRODUCTS_DIR), perhaps
> that's what needs to change.
> 
Fixed with a combination of SKIP_INSTALL and correcting build names.
> > Source/ThirdParty/ANGLE/Configurations/AngleMetalLib.xcconfig:21
> > +DEAD_CODE_STRIPPING[config=Debug] = NO;
> 
> Why do we want dead code preserved in debug? It's only more harmful there,
> as it takes more space.

Removed

[Kyle Piddington]

(In reply to Alexey Proskuryakov from comment #17)
> Comment on attachment 433631 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=433631&action=review
> 
> I have few enough comments that I don't need to see the next iteration
> (please do address these though).
> 
> Probably needs someone familiar with WebGL to give final review, and also to
> give another pass over Xcode configs. Even though I made a lot of comments,
> I'm not very confident that I didn't overlook anything.
> 
> > Source/ThirdParty/ANGLE/.gitignore:31
> > +/src/libANGLE/renderer/metal/shaders/compiled/mtl_default_shaders_compiled.inc
> 
> This seems unnecessary now.
> 
Removed
> > Source/ThirdParty/ANGLE/ANGLE.xcodeproj/project.pbxproj:3693
> > +				"$(BUILT_PRODUCTS_DIR)/ANGLEMetalLib.metallib",
> 
> Please do add create_mtl_internal_shaders.py.
> 
Added, though I'm never sure how SCRIPT_INPUT_FILES works for determining build order.
> > Source/ThirdParty/ANGLE/ANGLE.xcodeproj/project.pbxproj:3702
> > +			shellScript = "echo python \"${SRCROOT}/src/libANGLE/renderer/metal/shaders/create_mtl_internal_shaders.py\" \"$SCRIPT_INPUT_FILE_0\" \"$SCRIPT_OUTPUT_FILE_0\"\npython \"${SRCROOT}/src/libANGLE/renderer/metal/shaders/create_mtl_internal_shaders.py\" \"$SCRIPT_INPUT_FILE_0\" \"$SCRIPT_OUTPUT_FILE_0\"\nrm \"${SCRIPT_INPUT_FILE_0}\"\n";
> 
> Why does this rm SCRIPT_INPUT_FILE_0 in the end? This means that incremental
> builds will always have to redo this work, and that's harmful to build speed.
> 
> If the problem is that the file goes into $(BUILT_PRODUCTS_DIR), perhaps
> that's what needs to change.
> 
Fixed with a combination of SKIP_INSTALL and correcting build names.
> > Source/ThirdParty/ANGLE/Configurations/AngleMetalLib.xcconfig:21
> > +DEAD_CODE_STRIPPING[config=Debug] = NO;
> 
> Why do we want dead code preserved in debug? It's only more harmful there,
> as it takes more space.

Removed

[Kyle Piddington]

Created attachment 433654 [details]
Patch

[Kyle Piddington]

Created attachment 433660 [details]
Patch for landing

[EWS]

Committed r279980 (239723@main): <https://commits.webkit.org/239723@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 433660 [details].

[Truitt Savell]

Reverted r279980 for reason:

Broke Internal Builds

Committed r279986 (239729@main): <https://commits.webkit.org/239729@main>

[Kyle Piddington]

Created attachment 433693 [details]
Patch

[Dean Jackson]

Comment on attachment 433693 [details]
Patch

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

> Source/ThirdParty/ANGLE/ANGLE.xcodeproj/project.pbxproj:3704
> +			shellScript = "if [ \"${ACTION}\" = \"build\" ] || [ \"${ACTION}\" = \"install\" ]; then\necho python \"${SRCROOT}/src/libANGLE/renderer/metal/shaders/create_mtl_internal_shaders.py\" \"$SCRIPT_INPUT_FILE_0\" \"$SCRIPT_OUTPUT_FILE_0\"\npython \"${SRCROOT}/src/libANGLE/renderer/metal/shaders/create_mtl_internal_shaders.py\" \"$SCRIPT_INPUT_FILE_0\" \"$SCRIPT_OUTPUT_FILE_0\"\nfi\n";

We probably don't need to echo the command, but that's ok.

[EWS]

Committed r280010 (239752@main): <https://commits.webkit.org/239752@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 433693 [details].