Bug 227333 - Build Default Metal library offline, and install to WebCore Resources
Summary: Build Default Metal library offline, and install to WebCore Resources
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kyle Piddington
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-06-23 17:32 PDT by Kyle Piddington
Modified: 2021-07-16 16:59 PDT (History)
9 users (show)

See Also:


Attachments
Patch (10.18 KB, patch)
2021-06-23 17:34 PDT, Kyle Piddington
no flags Details | Formatted Diff | Diff
Patch (10.83 KB, patch)
2021-06-23 23:19 PDT, Kyle Piddington
no flags Details | Formatted Diff | Diff
Patch (16.38 KB, patch)
2021-07-14 18:54 PDT, Kyle Piddington
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (19.86 KB, patch)
2021-07-14 19:24 PDT, Kyle Piddington
no flags Details | Formatted Diff | Diff
Patch (21.33 KB, patch)
2021-07-15 12:05 PDT, Kyle Piddington
no flags Details | Formatted Diff | Diff
Patch (21.50 KB, patch)
2021-07-15 15:55 PDT, Kyle Piddington
no flags Details | Formatted Diff | Diff
Patch (21.59 KB, patch)
2021-07-15 21:50 PDT, Kyle Piddington
no flags Details | Formatted Diff | Diff
Patch for landing (21.59 KB, patch)
2021-07-15 23:49 PDT, Kyle Piddington
no flags Details | Formatted Diff | Diff
Patch (21.70 KB, patch)
2021-07-16 12:45 PDT, Kyle Piddington
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle Piddington 2021-06-23 17:32:12 PDT
Build Default Metal library offline, and install to WebCore Resources
Comment 1 Kyle Piddington 2021-06-23 17:34:10 PDT
Created attachment 432120 [details]
Patch
Comment 2 EWS Watchlist 2021-06-23 17:35:02 PDT
Note that there are important steps to take when updating ANGLE. See https://trac.webkit.org/wiki/UpdatingANGLE
Comment 3 Dean Jackson 2021-06-23 19:13:22 PDT
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.
Comment 4 Kyle Piddington 2021-06-23 23:19:22 PDT
Created attachment 432133 [details]
Patch
Comment 5 Tim Horton 2021-06-23 23:41:02 PDT
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 ]
Comment 6 Kimmo Kinnunen 2021-06-24 01:41:38 PDT
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 ?
Comment 7 Alexey Proskuryakov 2021-06-24 10:09:42 PDT
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.
Comment 8 Radar WebKit Bug Importer 2021-06-30 17:33:18 PDT
<rdar://problem/79994335>
Comment 9 Kyle Piddington 2021-07-14 18:54:54 PDT
Created attachment 433554 [details]
Patch
Comment 10 Kyle Piddington 2021-07-14 19:24:22 PDT
Created attachment 433557 [details]
Patch
Comment 11 Alexey Proskuryakov 2021-07-14 19:45:29 PDT
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.
Comment 12 Kyle Piddington 2021-07-15 11:47:00 PDT
(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.
Comment 13 Kyle Piddington 2021-07-15 12:05:20 PDT
Created attachment 433608 [details]
Patch
Comment 14 Alexey Proskuryakov 2021-07-15 12:13:09 PDT
> 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?
Comment 15 Kyle Piddington 2021-07-15 12:18:45 PDT
(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.
Comment 16 Kyle Piddington 2021-07-15 15:55:36 PDT
Created attachment 433631 [details]
Patch
Comment 17 Alexey Proskuryakov 2021-07-15 16:09:17 PDT
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.
Comment 18 Kenneth Russell 2021-07-15 16:31:46 PDT
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.
Comment 19 Kyle Piddington 2021-07-15 21:45:49 PDT
(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
Comment 20 Kyle Piddington 2021-07-15 21:45:54 PDT
(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
Comment 21 Kyle Piddington 2021-07-15 21:50:00 PDT
Created attachment 433654 [details]
Patch
Comment 22 Kyle Piddington 2021-07-15 23:49:35 PDT
Created attachment 433660 [details]
Patch for landing
Comment 23 EWS 2021-07-16 00:19:12 PDT
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].
Comment 24 Truitt Savell 2021-07-16 09:13:58 PDT
Reverted r279980 for reason:

Broke Internal Builds

Committed r279986 (239729@main): <https://commits.webkit.org/239729@main>
Comment 25 Kyle Piddington 2021-07-16 12:45:05 PDT
Created attachment 433693 [details]
Patch
Comment 26 Dean Jackson 2021-07-16 13:54:16 PDT
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.
Comment 27 EWS 2021-07-16 16:59:43 PDT
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].