Bug 204763

Summary: Only check each use...FuzzerAgent() option in VM constructor if any of the options are enabled.
Product: WebKit Reporter: Mark Lam <mark.lam>
Component: JavaScriptCoreAssignee: Mark Lam <mark.lam>
Status: RESOLVED FIXED    
Severity: Normal CC: ews, keith_miller, msaboff, sbarati, tzagallo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
proposed patch. keith_miller: review+

Description Mark Lam 2019-12-02 13:18:53 PST
We know that we'll never use fuzzer agents in deployment.  Hence, we shouldn't spend time checking for them in the normal use case.
Comment 1 Mark Lam 2019-12-02 13:26:02 PST
Created attachment 384652 [details]
proposed patch.
Comment 2 Tadeu Zagallo 2019-12-02 14:05:15 PST
Comment on attachment 384652 [details]
proposed patch.

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

> Source/JavaScriptCore/runtime/Options.cpp:550
> +    FOR_EACH_JSC_OPTION(CHECK_IF_USING_FUZZER_AGENT)

This is a bit unfortunate... how about maybe adding FOR_EACH_JSC_FUZZER_AGENT? That way you can call it here and just have FOR_EACH_JSC_OPTION call it as well.
Comment 3 Mark Lam 2019-12-02 14:14:04 PST
Comment on attachment 384652 [details]
proposed patch.

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

>> Source/JavaScriptCore/runtime/Options.cpp:550
>> +    FOR_EACH_JSC_OPTION(CHECK_IF_USING_FUZZER_AGENT)
> 
> This is a bit unfortunate... how about maybe adding FOR_EACH_JSC_FUZZER_AGENT? That way you can call it here and just have FOR_EACH_JSC_OPTION call it as well.

Why?  This code compiles down to one check each for the 4 use...FuzzerAgent options.  All other options are no-ops that hey optimized away.  I already verified it with a disassembler.
Comment 4 Keith Miller 2019-12-02 16:12:20 PST
Comment on attachment 384652 [details]
proposed patch.

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

> Source/JavaScriptCore/runtime/Options.cpp:545
> +        if (nameLength > 14 && !strncmp(name, "use", 3) && !strncmp(&name[nameLength -11], "FuzzerAgent", 11)) { \

I think you just need to check for 11 but it probably doesn't matter.

>>> Source/JavaScriptCore/runtime/Options.cpp:550
>>> +    FOR_EACH_JSC_OPTION(CHECK_IF_USING_FUZZER_AGENT)
>> 
>> This is a bit unfortunate... how about maybe adding FOR_EACH_JSC_FUZZER_AGENT? That way you can call it here and just have FOR_EACH_JSC_OPTION call it as well.
> 
> Why?  This code compiles down to one check each for the 4 use...FuzzerAgent options.  All other options are no-ops that hey optimized away.  I already verified it with a disassembler.

Yeah, I don't think this matters because clang will probably (TM) compile fold this into a constant in production.
Comment 5 Mark Lam 2019-12-02 16:21:18 PST
Thanks for the review.  Landed in r253015: <http://trac.webkit.org/r253015>.
Comment 6 Radar WebKit Bug Importer 2019-12-02 16:22:19 PST
<rdar://problem/57575360>
Comment 7 Saam Barati 2019-12-02 17:08:16 PST
Comment on attachment 384652 [details]
proposed patch.

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

> Source/JavaScriptCore/ChangeLog:13
> +        We know that we'll never use fuzzer agents in deployment.  Hence, we shouldn't
> +        spend time checking for them in the normal use case.  This probably doesn't matter
> +        much for Web processes, but for clients of JSC that repeatedly spawn and kill VMs,
> +        it might matter more.  We might want to eventually widen this idiom to include
> +        other debugging / development options, but for now, I'm only covering the fuzzer
> +        agent options.

I actually think our time is better spent profiling where we spend time in VM construction and making measurable improvements. It seems super unlikely checking falsey debug options is where VM construction spends its time.