A/B test concurrent GC

[JF Bastien]

Let's A/B test the concurrent GC and see if it affects crash rates.

[JF Bastien]

Created attachment 301800 [details]
patch

[JF Bastien]

<rdar://problem/30553220>

[WebKit Commit Bot]

Attachment 301800 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/Options.cpp:47:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Comment on attachment 301800 [details]
patch

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

LGTM, but I want ap's opinion too.

> Source/JavaScriptCore/runtime/Options.cpp:393
> +        const char* userUUID = getUserUUID();
> +        for (const char* c = userUUID; *c; ++c)
> +            accumulator += *c;
> +        accumulator = (accumulator & 0xFFFFu) ^ (accumulator >> 16);
> +        accumulator = (accumulator & 0xFF) ^ (accumulator >> 8);
> +        accumulator = (accumulator & 0xF) ^ (accumulator >> 4);
> +        accumulator = (accumulator & 0x3) ^ (accumulator >> 2);
> +        accumulator = (accumulator & 0x1) ^ (accumulator >> 1);

Could we use some existing WTF hash function?

[JF Bastien]

One downside is it'll affect even JSC, so I added a flag to opt out of the A/B test.

[Keith Miller]

Comment on attachment 301800 [details]
patch

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

> Source/JavaScriptCore/ChangeLog:9
> +        tell. A/B test this by setting it of for 50% of users, based on

typo?: off

[JF Bastien]

Created attachment 301801 [details]
patch

Fix typo.

[WebKit Commit Bot]

Attachment 301801 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/Options.cpp:47:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[JF Bastien]

(In reply to comment #4)
> Comment on attachment 301800 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=301800&action=review
> 
> LGTM, but I want ap's opinion too.
> 
> > Source/JavaScriptCore/runtime/Options.cpp:393
> > +        const char* userUUID = getUserUUID();
> > +        for (const char* c = userUUID; *c; ++c)
> > +            accumulator += *c;
> > +        accumulator = (accumulator & 0xFFFFu) ^ (accumulator >> 16);
> > +        accumulator = (accumulator & 0xFF) ^ (accumulator >> 8);
> > +        accumulator = (accumulator & 0xF) ^ (accumulator >> 4);
> > +        accumulator = (accumulator & 0x3) ^ (accumulator >> 2);
> > +        accumulator = (accumulator & 0x1) ^ (accumulator >> 1);
> 
> Could we use some existing WTF hash function?

My thinking was that reconstructing the on / off decision didn't need a fancy hash. This just sums the ASCII values, and then xors all the bits from the sum.

[Alexey Proskuryakov]

Comment on attachment 301800 [details]
patch

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

> Source/JavaScriptCore/runtime/Options.cpp:54
> +    static constexpr const size_t maxBytes = 42;

This is a surprising number - as you subtract one, this becomes 41 character, crashes on my machines have UUIDs with 36 characters.

> Source/JavaScriptCore/runtime/Options.cpp:55
> +    static char userUUID[maxBytes] = { '\0' };

I thought that "{ }" was best in C++. Most of it was about poor optimization in MSVC, but at least it's shorter.

> Source/JavaScriptCore/runtime/Options.cpp:59
> +        softLinkCRGetUserUUID = (CFStringRef (*)(void)) dlsym(CrashReporterSupportLibrary(), "CRGetUserUUID");

What was the reason to not use SOFT_LINK?

[Alexey Proskuryakov]

Comment on attachment 301801 [details]
patch

Carrying over the r+, my and Filip's comments apply to this version too.

[JF Bastien]

Created attachment 301803 [details]
patch

Updated to use 37 for buffer size.

(In reply to comment #10)
> Comment on attachment 301800 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=301800&action=review
> 
> > Source/JavaScriptCore/runtime/Options.cpp:54
> > +    static constexpr const size_t maxBytes = 42;
> 
> This is a surprising number - as you subtract one, this becomes 41
> character, crashes on my machines have UUIDs with 36 characters.

Done.

> > Source/JavaScriptCore/runtime/Options.cpp:55
> > +    static char userUUID[maxBytes] = { '\0' };
> 
> I thought that "{ }" was best in C++. Most of it was about poor optimization
> in MSVC, but at least it's shorter.

It works with both C and C++ to have just one element to fill the array with that value. I'm guessing we'll move this code around later, some of it looked like extern C, so that seemed relevant. I also like being explicit about this, even in pure C++.

> > Source/JavaScriptCore/runtime/Options.cpp:59
> > +        softLinkCRGetUserUUID = (CFStringRef (*)(void)) dlsym(CrashReporterSupportLibrary(), "CRGetUserUUID");
> 
> What was the reason to not use SOFT_LINK?

AFAICT SOFT_LINK requires a namespace, which CrashReporter doesn't have.

[WebKit Commit Bot]

Comment on attachment 301803 [details]
patch

Rejecting attachment 301803 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 301803, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in Source/JavaScriptCore/ChangeLog contains OOPS!.

Full output: http://webkit-queues.webkit.org/results/3134637

[Alexey Proskuryakov]

> AFAICT SOFT_LINK requires a namespace, which CrashReporter doesn't have.

We use SOFT_LINK all over the place, so I'd be surprised is CrashReporter didn't work with it. 

A couple late comments:
- please confirm that the function returns the same UUID that we get in crash logs;
- please confirm that it works in all clients that use JavaScriptCore (e.g. no sandboxing limitations).

[JF Bastien]

Created attachment 301825 [details]
patch

Update with SOFT_LINK.

Leaving as cq- as we may not want to have this on for everything (e.g. jsc is weird, and affects our testing for half the bots). Waiting on input to figure out if I should restrict it more.

(In reply to comment #14)
> > AFAICT SOFT_LINK requires a namespace, which CrashReporter doesn't have.
> 
> We use SOFT_LINK all over the place, so I'd be surprised is CrashReporter
> didn't work with it. 

Ah you're correct, I was looking at the wrong version of SOFT_LINK which took a namespace (whereas bmalloc uses CrashReporter but has its own BSOFT_LINK_FUNCTION).

> A couple late comments:
> - please confirm that the function returns the same UUID that we get in
> crash logs;

At least when I run jsc, it generates the same anonymous UUID in  ~/Library/Logs/DiagnosticReports/ as the one the A/B test prints on the screen.

> - please confirm that it works in all clients that use JavaScriptCore (e.g.
> no sandboxing limitations).

The Safari I built launches just fine, so does jsc. I tested regular make build as well as cmake's jsc (with internal SDK in both cases). Which else do you think I should try out?

[WebKit Commit Bot]

Attachment 301825 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/Options.cpp:47:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexey Proskuryakov]

> The Safari I built launches just fine, so does jsc. I tested regular make build as well as cmake's jsc (with internal SDK in both cases). Which else do you think I should try out?

JavaScriptCore is used in a lot of places.

I thin that doing these two things would be sufficient, sorry that they are not super straightforward:

1. Read the implementation of CRGetUserUUID to see what it's doing.
2. Build a tool that uses JSC and have a super minimal sandbox; confirm that it works.

Also, a way to mitigate the risks is to fail softly instead of crashing when CRGetUserUUID returns null or empty string.

[Filip Pizlo]

(In reply to comment #17)
> > The Safari I built launches just fine, so does jsc. I tested regular make build as well as cmake's jsc (with internal SDK in both cases). Which else do you think I should try out?
> 
> JavaScriptCore is used in a lot of places.
> 
> I thin that doing these two things would be sufficient, sorry that they are
> not super straightforward:
> 
> 1. Read the implementation of CRGetUserUUID to see what it's doing.
> 2. Build a tool that uses JSC and have a super minimal sandbox; confirm that
> it works.
> 
> Also, a way to mitigate the risks is to fail softly instead of crashing when
> CRGetUserUUID returns null or empty string.

I agree that we need to verify this case.

[JF Bastien]

Created attachment 301842 [details]
patch

Here's a version that will soft-fail. I'm not a fan because we won't know if the A/B test is on, but it's as stable as before. Lack of crash reduction won't mean the test is conclusion though.

[David Kilzer (:ddkilzer)]

Comment on attachment 301842 [details]
patch

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

> Source/JavaScriptCore/runtime/Options.cpp:52
> +#include <WebCore/SoftLinking.h>
> +SOFT_LINK_PRIVATE_FRAMEWORK(CrashReporterSupport);
> +SOFT_LINK(CrashReporterSupport, CRGetUserUUID, CFStringRef, (void), ());

Technically it's a layering violation to include WebCore headers in JavaScriptCore.  :)

If we want to use SOFT_LINK macros in JavaScriptCore, they need to be moved from WebCore into WTF.

[JF Bastien]

(In reply to comment #20)
> Comment on attachment 301842 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=301842&action=review
> 
> > Source/JavaScriptCore/runtime/Options.cpp:52
> > +#include <WebCore/SoftLinking.h>
> > +SOFT_LINK_PRIVATE_FRAMEWORK(CrashReporterSupport);
> > +SOFT_LINK(CrashReporterSupport, CRGetUserUUID, CFStringRef, (void), ());
> 
> Technically it's a layering violation to include WebCore headers in
> JavaScriptCore.  :)
> 
> If we want to use SOFT_LINK macros in JavaScriptCore, they need to be moved
> from WebCore into WTF.

Yeah quick hack for fewer lines touched :)
I'll do it better in bug #168467, in WTF. In a few days though, I've got another bug pressing right now.

[JF Bastien]

Created attachment 302016 [details]
path

Here's an updated patch which should be less brittle.

I'm still building it and will test on MacOS as well as iOS, but I think the rough sketch is right. I had in-person chats with a few people to validate the general approach.

Comments welcome! I'll post back when it's tested.

[WebKit Commit Bot]

Attachment 302016 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/WebCoreSplitTestInitializer.cpp:31:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/platform/WebCoreSplitTestInitializer.cpp:34:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/platform/WebCoreSplitTestInitializer.cpp:37:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/platform/WebCoreSplitTestInitializer.cpp:76:  Use equivelent function in <wtf/ASCIICType.h> instead of the tolower() function.  [runtime/ctype_function] [4]
ERROR: Source/WTF/wtf/SplitTest.h:26:  Use #pragma once instead of #ifndef for header guard.  [build/header_guard] [5]
ERROR: Source/WTF/wtf/SplitTest.h:30:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 6 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[JF Bastien]

Created attachment 302019 [details]
patch

Fix webkit style.

[WebKit Commit Bot]

Attachment 302019 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/WebCoreSplitTestInitializer.cpp:33:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Comment on attachment 302019 [details]
patch

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

> Source/WebCore/platform/WebCoreSplitTestInitializer.cpp:43
> +struct UUID {

Can this be in an anonymous namespace?

> Source/WebCore/platform/WebCoreSplitTestInitializer.cpp:48
> +std::optional<UUID> getUUID()

Can this be in that same anonymous namespace, or static?

[JF Bastien]

Created attachment 302025 [details]
patch

Move to anonymous namespace as suggested by Fil.

[Filip Pizlo]

Comment on attachment 302025 [details]
patch

LGTM.  Would be good to get ap's or cdumez's feedback too.

[WebKit Commit Bot]

Attachment 302025 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/WebCoreSplitTestInitializer.cpp:33:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[JF Bastien]

Created attachment 302027 [details]
patch

Fix typo. Fil r+'s, but I'll wait for input for others, and I still need to run tests.

[JF Bastien]

Created attachment 302028 [details]
patch

Missing WTF_EXPORT made the build sad.

[Filip Pizlo]

Comment on attachment 302028 [details]
patch

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

> Source/JavaScriptCore/runtime/Options.cpp:367
> +    if (Options::enableSplitTesting() && Options::useConcurrentGC()) {
> +        // Run an A/B split test on concurrent GC: if it was going to be on,
> +        // turn it off with some probability. Do this deterministically per
> +        // unique user identifier so that crashes don't skew statistics, and do
> +        // it in a manner which can be reconstructed from a crash trace.
> +        uint64_t concurrentGCExperimentUniqueIdentifier = 0xCAFEC0FECAFEC0FEull;
> +        std::optional<bool> enableExperiment = SplitTest::singleton().enableBooleanExperiment(concurrentGCExperimentUniqueIdentifier);
> +        Options::useConcurrentGC() = enableExperiment.value_or(true);
> +    }

Hmmm.  I think you want this in overrideDefaults.

Putting this here means that nobody can override this when using jsc on the command line.  It means, for example, that --useConcurrentGC=false won't work at all.  I don't think we want that.

Putting this in overrideDefaults means that if the user sets the environment variable or uses the cmdline option, they will override the default, just as it would work in trunk now.

[JF Bastien]

Created attachment 302049 [details]
patch

Make the change suggested by Fil.

I tested it out locally on MacOS and it seems fine. I added WTFLogAlways to check that initialization is going on as I expect (it does, gets the UUID fine, changes options). My iOS build was having the sads so I blew it away and am rebuilding it. Will comment when I've tried it, not today.

[WebKit Commit Bot]

Attachment 302049 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/WebCoreSplitTestInitializer.cpp:34:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexey Proskuryakov]

Comment on attachment 302049 [details]
patch

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

I have some pedantic nitpicks.

> Source/JavaScriptCore/runtime/Options.cpp:328
> +    if (Options::enableSplitTesting()) {

Seems strange to have a JSC option for this given that split testing is a WebCore concept now.

> Source/JavaScriptCore/runtime/Options.cpp:334
> +            uint64_t concurrentGCExperimentUniqueIdentifier = 0xAC0DE24B1D6CB000ull;

How did you come up with this number? Can it be 0 for this first A/B test of ours?

My understanding is that we would only need another number when running more than one test at at the same time.

> Source/WTF/wtf/SplitTest.h:42
> +// It's important to "salt" with unique experiment IDs so that users don't all
> +// fall in the same set of experiments.

I'd mention "when running multiple experiments at the same time". That wold probably be highly unusual for us, so this comment can be confusing.

> Source/WTF/wtf/SplitTest.h:60
> +    // FIXME: add a version which returns a number in a provided [min, max]
> +    //        range, which is more useful for bucketed experiments.

A couple minor style violations here:
1. Please start the sentence with an upper case letter.
2. Please don't align text (i.e. "range" should start at the same column as "FIXME").

> Source/WebCore/platform/WebCoreSplitTestInitializer.cpp:31
> +#include <CoreFoundation/CFString.h>

This is unnecessary, CoreFoundation.h is included everywhere via prefix file. Please remove the include.

> Source/WebCore/platform/WebCoreSplitTestInitializer.cpp:33
> +#include <inttypes.h>

What is used from this header?

> Source/WebCore/platform/WebCoreSplitTestInitializer.cpp:35
> +#include <SystemConfiguration/SystemConfiguration.h>

And this one?

> Source/WebCore/platform/WebCoreSplitTestInitializer.cpp:86
> +                continue; // Don't get an extra nibble.

What is an "extra nibble"? Is this check about skipping dashes?

> Source/WebCore/platform/WebCoreSplitTestInitializer.cpp:107
> +void InitWebCoreSplitTest(void)

This name looks strange. Why does it start with an upper case letter, and an abbreviated word? I suggest initializeWebCoreSplitTest.

Also, I don't think that "(void)" is warranted here. The header file might be included from a pure C file, but matching the syntax here shouldn't be necessary.

[JF Bastien]

Created attachment 302064 [details]
patch

Address ap's comments.


(In reply to comment #35)
> Comment on attachment 302049 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=302049&action=review
> 
> I have some pedantic nitpicks.
> 
> > Source/JavaScriptCore/runtime/Options.cpp:328
> > +    if (Options::enableSplitTesting()) {
> 
> Seems strange to have a JSC option for this given that split testing is a
> WebCore concept now.

Yeah good point. I'm renaming it to a JSC option for just this test, and later I'll add a WebCore / WTF option for split testing in general.


> > Source/JavaScriptCore/runtime/Options.cpp:334
> > +            uint64_t concurrentGCExperimentUniqueIdentifier = 0xAC0DE24B1D6CB000ull;
> 
> How did you come up with this number? Can it be 0 for this first A/B test of
> ours?

Fancy 1337 speak. I'll remove this entirely to make the patch simple, and will follow-up in #168467.

> My understanding is that we would only need another number when running more
> than one test at at the same time.

That is correct.


> > Source/WebCore/platform/WebCoreSplitTestInitializer.cpp:107
> > +void InitWebCoreSplitTest(void)
> 
> This name looks strange. Why does it start with an upper case letter, and an
> abbreviated word? I suggest initializeWebCoreSplitTest.
> 
> Also, I don't think that "(void)" is warranted here. The header file might
> be included from a pure C file, but matching the syntax here shouldn't be
> necessary.

Hmm, I cargo-culled from a random header (WebCoreThreadSystemInterface) and hit the one that's capitalized that way. It's also silly with (void) so I thought that was Style. I fixed both.

`init` is more common that `initialize` in WebCore, so I kept the abbreviation.

[WebKit Commit Bot]

Attachment 302064 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/WebCoreSplitTestInitializer.cpp:32:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ryosuke Niwa]

Comment on attachment 302064 [details]
patch

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

> Source/WTF/wtf/SplitTest.h:38
> +// Split testing, a.k.a. A/B testing, is used to determine whether a particular
> +// experiment should be enabled or not. This singleton is initialized at process
> +// start, e.g. by the WebContent process based on the user's UUID.
> +//

We don't normally add comments like to class declaration. Please remove.

> Source/WTF/wtf/SplitTest.h:54
> +    // Initialize the split test singleton once at process creation time with
> +    // 128 bits of unique identification.
> +    WTF_EXPORT void initialize(uint64_t lo, uint64_t hi);

Please rename the function to initializeSingleton to get rid of the comment.

> Source/WTF/wtf/SplitTest.h:58
> +    // Returns a boolean determining whether to enable an experiment or not.
> +    // Returns void if the singleton isn't initialized, in which case we
> +    // shouldn't run the experiment.

This comment is unnecessary.

> Source/WebCore/platform/WebCoreSplitTestInitializer.cpp:27
> +#include "WebCoreSplitTestInitializer.h"

We shouldn't be prefixing these .h & .cpp files with "WebCore".

> Source/WebCore/platform/WebCoreSplitTestInitializer.cpp:104
> +void initWebCoreSplitTest()

Ditto. This should be simply initSplitTest.

> Source/WebKit2/WebProcess/EntryPoint/mac/XPCService/WebContentServiceEntryPoint.mm:37
>  #import <WebCore/WebCoreThreadSystemInterface.h>

I don't understand why this header file has "WebCore" prefix but that should be fixed, not replicated.

[JF Bastien]

Created attachment 302089 [details]
patch

Address rniwa's comments.

[JF Bastien]

Created attachment 302090 [details]
patch

Missing file change.

[WebKit Commit Bot]

Attachment 302090 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/SplitTestInitializer.cpp:32:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexey Proskuryakov]

Comment on attachment 302090 [details]
patch

I'm told that the plan is to NOT land this patch on trunk at this time.

r=me

[JF Bastien]

Marking as won't fix for now. We can revisit some other time if needed.