Bug 238776 - [PGO] We should be able to build WebKit to collect PGO profiles easily
Summary: [PGO] We should be able to build WebKit to collect PGO profiles easily
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Local Build
Hardware: All macOS 10.15
: P2 Major
Assignee: Justin Michaud
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-04-04 16:25 PDT by Justin Michaud
Modified: 2022-04-14 08:58 PDT (History)
22 users (show)

See Also:


Attachments
Patch (35.76 KB, patch)
2022-04-07 13:39 PDT, Justin Michaud
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (35.73 KB, patch)
2022-04-07 13:50 PDT, Justin Michaud
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (35.69 KB, patch)
2022-04-07 14:21 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (45.39 KB, patch)
2022-04-08 18:20 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (42.95 KB, patch)
2022-04-11 15:12 PDT, Justin Michaud
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (42.92 KB, patch)
2022-04-11 15:25 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (42.14 KB, patch)
2022-04-12 16:04 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (42.65 KB, patch)
2022-04-13 16:14 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Justin Michaud 2022-04-04 16:25:58 PDT
Let's make it possible for us to build WebKit with LLVM's pgo instrumentation with one flag.
Comment 1 Justin Michaud 2022-04-07 13:39:42 PDT
Created attachment 456963 [details]
Patch
Comment 2 Justin Michaud 2022-04-07 13:50:44 PDT
Created attachment 456965 [details]
Patch
Comment 3 Justin Michaud 2022-04-07 14:21:07 PDT
Created attachment 456967 [details]
Patch
Comment 4 Saam Barati 2022-04-07 16:24:20 PDT
Comment on attachment 456967 [details]
Patch

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

> Source/WTF/ChangeLog:9
> +        Then, follow the included directions to collect your raw PGO profiles!

where are the included directions?

> Source/WTF/wtf/GenerateProfiles.h:41
> +inline void registerProfileGenerationCallback(const char* name)

I think you want ALWAYS_INLINE here, since this code really needs to live in the dylib it's called from to get proper data?

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:579
> +    WTF::registerProfileGenerationCallback<WebPage>("WebKit");

nit: "WebContentProcess" to be consistent w/ your other calls?
Comment 5 Justin Michaud 2022-04-07 16:57:51 PDT
Comment on attachment 456967 [details]
Patch

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

>> Source/WTF/wtf/GenerateProfiles.h:41
>> +inline void registerProfileGenerationCallback(const char* name)
> 
> I think you want ALWAYS_INLINE here, since this code really needs to live in the dylib it's called from to get proper data?

Nice catch

>> Source/WebKit/WebProcess/WebPage/WebPage.cpp:579
>> +    WTF::registerProfileGenerationCallback<WebPage>("WebKit");
> 
> nit: "WebContentProcess" to be consistent w/ your other calls?

Hmm. *Process files contain symbols from multiple frameworks, but this should only be including symbols from WebKit.framework. Actually, I guess this would include symbols from every dylib we link against? I'll have to do some research on this one.
Comment 6 Elliott Williams 2022-04-07 17:45:18 PDT
Comment on attachment 456967 [details]
Patch

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

> Source/JavaScriptCore/Configurations/JavaScriptCore.xcconfig:56
> +GENERATE_PROFILE_FLAGS_ON = -fprofile-instr-generate -DENABLE_LLVM_PROFILE_GENERATION=1;
> +USE_PROFILE_FLAGS_ON_GENERATE_OFF = -fprofile-instr-use=$(BUILT_PRODUCTS_DIR)/DerivedSources/JavaScriptCore/JavaScriptCore.profdata;
> +USE_PROFILE_FLAGS_ON_GENERATE_ = $(USE_PROFILE_FLAGS_ON_GENERATE_OFF)
> +
> +GENERATE_PROFILE_FLAGS = $(GENERATE_PROFILE_FLAGS_$(ENABLE_LLVM_PROFILE_GENERATION));
> +USE_PROFILE_FLAGS_ON = $(USE_PROFILE_FLAGS_ON_GENERATE_$(ENABLE_LLVM_PROFILE_GENERATION));
> +
>  PROFILE_DATA_FLAGS = $(PROFILE_DATA_FLAGS_$(CONFIGURATION)_$(WK_PLATFORM_NAME));
>  PROFILE_DATA_FLAGS_Release_macosx = $(PROFILE_DATA_FLAGS_Release_macosx$(WK_MACOS_1200));
> -PROFILE_DATA_FLAGS_Release_macosx_MACOS_SINCE_1200 = $(PROFILE_DATA_FLAGS_ENABLED);
> -PROFILE_DATA_FLAGS_Release_iphoneos = $(PROFILE_DATA_FLAGS_ENABLED);
> +PROFILE_DATA_FLAGS_Release_macosx_MACOS_SINCE_1200 = $(USE_PROFILE_FLAGS_ON);
> +PROFILE_DATA_FLAGS_Release_iphoneos = $(USE_PROFILE_FLAGS_ON);
>  PROFILE_DATA_FLAGS_Production_macosx = $(PROFILE_DATA_FLAGS_Production_macosx$(WK_MACOS_1200));
> -PROFILE_DATA_FLAGS_Production_macosx_MACOS_SINCE_1200 = $(PROFILE_DATA_FLAGS_ENABLED);
> -PROFILE_DATA_FLAGS_Production_iphoneos = $(PROFILE_DATA_FLAGS_ENABLED);
> -PROFILE_DATA_FLAGS_ENABLED = -fprofile-instr-use=$(BUILT_PRODUCTS_DIR)/DerivedSources/JavaScriptCore/JavaScriptCore.profdata;
> +PROFILE_DATA_FLAGS_Production_macosx_MACOS_SINCE_1200 = $(USE_PROFILE_FLAGS_ON);
> +PROFILE_DATA_FLAGS_Production_iphoneos = $(USE_PROFILE_FLAGS_ON);

I think this can be made simpler. How about:

    PROFILE_DATA_FLAGS_GENERATE_YES = -fprofile-instr-generate -DENABLE_LLVM_PROFILE_GENERATION=1;
    PROFILE_DATA_FLAGS_GENERATE_ = -fprofile-instr-use=$(BUILT_PRODUCTS_DIR)/DerivedSources/JavaScriptCore/JavaScriptCore.profdata;

Then, change the expansions from `$(PROFILE_DATA_FLAGS_ENABLED)` to `$($(PROFILE_DATA_FLAGS_GENERATE_$(ENABLE_LLVM_PROFILE_GENERATION))`.

> Tools/Scripts/webkitdirs.pm:1060
> +    appendToEnvironmentVariableList("ENABLE_LLVM_PROFILE_GENERATION", "ON") if defined $ENV{'ENABLE_LLVM_PROFILE_GENERATION'};

Are you sure this is necessary? Make is passing ENABLE_LLVM_PROFILE_GENERATION=YES to xcodebuild, so anything executed by Xcode will have the environment variable already set. Unless I'm missing something, this appears to be setting ENABLE_LLVM_PROFILE_GENERATION=ON when ENABLE_LLVM_PROFILE_GENERATION is already set.

> Tools/Scripts/webkitperl/FeatureList.pm:361
> +      { option => "llvm-profile-generation", desc => "Include LLVM's instrumentation to generate profiles for PGO",
> +      define => "ENABLE_LLVM_PROFILE_GENERATION", value => \$llvmProfileGenerationSupport },
> +

nit: indentation

> Makefile.shared:5
> +ifneq (,$(ENABLE_LLVM_PROFILE_GENERATION))
> +		export ENABLE_LLVM_PROFILE_GENERATION=ON
> +endif

If we can remove the line in webkitdirs.pm, then we can remove this too, and only pass the setting in XCODE_OPTIONS.

> Makefile.shared:9
> +ifneq (,$(ENABLE_LLVM_PROFILE_GENERATION))
> +		XCODE_OPTIONS += ENABLE_LLVM_PROFILE_GENERATION=ON
> +endif

nit: Using ifneq here means I could type `make ENABLE_LLVM_PROFILE_GENERATION=NO` and it would do the opposite and pass ENABLE_LLVM_PROFILE_GENERATION=ON. Can this be `ifeq ($(ENABLE_LLVM_PROFILE_GENERATION),YES)`?
Comment 7 Justin Michaud 2022-04-08 13:45:37 PDT
Comment on attachment 456967 [details]
Patch

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

>> Source/JavaScriptCore/Configurations/JavaScriptCore.xcconfig:56
>> +PROFILE_DATA_FLAGS_Production_iphoneos = $(USE_PROFILE_FLAGS_ON);
> 
> I think this can be made simpler. How about:
> 
>     PROFILE_DATA_FLAGS_GENERATE_YES = -fprofile-instr-generate -DENABLE_LLVM_PROFILE_GENERATION=1;
>     PROFILE_DATA_FLAGS_GENERATE_ = -fprofile-instr-use=$(BUILT_PRODUCTS_DIR)/DerivedSources/JavaScriptCore/JavaScriptCore.profdata;
> 
> Then, change the expansions from `$(PROFILE_DATA_FLAGS_ENABLED)` to `$($(PROFILE_DATA_FLAGS_GENERATE_$(ENABLE_LLVM_PROFILE_GENERATION))`.

LDFlags are only used for generation, so there will always be a certain amount of duplication here. I have tried to make it simpler.

>> Tools/Scripts/webkitdirs.pm:1060
>> +    appendToEnvironmentVariableList("ENABLE_LLVM_PROFILE_GENERATION", "ON") if defined $ENV{'ENABLE_LLVM_PROFILE_GENERATION'};
> 
> Are you sure this is necessary? Make is passing ENABLE_LLVM_PROFILE_GENERATION=YES to xcodebuild, so anything executed by Xcode will have the environment variable already set. Unless I'm missing something, this appears to be setting ENABLE_LLVM_PROFILE_GENERATION=ON when ENABLE_LLVM_PROFILE_GENERATION is already set.

This is needed for check-for-weak-vtables-and-externals
Comment 8 Justin Michaud 2022-04-08 18:20:39 PDT
Created attachment 457133 [details]
Patch
Comment 9 Saam Barati 2022-04-11 09:37:31 PDT
Comment on attachment 457133 [details]
Patch

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

> Source/JavaScriptCore/Configurations/JavaScriptCore.xcconfig:43
> +PROFILE_DATA_FLAGS_ENABLED = -fprofile-instr-use=$(BUILT_PRODUCTS_DIR)/DerivedSources/JavaScriptCore/JavaScriptCore.profdata;

why move this?

> Source/JavaScriptCore/Configurations/JavaScriptCore.xcconfig:55
> +PROFILE_GENERATE_OR_USE_CFLAGS_GENERATE_ = $(PROFILE_GENERATE_OR_USE_CFLAGS_GENERATE_)

Does this work? Assigning to yourself like this? Did you mean to have $(PROFILE_GENERATE_OR_USE_CFLAGS_GENERATE_OFF) on the RHS?

> Source/WebCore/Configurations/WebCoreTestSupport.xcconfig:66
> +OTHER_CFLAGS = $(inherited) // Workaround for rdar://91365594 (Clang hangs when building WebKit WebCoreTestSupport with PGO profile generation enabled)

Don't understand why we need to explicitly do this? Is it just a form of documentation?

> Source/WebCore/page/Frame.cpp:175
> +    WTF::registerProfileGenerationCallback<Frame>("WebCore");

Maybe ScriptController::initializeMainThread is the better place for this?

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:579
> +    WTF::registerProfileGenerationCallback<WebPage>("WebKit");

Put this in InitializeWebKit2?
Comment 10 Elliott Williams 2022-04-11 10:49:56 PDT
Comment on attachment 457133 [details]
Patch

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

> Source/JavaScriptCore/Configurations/JavaScriptCore.xcconfig:61
> +PROFILE_GENERATE_OR_USE_LDFLAGS_GENERATE_OFF = ;
> +PROFILE_GENERATE_OR_USE_LDFLAGS_GENERATE_ON = -fprofile-instr-generate -DENABLE_LLVM_PROFILE_GENERATION=1;
> +PROFILE_GENERATE_OR_USE_LDFLAGS_GENERATE_ = $(PROFILE_GENERATE_OR_USE_LDFLAGS_GENERATE_OFF)
> +PROFILE_GENERATE_OR_USE_LDFLAGS = $(PROFILE_GENERATE_OR_USE_LDFLAGS_GENERATE_$(ENABLE_LLVM_PROFILE_GENERATION));

No need to define empty build settings. You can just do:

PROFILE_GENERATE_OR_USE_LDFLAGS_GENERATE_ON = -fprofile-instr-generate -DENABLE_LLVM_PROFILE_GENERATION=1;
PROFILE_GENERATE_OR_USE_LDFLAGS = $(PROFILE_GENERATE_OR_USE_LDFLAGS_GENERATE_$(ENABLE_LLVM_PROFILE_GENERATION));

> Source/WebKit/Configurations/BaseXPCService.xcconfig:44
> +OTHER_CFLAGS = $(OTHER_CFLAGS_BASETARGET);

It might be easier to follow this if you keep all the OTHER_CFLAGS settings in BaseTarget.xcconfig, and set

    PROFILE_GENERATE_OR_USE_CFLAGS = ;

in any target which we intend to disable PGO in. Then you don't have this back-and-forth where OTHER_CFLAGS is getting evaluated using multiple files.

> Tools/Scripts/webkitdirs.pm:1060
> +    appendToEnvironmentVariableList("ENABLE_LLVM_PROFILE_GENERATION", $ENV{'ENABLE_LLVM_PROFILE_GENERATION'}) if defined $ENV{'ENABLE_LLVM_PROFILE_GENERATION'};

Still confused about this. Xcode exports the target's build settings to any script phases, and AFAICT check-for-weak-vtables-and-externals is only run as a script phase. So we shouldn't need to manually define it here.

The reason why the existing WEBKIT_COVERAGE_BUILD variable is set here is because it's _not_ set in xcconfigs; it's derived from the active coverage settings.

> Tools/Scripts/webkitperl/FeatureList.pm:361
> +    { option => "llvm-profile-generation", desc => "Include LLVM's instrumentation to generate profiles for PGO",
> +      define => "ENABLE_LLVM_PROFILE_GENERATION", value => \$llvmProfileGenerationSupport },
> +

Here's something annoying: Feature flags appear in Xcode build settings as defined as their own name, _not_ as YES or NO. i.e. if I run `build-webkit --llvm-profile-generation`, this build setting is passed to Xcode:

    ENABLE_LLVM_PROFILE_GENERATION=ENABLE_LLVM_PROFILE_GENERATION

So you need to handle that case in the xcconfig expansions. Perhaps we should have Make match this behavior and transform YES into ENABLE_LLVM_PROFILE_GENERATION. Then, in xcconfigs, do something like:

    PROFILE_GENERATE_OR_USE_CFLAGS_ENABLE_LLVM_PROFILE_GENERATION = -fprofile-instr-generate -DENABLE_LLVM_PROFILE_GENERATION=1;
    PROFILE_GENERATE_OR_USE_CFLAGS_ = $(PROFILE_DATA_FLAGS);
    PROFILE_GENERATE_OR_USE_CFLAGS = $(PROFILE_GENERATE_OR_USE_CFLAGS_$(ENABLE_LLVM_PROFILE_GENERATION));
Comment 11 Justin Michaud 2022-04-11 15:09:40 PDT
(In reply to Saam Barati from comment #9)
> Comment on attachment 457133 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=457133&action=review
> 
> > Source/JavaScriptCore/Configurations/JavaScriptCore.xcconfig:43
> > +PROFILE_DATA_FLAGS_ENABLED = -fprofile-instr-use=$(BUILT_PRODUCTS_DIR)/DerivedSources/JavaScriptCore/JavaScriptCore.profdata;
> 
> why move this?
This makes the copy pasta block after easier to copy & paste.

> 
> > Source/JavaScriptCore/Configurations/JavaScriptCore.xcconfig:55
> > +PROFILE_GENERATE_OR_USE_CFLAGS_GENERATE_ = $(PROFILE_GENERATE_OR_USE_CFLAGS_GENERATE_)
> 
> Does this work? Assigning to yourself like this? Did you mean to have
> $(PROFILE_GENERATE_OR_USE_CFLAGS_GENERATE_OFF) on the RHS?

Copy pasta error

> 
> > Source/WebCore/Configurations/WebCoreTestSupport.xcconfig:66
> > +OTHER_CFLAGS = $(inherited) // Workaround for rdar://91365594 (Clang hangs when building WebKit WebCoreTestSupport with PGO profile generation enabled)
> 
> Don't understand why we need to explicitly do this? Is it just a form of
> documentation?
We need to overwrite the included definition, which enables PGO. Elliott's solution was better.
Comment 12 Justin Michaud 2022-04-11 15:12:51 PDT
Created attachment 457290 [details]
Patch
Comment 13 Justin Michaud 2022-04-11 15:25:10 PDT
Created attachment 457292 [details]
Patch
Comment 14 Elliott Williams 2022-04-11 16:14:17 PDT
Comment on attachment 457292 [details]
Patch

Looks great! r=me
Comment 15 Radar WebKit Bug Importer 2022-04-11 16:26:15 PDT
<rdar://problem/91595425>
Comment 16 Saam Barati 2022-04-11 23:10:02 PDT
Comment on attachment 457292 [details]
Patch

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

> Source/JavaScriptCore/Configurations/JavaScriptCore.xcconfig:57
> +PROFILE_GENERATE_OR_USE_LDFLAGS = $(PROFILE_GENERATE_OR_USE_LDFLAGS_$(ENABLE_LLVM_PROFILE_GENERATION));

So the idea is we're run with the variable "ENABLE_LLVM_PROFILE_GENERATION" equal to the string "ENABLE_LLVM_PROFILE_GENERATION"? Why not just make it "YES"/"NO", like over strings?

> Source/WTF/wtf/GenerateProfiles.h:40
> +template<typename OnceFlagDiversifier>

I don't understand this template parameter.

> Source/WebCore/bindings/js/ScriptController.cpp:90
> +    WTF::registerProfileGenerationCallback<Frame>("WebCore");

I'd put this at the end, not the beginning, so we don't need to worry about what we can use in this function.

Also, "Frame"?
Comment 17 Saam Barati 2022-04-11 23:14:29 PDT
Comment on attachment 457133 [details]
Patch

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

>> Tools/Scripts/webkitperl/FeatureList.pm:361
>> +
> 
> Here's something annoying: Feature flags appear in Xcode build settings as defined as their own name, _not_ as YES or NO. i.e. if I run `build-webkit --llvm-profile-generation`, this build setting is passed to Xcode:
> 
>     ENABLE_LLVM_PROFILE_GENERATION=ENABLE_LLVM_PROFILE_GENERATION
> 
> So you need to handle that case in the xcconfig expansions. Perhaps we should have Make match this behavior and transform YES into ENABLE_LLVM_PROFILE_GENERATION. Then, in xcconfigs, do something like:
> 
>     PROFILE_GENERATE_OR_USE_CFLAGS_ENABLE_LLVM_PROFILE_GENERATION = -fprofile-instr-generate -DENABLE_LLVM_PROFILE_GENERATION=1;
>     PROFILE_GENERATE_OR_USE_CFLAGS_ = $(PROFILE_DATA_FLAGS);
>     PROFILE_GENERATE_OR_USE_CFLAGS = $(PROFILE_GENERATE_OR_USE_CFLAGS_$(ENABLE_LLVM_PROFILE_GENERATION));

Can we just transform ENABLE_LLVM_PROFILE_GENERATION=ENABLE_LLVM_PROFILE_GENERATION into ENABLE_LLVM_PROFILE_GENERATION=ON?
Reasoning being:
PROFILE_GENERATE_OR_USE_CFLAGS_ON is much easier to read than PROFILE_GENERATE_OR_USE_CFLAGS_ENABLE_LLVM_PROFILE_GENERATION
Comment 18 Saam Barati 2022-04-11 23:20:26 PDT
Comment on attachment 457133 [details]
Patch

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

>>> Tools/Scripts/webkitperl/FeatureList.pm:361
>>> +
>> 
>> Here's something annoying: Feature flags appear in Xcode build settings as defined as their own name, _not_ as YES or NO. i.e. if I run `build-webkit --llvm-profile-generation`, this build setting is passed to Xcode:
>> 
>>     ENABLE_LLVM_PROFILE_GENERATION=ENABLE_LLVM_PROFILE_GENERATION
>> 
>> So you need to handle that case in the xcconfig expansions. Perhaps we should have Make match this behavior and transform YES into ENABLE_LLVM_PROFILE_GENERATION. Then, in xcconfigs, do something like:
>> 
>>     PROFILE_GENERATE_OR_USE_CFLAGS_ENABLE_LLVM_PROFILE_GENERATION = -fprofile-instr-generate -DENABLE_LLVM_PROFILE_GENERATION=1;
>>     PROFILE_GENERATE_OR_USE_CFLAGS_ = $(PROFILE_DATA_FLAGS);
>>     PROFILE_GENERATE_OR_USE_CFLAGS = $(PROFILE_GENERATE_OR_USE_CFLAGS_$(ENABLE_LLVM_PROFILE_GENERATION));
> 
> Can we just transform ENABLE_LLVM_PROFILE_GENERATION=ENABLE_LLVM_PROFILE_GENERATION into ENABLE_LLVM_PROFILE_GENERATION=ON?
> Reasoning being:
> PROFILE_GENERATE_OR_USE_CFLAGS_ON is much easier to read than PROFILE_GENERATE_OR_USE_CFLAGS_ENABLE_LLVM_PROFILE_GENERATION

Actually, I take this back, this is actually easier to read, just a bit verbose.

But I think the reason it's a bit weird to read is that the variable name contains an OR in it. Maybe we could define one rule by running NOT_ on another. Probably there is some way to define the rules that don't rely on the OR semantics in the variable, but maybe it's not worth bike shedding over.
Comment 19 Saam Barati 2022-04-11 23:26:08 PDT
Comment on attachment 457292 [details]
Patch

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

> Source/JavaScriptCore/runtime/VM.cpp:245
> +    WTF::registerProfileGenerationCallback<VM>("JavaScriptCore");

I'd put this in initializeThreading

> Source/WebKit/NetworkProcess/mac/NetworkProcessMac.mm:55
> +    WTF::registerProfileGenerationCallback<NetworkProcess>("NetworkProcess");

Why are these needed if we're in InitializeWebKit2?
Comment 20 Justin Michaud 2022-04-12 07:58:43 PDT
(In reply to Saam Barati from comment #16)
> Comment on attachment 457292 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=457292&action=review
> 
> > Source/JavaScriptCore/Configurations/JavaScriptCore.xcconfig:57
> > +PROFILE_GENERATE_OR_USE_LDFLAGS = $(PROFILE_GENERATE_OR_USE_LDFLAGS_$(ENABLE_LLVM_PROFILE_GENERATION));
> 
> So the idea is we're run with the variable "ENABLE_LLVM_PROFILE_GENERATION"
> equal to the string "ENABLE_LLVM_PROFILE_GENERATION"? Why not just make it
> "YES"/"NO", like over strings?

Elliott mentioned this above. We don't get a choice here, Xcode picked this convention. The command line still uses ON.

> 
> > Source/WTF/wtf/GenerateProfiles.h:40
> > +template<typename OnceFlagDiversifier>
> 
> I don't understand this template parameter.
> 
> > Source/WebCore/bindings/js/ScriptController.cpp:90
> > +    WTF::registerProfileGenerationCallback<Frame>("WebCore");
> 
> I'd put this at the end, not the beginning, so we don't need to worry about
> what we can use in this function.
> 
> Also, "Frame"?

This is spectacularly wrong, it is in the Windows port. The cocoa version is obj c, so I am searching for a different place to put this.

> > Source/WebKit/NetworkProcess/mac/NetworkProcessMac.mm:55
> > +    WTF::registerProfileGenerationCallback<NetworkProcess>("NetworkProcess");
> Why are these needed if we're in InitializeWebKit2?

See ^

------

Finding a place for the WebKit profiling call is spectacularly important, and I will try to move the JSC one too. After that I think it should be ready to land for real 100% no whammies.
Comment 21 Justin Michaud 2022-04-12 16:04:56 PDT
Created attachment 457488 [details]
Patch
Comment 22 Wenson Hsieh 2022-04-13 14:44:46 PDT
Comment on attachment 457488 [details]
Patch

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

> Source/WebCore/bindings/js/ScriptController.cpp:95
> +    WTF::registerProfileGenerationCallback<Frame>("WebCore");

Is there a reason why we use existing classes here (and in JSC), but use a named enum type in WebKit2?
Comment 23 Saam Barati 2022-04-13 15:06:13 PDT
Comment on attachment 457488 [details]
Patch

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

> Source/WTF/wtf/GenerateProfiles.h:43
> +template<typename OnceFlagDiversifier>

Why do we need this template parameter?
Comment 24 Justin Michaud 2022-04-13 15:21:18 PDT
(In reply to Wenson Hsieh from comment #22)
> Comment on attachment 457488 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=457488&action=review
> 
> > Source/WebCore/bindings/js/ScriptController.cpp:95
> > +    WTF::registerProfileGenerationCallback<Frame>("WebCore");
> 
> Is there a reason why we use existing classes here (and in JSC), but use a
> named enum type in WebKit2?

(In reply to Saam Barati from comment #23)
> Comment on attachment 457488 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=457488&action=review
> 
> > Source/WTF/wtf/GenerateProfiles.h:43
> > +template<typename OnceFlagDiversifier>
> 
> Why do we need this template parameter?

We just need some unique name to make the static once_flag different for each dylib. Anything will work.
Comment 25 Justin Michaud 2022-04-13 16:14:28 PDT
Created attachment 457571 [details]
Patch
Comment 26 EWS 2022-04-14 08:57:55 PDT
Committed r292870 (249641@main): <https://commits.webkit.org/249641@main>

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