Bug 204763 – Only check each use...FuzzerAgent() option in VM constructor if any of the options are enabled.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

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.

[Mark Lam]

Created attachment 384652 [details]
proposed patch.

[Tadeu Zagallo]

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.

[Mark Lam]

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.

[Keith Miller]

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.

[Mark Lam]

Thanks for the review.  Landed in r253015: <http://trac.webkit.org/r253015>.

[Radar WebKit Bug Importer]

<rdar://problem/57575360>

[Saam Barati]

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.