Bug 203993 – Add FuzzerAgents that narrow and widen number predictions

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

[Tuomas Karkkainen]

Add FuzzerAgents that narrow and widen number predictions

[Tuomas Karkkainen]

Created attachment 383121 [details]
proposed patch

proposed patch

[Tuomas Karkkainen]

Created attachment 383126 [details]
updated patch

added m_lock

[Mark Lam]

Comment on attachment 383126 [details]
updated patch

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

> Source/JavaScriptCore/ChangeLog:5
> +        Add two FuzzerAgents such that for any predictions that are originally subsets of SpecFullNumber:
> +        - one adds more number types to the prediction
> +        - the other removes some of the number types from the prediction

You should put this txt that explains what's changed in the region below "Reviewed by ..." but above the list of changed files.  Here, you should have a 1 line title instead e.g. "Add FuzzerAgents that narrow and widen number predictions." from the bug title.

> Source/JavaScriptCore/ChangeLog:9
> +        Reviewed by NOBODY (OOPS!).
> +

Move the above "what's changed" text here after the "Reviewed by ..." line.

[Tuomas Karkkainen]

Created attachment 383149 [details]
fix ChangeLog as per review

[Tuomas Karkkainen]

Created attachment 383437 [details]
proposed patch

replaces SpecFullNumber with SpecBytecodeNumber as the superset of number types included in the speculation.

also simplifies a couple of things.

previous patches returned 'original' from the narrowing fuzzer agent, as opposed to the intended 'generated'.

[Tuomas Karkkainen]

Created attachment 384604 [details]
proposed patch

updated to address changes from other FuzzerAgent patches

[Tuomas Karkkainen]

Created attachment 384605 [details]
proposed patch

remove extra whitespace change

[Tuomas Karkkainen]

Created attachment 384692 [details]
proposed patch

update patch to account for external changes.

[Yusuke Suzuki]

Comment on attachment 384692 [details]
proposed patch

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

I have question about how narrowing and widening types.

> Source/JavaScriptCore/runtime/NarrowingNumberPredictionFuzzerAgent.cpp:49
> +    unsigned numberOfTypesToKeep = m_random.getUint32(numberTypesThatCouldBePartOfSpeculation.size() + 1);

This means that, we could see SpeculateType sets in the same ratio for each count. Let's consider the case that is having 5 SpeculatedTypes. So,

0 => 1/6
1 => 1/6
...
4 => 1/6
5 => 1/6

We have 5 combination when "4" is picked. And each one gets 1/24, which is 1/4 when comparing to SpecNone. Is it intentional?

> Source/JavaScriptCore/runtime/WideningNumberPredictionFuzzerAgent.cpp:61
> +    unsigned numberOfTypesToAdd = m_random.getUint32(numberTypesNotIncludedInSpeculation.size() + 1);
> +    if (!numberOfTypesToAdd)
> +        return original;
> +
> +    SpeculatedType generated = original;
> +    for (unsigned i = 0; i < numberOfTypesToAdd; i++) {
> +        unsigned indexOfNewType = m_random.getUint32(numberTypesNotIncludedInSpeculation.size());
> +        mergeSpeculation(generated, numberTypesNotIncludedInSpeculation[indexOfNewType]);
> +        numberTypesNotIncludedInSpeculation.remove(indexOfNewType);
> +    }

Ditto.

[Tuomas Karkkainen]

(In reply to Yusuke Suzuki from comment #9)
> Comment on attachment 384692 [details]
> proposed patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=384692&action=review
> 
> I have question about how narrowing and widening types.
> 
> > Source/JavaScriptCore/runtime/NarrowingNumberPredictionFuzzerAgent.cpp:49
> > +    unsigned numberOfTypesToKeep = m_random.getUint32(numberTypesThatCouldBePartOfSpeculation.size() + 1);
> 
> This means that, we could see SpeculateType sets in the same ratio for each
> count. Let's consider the case that is having 5 SpeculatedTypes. So,
> 
> 0 => 1/6
> 1 => 1/6
> ...
> 4 => 1/6
> 5 => 1/6
> 
> We have 5 combination when "4" is picked. And each one gets 1/24, which is
> 1/4 when comparing to SpecNone. Is it intentional?

For the widening FuzzerAgent I think this is fine. For the narrowing FuzzerAgent I don't like how often it returns SpecNone. I think it would probably be better to move the "+ 1" outside of the parentheses, which would mean that it never returned SpecNone, and single number type predictions were always left unchanged.

The real answer depends on how the speculations affect the machine code, and I don't have an answer for that.

> > Source/JavaScriptCore/runtime/WideningNumberPredictionFuzzerAgent.cpp:61
> > +    unsigned numberOfTypesToAdd = m_random.getUint32(numberTypesNotIncludedInSpeculation.size() + 1);
> > +    if (!numberOfTypesToAdd)
> > +        return original;
> > +
> > +    SpeculatedType generated = original;
> > +    for (unsigned i = 0; i < numberOfTypesToAdd; i++) {
> > +        unsigned indexOfNewType = m_random.getUint32(numberTypesNotIncludedInSpeculation.size());
> > +        mergeSpeculation(generated, numberTypesNotIncludedInSpeculation[indexOfNewType]);
> > +        numberTypesNotIncludedInSpeculation.remove(indexOfNewType);
> > +    }
> 
> Ditto.

[Tuomas Karkkainen]

Created attachment 385135 [details]
proposed patch

Changes the narrowing fuzzer agent to always keep at least one number type. This means it never returns SpecNone.

[Yusuke Suzuki]

Comment on attachment 385135 [details]
proposed patch

OK, r=me

[WebKit Commit Bot]

Comment on attachment 385135 [details]
proposed patch

Clearing flags on attachment: 385135

Committed r254230: <https://trac.webkit.org/changeset/254230>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/58424588>