Don't create a GenericContinuation subclass for each continuation

A subclass of GenericContinuation is created for each
generator function in code we emit. This was originally intended to be
an optimization to allow burning in Func*s in the translation of
FPushContFunc, but that was not a noticeable win in perflab and tricky
to get right in sandbox mode (see the attached task). This diff
changes the emitter and runtime to not generate all these subclasses,
instead just using MethodContinuation and FunctionContinuation to
allow some specialization at translation time.

doc/bytecode.specification

@@ -3481,14 +3481,13 @@ NativeImpl    []  ->  []
 14. Continuation creation and execution
 ---------------------------------------
 
-CreateCont <getargs> <function name> <class name>    []  ->  [C]
-
-  Creates a <class name> (which must subclass c_GenericContinuation) object and
-  pushes it on the stack. The Continuation will capture all defined local
-  variables in the current function, and if the <getargs> immediate is nonzero
-  it will also store the result of func_get_args(). The Continuation will store
-  a reference to the function named by the string immediate to be used as its
-  body.
+CreateCont <getargs> <function name>    []  ->  [C]
+
+  Creates a GenericContinuation object and pushes it on the stack. The
+  Continuation will capture all defined local variables in the current
+  function, and if the <getargs> immediate is nonzero it will also store the
+  result of func_get_args(). The Continuation will store a reference to the
+  function named by the string immediate to be used as its body.
 
 UnpackCont    []  ->  [C:Int]
 

src/compiler/analysis/emitter.cpp

@@ -1035,19 +1035,6 @@ void MetaInfoBuilder::setForUnit(UnitEmitter& target) const {
   free(meta);
 }
 
-StringData* EmitterVisitor::continuationClassName(
-  const StringData* fname) {
-  std::ostringstream str;
-  str << "continuation$"
-      << std::hex
-      << m_curFunc->ue().md5().q[1] << m_curFunc->ue().md5().q[0]
-      << std::dec
-      << '$'
-      << fname->data();
-
-  return StringData::GetStaticString(str.str());
-}
-
 EmitterVisitor::EmitterVisitor(UnitEmitter& ue)
   : m_ue(ue), m_curFunc(ue.getMain()), m_evalStackIsUnknown(false),
     m_actualStackHighWater(0), m_fdescHighWater(0), m_closureCounter(0) {
@@ -1499,9 +1486,6 @@ void EmitterVisitor::visit(FileScopePtr file) {
       m_methLabels[methName] = new Label();
       // Emit afterwards
       postponeMeth(meth, NULL, true);
-      if (meth->getFunctionScope()->isGenerator()) {
-        newContinuationClass(methName);
-      }
     }
   }
   {
@@ -2318,7 +2302,6 @@ bool EmitterVisitor::visitImpl(ConstructPtr node) {
           }
 
           postponeMeth(m, NULL, true);
-          newContinuationClass(nName);
         } else {
           FuncEmitter* fe = m_ue.newFuncEmitter(nName, false);
           e.DefFunc(fe->id());
@@ -2847,7 +2830,7 @@ bool EmitterVisitor::visitImpl(ConstructPtr node) {
           const StringData* nameStr =
             StringData::GetStaticString(nameVar.getStringData());
           bool callGetArgs = params->getCount() == 4;
-          e.CreateCont(callGetArgs, nameStr, continuationClassName(nameStr));
+          e.CreateCont(callGetArgs, nameStr);
           return true;
         } else if (call->isCompilerCallToFunction("hphp_continuation_raised")) {
           e.ContRaised();
@@ -4897,15 +4880,6 @@ void EmitterVisitor::emitPostponedMeths() {
   }
 }
 
-void EmitterVisitor::newContinuationClass(const StringData* name) {
-  StringData* className = continuationClassName(name);
-  static const StringData* parentName =
-    StringData::GetStaticString("GenericContinuation");
-  PreClassEmitter* pce = m_ue.newPreClassEmitter(className,
-                                                 PreClass::AlwaysHoistable);
-  pce->init(0, 0, m_ue.bcPos(), AttrUnique, parentName, NULL);
-}
-
 void EmitterVisitor::emitPostponedCtors() {
   while (!m_postponedCtors.empty()) {
     PostponedCtor& p = m_postponedCtors.front();
@@ -5515,9 +5489,6 @@ PreClass::Hoistable EmitterVisitor::emitClass(Emitter& e, ClassScopePtr cNode,
         bool added UNUSED = pce->addMethod(fe);
         ASSERT(added);
         postponeMeth(meth, fe, false);
-        if (isGenerator) {
-          newContinuationClass(methName);
-        }
       } else if (ClassVariablePtr cv =
                  dynamic_pointer_cast<ClassVariable>((*stmts)[i])) {
         ModifierExpressionPtr mod(cv->getModifiers());
@@ -6434,6 +6405,22 @@ static Unit* emitHHBCNativeClassUnit(const HhbcExtClassInfo* builtinClasses,
     }
   }
 
+  // We also want two subclasses of GenericContinuation, for different
+  // types of continuations.
+  static const StringData* genericContinuation =
+    StringData::GetStaticString("GenericContinuation");
+  static const StringData* names[] = {
+    StringData::GetStaticString("MethodContinuation"),
+    StringData::GetStaticString("FunctionContinuation")
+  };
+  if (classEntries.size()) {
+    for (unsigned i = 0; i < array_size(names); ++i) {
+      PreClassEmitter* pce =
+        ue->newPreClassEmitter(names[i], PreClass::AlwaysHoistable);
+      pce->init(0, 0, ue->bcPos(), AttrUnique, genericContinuation, NULL);
+    }
+  }
+
   metaInfo.setForUnit(*ue);
 
   Unit* unit = ue->create();

src/compiler/analysis/emitter.h

@@ -584,8 +584,6 @@ class EmitterVisitor {
   void saveMaxStackCells(FuncEmitter* fe);
   void finishFunc(Emitter& e, FuncEmitter* fe);
   StringData* newClosureName();
-  StringData* continuationClassName(const StringData* fname);
-  void newContinuationClass(const StringData* name);
 
   void initScalar(TypedValue& tvVal, ExpressionPtr val);
 

src/runtime/base/execution_context.h

@@ -431,9 +431,9 @@ class VMExecutionContext : public BaseExecutionContext {
   void requestExit();
 
   static void getElem(TypedValue* base, TypedValue* key, TypedValue* dest);
+  template<bool isMethod>
   static c_GenericContinuation* createContinuation(VM::ActRec* fp, bool getArgs,
                                                    const VM::Func* origFunc,
-                                                   VM::Class* genClass,
                                                    const VM::Func* genFunc);
   static c_GenericContinuation* fillContinuationVars(
     VM::ActRec* fp, const VM::Func* origFunc, const VM::Func* genFunc,

src/runtime/vm/bytecode.cpp

@@ -6568,13 +6568,12 @@ inline void OPTBLD_INLINE VMExecutionContext::iopParent(PC& pc) {
   m_stack.pushClass(parent);
 }
 
+template<bool isMethod>
 c_GenericContinuation*
 VMExecutionContext::createContinuation(ActRec* fp,
                                        bool getArgs,
                                        const Func* origFunc,
-                                       Class* genClass,
                                        const Func* genFunc) {
-  bool isMethod = origFunc->isNonClosureMethod();
   Object obj;
   Array args;
   if (fp->hasThis()) {
@@ -6587,6 +6586,8 @@ VMExecutionContext::createContinuation(ActRec* fp,
   const StringData* origName =
     origFunc->isClosureBody() ? closure : origFunc->fullName();
   int nLocals = genFunc->numNamedLocals() - 1; //Don't need space for __cont__
+  Class* genClass = isMethod ? SystemLib::s_MethodContinuationClass
+                             : SystemLib::s_FunctionContinuationClass;
   c_GenericContinuation* cont =
     c_GenericContinuation::alloc(genClass, nLocals);
   memset(cont->locals(), 0, sizeof(TypedValue) * nLocals);
@@ -6665,16 +6666,16 @@ inline void OPTBLD_INLINE VMExecutionContext::iopCreateCont(PC& pc) {
   NEXT();
   DECODE_IVA(getArgs);
   DECODE_LITSTR(genName);
-  DECODE_LITSTR(className);
 
   const Func* origFunc = m_fp->m_func;
   const Func* genFunc = origFunc->getGeneratorBody(genName);
   ASSERT(genFunc != NULL);
 
-  Class* cls = Unit::lookupClass(className);
-  ASSERT(cls);
-  c_GenericContinuation* cont =
-    createContinuation(m_fp, getArgs, origFunc, cls, genFunc);
+  bool isMethod = origFunc->isNonClosureMethod();
+  c_GenericContinuation* cont = isMethod ?
+    createContinuation<true>(m_fp, getArgs, origFunc, genFunc) :
+    createContinuation<false>(m_fp, getArgs, origFunc, genFunc);
+
   fillContinuationVars(m_fp, origFunc, genFunc, cont);
 
   TypedValue* ret = m_stack.allocTV();

src/runtime/vm/hhbc.h

@@ -517,7 +517,7 @@ enum SetOpOp {
   O(Parent,          NA,               NOV,             ONE(AV),    NF) \
   O(LateBoundCls,    NA,               NOV,             ONE(AV),    NF) \
   O(NativeImpl,      NA,               NOV,             NOV,        CF_TF) \
-  O(CreateCont,      THREE(IVA,SA,SA), NOV,             ONE(CV),    NF) \
+  O(CreateCont,      TWO(IVA,SA),      NOV,             ONE(CV),    NF) \
   O(UnpackCont,      NA,               NOV,             ONE(CV),    NF) \
   O(PackCont,        ONE(IVA),         ONE(CV),         NOV,        NF) \
   O(ContRaised,      NA,               NOV,             NOV,        NF) \

src/runtime/vm/translator/translator-x64-internal.h

@@ -1,3 +1,18 @@
+/*
+   +----------------------------------------------------------------------+
+   | HipHop for PHP                                                       |
+   +----------------------------------------------------------------------+
+   | Copyright (c) 2010- Facebook, Inc. (http://www.facebook.com)         |
+   +----------------------------------------------------------------------+
+   | This source file is subject to version 3.01 of the PHP license,      |
+   | that is bundled with this package in the file LICENSE, and is        |
+   | available through the world-wide-web at the following url:           |
+   | http://www.php.net/license/3_01.txt                                  |
+   | If you did not receive a copy of the PHP license and are unable to   |
+   | obtain it through the world-wide-web, please send a note to          |
+   | license@php.net so we can mail you a copy immediately.               |
+   +----------------------------------------------------------------------+
+*/
 #ifndef incl_TRANSLATOR_X64_INTERNAL_H_
 #define incl_TRANSLATOR_X64_INTERNAL_H_
 

src/runtime/vm/translator/translator-x64.cpp

@@ -6080,28 +6080,29 @@ void TranslatorX64::translateCreateCont(const Tracelet& t,
                                         const NormalizedInstruction& i) {
   bool getArgs = i.imm[0].u_IVA;
   const StringData* genName = curUnit()->lookupLitstrId(i.imm[1].u_SA);
-  const StringData* className = curUnit()->lookupLitstrId(i.imm[2].u_SA);
   const Func* origFunc = curFunc();
-  Class* genClass = Unit::lookupClass(className);
-  ASSERT(genClass);
   const Func* genFunc = origFunc->getGeneratorBody(genName);
 
   if (false) {
     ActRec* fp = NULL;
     UNUSED c_GenericContinuation* cont =
-      VMExecutionContext::createContinuation(fp, getArgs, origFunc, genClass,
-                                             genFunc);
+      VMExecutionContext::createContinuation<true>(fp, getArgs, origFunc,
+                                                   genFunc);
+    VMExecutionContext::createContinuation<false>(fp, getArgs, origFunc,
+                                                 genFunc);
   }
 
   // Even callee-saved regs need to be clean, because
   // createContinuation will read all locals.
   m_regMap.cleanAll();
+  auto helper = origFunc->isNonClosureMethod() ?
+    VMExecutionContext::createContinuation<true> :
+    VMExecutionContext::createContinuation<false>;
   EMIT_CALL(a,
-             VMExecutionContext::createContinuation,
+             (TCA)helper,
              R(rVmFp),
              IMM(getArgs),
              IMM((intptr_t)origFunc),
-             IMM((intptr_t)genClass),
              IMM((intptr_t)genFunc));
   ScratchReg holdRax(m_regMap, rax);
 
@@ -7949,38 +7950,28 @@ TranslatorX64::translateFPushFuncD(const Tracelet& t,
 void
 TranslatorX64::translateFPushContFunc(const Tracelet& t,
                                       const NormalizedInstruction& i) {
-  // All functions that use this instruction are cloned into
-  // subclasses so we can burn in the Continuation's body Func*
   ASSERT(curFrame()->hasThis());
-  c_GenericContinuation* cont =
-    dynamic_cast<c_GenericContinuation*>(curFrame()->getThis());
-  ASSERT(cont);
-  const Func* genFunc = cont->m_vmFunc;
-  genFunc->validate();
-  ASSERT(IMPLIES(genFunc->isTraitMethod(), cont->m_isMethod));
-
-  // TODO: Make the FuncIds for Continuation subclasses depend on the
-  // containing class's parents as well, so we can burn in the Func*
-  // for method generators. See t1061448.
-  if (cont->m_isMethod) {
-    genFunc = NULL;
-  }
-
+  Class* genClass = curFrame()->getThis()->getVMClass();
+  ASSERT(genClass == SystemLib::s_MethodContinuationClass ||
+         genClass == SystemLib::s_FunctionContinuationClass);
+  bool isMethod = genClass == SystemLib::s_MethodContinuationClass;
   size_t thisOff = AROFF(m_this) - sizeof(ActRec);
   size_t funcOff = AROFF(m_func) - sizeof(ActRec);
   m_regMap.scrubStackRange(i.stackOff,
                            i.stackOff + kNumActRecCells);
-  emitPushAR(i, genFunc, 0, false, !cont->m_isMethod);
-  if (cont->m_isMethod) {
-    ScratchReg rCont(m_regMap);
-    ScratchReg rScratch(m_regMap);
-    a.  load_reg64_disp_reg64(rVmFp, AROFF(m_this), *rCont);
-    if (genFunc == NULL) {
-      a.load_reg64_disp_reg64(*rCont,
-                              offsetof(c_GenericContinuation, m_vmFunc),
-                              *rScratch);
-      emitVStackStore(a, i, *rScratch, funcOff, sz::qword);
-    }
+  emitPushAR(i, NULL, 0, false, false);
+  ScratchReg rCont(m_regMap);
+  ScratchReg rScratch(m_regMap);
+  a.  load_reg64_disp_reg64(rVmFp, AROFF(m_this), *rCont);
+
+  // Store the func
+  a.load_reg64_disp_reg64(*rCont,
+                          offsetof(c_GenericContinuation, m_vmFunc),
+                          *rScratch);
+  emitVStackStore(a, i, *rScratch, funcOff, sz::qword);
+
+  if (isMethod) {
+    // Store m_this
     a.  load_reg64_disp_reg64(*rCont,
                               offsetof(c_GenericContinuation, m_obj),
                               *rScratch);
@@ -7997,6 +7988,8 @@ TranslatorX64::translateFPushContFunc(const Tracelet& t,
       // m_vmCalledClass already has its low bit set
       emitVStackStore(a, i, *rScratch, thisOff, sz::qword);
     }
+  } else {
+    emitVStackStoreImm(a, i, 0, thisOff, sz::qword);
   }
 }
 

src/runtime/vm/vm.cpp

@@ -66,6 +66,8 @@ static StaticString s_resource(LITSTR_INIT("__resource"));
 static StaticString s_DOMException(LITSTR_INIT("DOMException"));
 static StaticString s_PDOException(LITSTR_INIT("PDOException"));
 static StaticString s_SoapFault(LITSTR_INIT("SoapFault"));
+static StaticString s_MethodContinuation(LITSTR_INIT("MethodContinuation"));
+static StaticString s_FunctionContinuation(LITSTR_INIT("FunctionContinuation"));
 
 class VMClassInfoHook : public ClassInfoHook {
 public:
@@ -226,10 +228,21 @@ void ProcessInit() {
   // load builtins
   SystemLib::s_nativeFuncUnit->merge();
 
+  // We call a special bytecode emitter function to build the native
+  // unit which will contain all of our cppext functions and classes.
+  // Each function and method will have a bytecode body that will thunk
+  // to the native implementation.
+  Unit* nativeClassUnit = build_native_class_unit(hhbc_ext_classes,
+                                                  hhbc_ext_class_count);
+  SystemLib::s_nativeClassUnit = nativeClassUnit;
+
+  // Load the nativelib unit to build the Class objects
+  SystemLib::s_nativeClassUnit->merge();
+
 #define INIT_SYSTEMLIB_CLASS_FIELD(cls)                                 \
   {                                                                     \
     Class *cls = *Unit::GetNamedEntity(s_##cls.get())->clsList();       \
-    ASSERT(cls);                                                        \
+    ASSERT(!hhbc_ext_class_count || cls);                               \
     SystemLib::s_##cls##Class = cls;                                    \
   }
 
@@ -251,20 +264,11 @@ void ProcessInit() {
   INIT_SYSTEMLIB_CLASS_FIELD(DOMException);
   INIT_SYSTEMLIB_CLASS_FIELD(PDOException);
   INIT_SYSTEMLIB_CLASS_FIELD(SoapFault);
+  INIT_SYSTEMLIB_CLASS_FIELD(MethodContinuation);
+  INIT_SYSTEMLIB_CLASS_FIELD(FunctionContinuation);
 
 #undef INIT_SYSTEMLIB_CLASS_FIELD
 
-  // We call a special bytecode emitter function to build the native
-  // unit which will contain all of our cppext functions and classes.
-  // Each function and method will have a bytecode body that will thunk
-  // to the native implementation.
-  Unit* nativeClassUnit = build_native_class_unit(hhbc_ext_classes,
-                                                  hhbc_ext_class_count);
-  SystemLib::s_nativeClassUnit = nativeClassUnit;
-
-  // Load the nativelib unit to build the Class objects
-  SystemLib::s_nativeClassUnit->merge();
-
   // Retrieve all of the class pointers
   for (long long i = 0LL; i < hhbc_ext_class_count; ++i) {
     const HhbcExtClassInfo* info = hhbc_ext_classes + i;

src/system/lib/systemlib.cpp

@@ -57,6 +57,8 @@ HPHP::VM::Class* SystemLib::s_pinitSentinelClass = NULL;
 HPHP::VM::Class* SystemLib::s_resourceClass = NULL;
 HPHP::VM::Class* SystemLib::s_DOMExceptionClass = NULL;
 HPHP::VM::Class* SystemLib::s_SoapFaultClass = NULL;
+HPHP::VM::Class* SystemLib::s_MethodContinuationClass = NULL;
+HPHP::VM::Class* SystemLib::s_FunctionContinuationClass = NULL;
 
 ObjectData* SystemLib::AllocStdClassObject() {
   if (hhvm) {

src/system/lib/systemlib.h

@@ -55,6 +55,8 @@ class SystemLib {
   static HPHP::VM::Class* s_DOMExceptionClass;
   static HPHP::VM::Class* s_PDOExceptionClass;
   static HPHP::VM::Class* s_SoapFaultClass;
+  static HPHP::VM::Class* s_MethodContinuationClass;
+  static HPHP::VM::Class* s_FunctionContinuationClass;
 
   static HPHP::VM::Func* GetNullFunction();
 

src/util/base.h

@@ -559,6 +559,11 @@ To safe_cast(From val) {
     return static_cast<To>(val);
   }
 }
+
+template<class T, size_t Sz>
+size_t array_size(T (&t)[Sz]) {
+  return Sz;
+}
 }
 
 namespace boost {