Currently, we use inference's analyzeTypes to get the type information needed to figure out definite properties on scripts invoked using 'new'. This is the last remaining use of analyzeTypes, and it would be good to remove this, and allow killing analyzeTypes for good, to simplify the threading model for reading and updating type sets in bug 785905. The new approach would be to run IonBuilder on a script the first time it is called with 'new', to analyze types and make 'inlining' decisions. From the resulting MIR graph all the info stored in newScript can be determined. The MIR graph probably wouldn't be compiled into an IonScript as without monitoring type information is pretty limited.

This patch removes the definite properties analysis from jsinfer.cpp that is based on analyzeTypes state, and instead uses an IonBuilder-constructed MIR graph analysis to do essentially the same thing.

Comment on attachment 796752 [details] [diff] [review] patch (f25d46b4f39f) Review of attachment 796752 [details] [diff] [review]: ----------------------------------------------------------------- Very nice, r=me with comments below addressed. Does this mean we can remove analyzeTypes now? ::: js/src/jit/IonAnalysis.cpp @@ +1760,5 @@ > + > +static bool > +AppendUses(MDefinition *def, Vector<MUse *> *useList) > +{ > + for (MUseIterator uses = def->usesBegin(); uses != def->usesEnd(); uses++) { I think you can use MUseDefIterator here instead, because we always skip non-definition uses. Then you can also change the vector's type from MUse to MDefinition or MInstruction. @@ +1849,5 @@ > + // Don't track |this| through assignments to phis. > + if (!use->consumer()->toDefinition()->isInstruction()) > + return true; > + > + MInstruction *ins = use->consumer()->toDefinition()->toInstruction(); ... and don't need these checks. @@ +1852,5 @@ > + > + MInstruction *ins = use->consumer()->toDefinition()->toInstruction(); > + > + // Follow any wrapper instructions to their own uses. > + if (ins->isGuardShape()) { IonBuilder will always use CallSetProperty and CallGetProperty when analyzing new script properties, so where's this GuardShape coming from? It would be good to add a comment if you saw this thwarting the analysis somewhere. Also, if we don't need this check, we can get rid of this whole loop I think and maybe inline AppendUses because it will have only one caller. ::: js/src/jsinfer.cpp @@ +4932,5 @@ > + initializerList.length()); > + > +#else // JS_ION > + return; > +#endif // JS_ION Nit: I think you only need the #endif, without the "#else return;"

(In reply to Jan de Mooij [:jandem] from comment #2) > Does this mean we can remove analyzeTypes now? Yes, once this is in we'll be able to remove analyzeTypes and a whole bunch of related stuff.

Followup to fix a couple issues. apply() calls in octane-raytrace weren't being inlined, and handle an old bug where we were doing the definite properties analysis over 10000 times on some benchmarks (!). Fixing the latter improves deltablue and raytrace by several thousand points and is a ~1-2% win on octane on my machine.

Comment on attachment 798189 [details] [diff] [review] followup Review of attachment 798189 [details] [diff] [review]: ----------------------------------------------------------------- Wow nice find!