Add FuzzerAgents that narrow and widen number predictions
Created attachment 383121 [details] proposed patch proposed patch
Created attachment 383126 [details] updated patch added m_lock
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.
Created attachment 383149 [details] fix ChangeLog as per review
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'.
Created attachment 384604 [details] proposed patch updated to address changes from other FuzzerAgent patches
Created attachment 384605 [details] proposed patch remove extra whitespace change
Created attachment 384692 [details] proposed patch update patch to account for external changes.
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.
(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.
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.
Comment on attachment 385135 [details] proposed patch OK, r=me
Comment on attachment 385135 [details] proposed patch Clearing flags on attachment: 385135 Committed r254230: <https://trac.webkit.org/changeset/254230>
All reviewed patches have been landed. Closing bug.
<rdar://problem/58424588>