[refactoring] Split CodeAssemblerState out of CodeAssembler

src/compiler/code-assembler.h

Index: src/compiler/code-assembler.h
diff --git a/src/compiler/code-assembler.h b/src/compiler/code-assembler.h
index b3369004a2ce499a7f10fddceb608c0cffe46296..e55631636a02425d93de88b5064a0c7c7ee72f87 100644
--- a/src/compiler/code-assembler.h
+++ b/src/compiler/code-assembler.h
@@ -173,18 +173,16 @@ class RawMachineLabel;
 // clients, CodeAssembler also provides an abstraction for creating variables
 // and enhanced Label functionality to merge variable values along paths where
 // they have differing values, including loops.
+//
+// The CodeAssembler itself is stateless (and instances are expected to be
+// temporary-scoped and short-lived); all its state is encapsulated into
+// a CodeAssemblerState instance.
+
+class CodeAssemblerState;

[Michael Starzinger, 2016/11/15 11:45:20]

nit: Please move this forward declaration up to the other ones at the top of
this file.

[Jakob Kummerow, 2016/11/15 13:40:32]

Done.

+
 class V8_EXPORT_PRIVATE CodeAssembler {
  public:
-  // Create with CallStub linkage.
-  // |result_size| specifies the number of results returned by the stub.
-  // TODO(rmcilroy): move result_size to the CallInterfaceDescriptor.
-  CodeAssembler(Isolate* isolate, Zone* zone,
-                const CallInterfaceDescriptor& descriptor, Code::Flags flags,
-                const char* name, size_t result_size = 1);
-
-  // Create with JSCall linkage.
-  CodeAssembler(Isolate* isolate, Zone* zone, int parameter_count,
-                Code::Flags flags, const char* name);
+  explicit CodeAssembler(CodeAssemblerState* state) : state_(state) {}
 
   virtual ~CodeAssembler();
 
@@ -207,6 +205,7 @@ class V8_EXPORT_PRIVATE CodeAssembler {
 
    private:
     friend class CodeAssembler;
+    friend class CodeAssemblerState;
     class Impl;
     Impl* impl_;
     CodeAssembler* assembler_;
@@ -471,17 +470,12 @@ class V8_EXPORT_PRIVATE CodeAssembler {
   virtual void CallEpilogue();
 
  private:
-  CodeAssembler(Isolate* isolate, Zone* zone, CallDescriptor* call_descriptor,
-                Code::Flags flags, const char* name);
-
   Node* CallN(CallDescriptor* descriptor, Node* code_target, Node** args);
   Node* TailCallN(CallDescriptor* descriptor, Node* code_target, Node** args);
 
-  std::unique_ptr<RawMachineAssembler> raw_assembler_;
-  Code::Flags flags_;
-  const char* name_;
-  bool code_generated_;
-  ZoneSet<Variable::Impl*> variables_;
+  RawMachineAssembler* raw_assembler() const;
+
+  CodeAssemblerState* state_;
 
   DISALLOW_COPY_AND_ASSIGN(CodeAssembler);
 };
@@ -523,6 +517,40 @@ class CodeAssembler::Label {
   std::map<Variable::Impl*, std::vector<Node*>> variable_merges_;
 };
 
+class V8_EXPORT_PRIVATE CodeAssemblerState {

[Michael Starzinger, 2016/11/15 11:45:20]

nit: Can we make this final?

[Igor Sheludko, 2016/11/15 13:06:35]

Thinking out loud: we could even hide this class inside .cc file if we add
static methods that create CodeAssemblerState object to CodeAssembler class.

[Jakob Kummerow, 2016/11/15 13:40:32]

We could do that. But then we can't heap-allocate them any more. Do we want
that?

[Jakob Kummerow, 2016/11/15 13:40:32]

I'd love to; but see comment on unit test.

[Igor Sheludko, 2016/11/15 14:02:53]

Ah, I missed the DISALLOW_COPY_AND_ASSIGN thing.

+ public:
+  // Create with CallStub linkage.
+  // |result_size| specifies the number of results returned by the stub.
+  // TODO(rmcilroy): move result_size to the CallInterfaceDescriptor.
+  CodeAssemblerState(Isolate* isolate, Zone* zone,
+                     const CallInterfaceDescriptor& descriptor,
+                     Code::Flags flags, const char* name,
+                     size_t result_size = 1);
+
+  // Create with JSCall linkage.
+  CodeAssemblerState(Isolate* isolate, Zone* zone, int parameter_count,
+                     Code::Flags flags, const char* name);
+
+  ~CodeAssemblerState();
+
+  Handle<Code> GenerateCode();

[Michael Starzinger, 2016/11/15 11:45:20]

Can we move this back to the {CodeAssembler} class? That way this class is pure
container class without logic. The only way the state of this class changes is
by using the {CodeAssmelber} to mutate it. WDYT?

[Igor Sheludko, 2016/11/15 13:06:35]

If we do that then we will not be able to generate builtins code from the state
as we do in BuildWithCodeStubAssembler*().

[Jakob Kummerow, 2016/11/15 13:40:32]

Summarizing offline discussion:
- The argument in favor of moving this back is so that the State is just a data
container with no logic at all. That design preserves maximum freedom from the
compiler:: side to change under-the-hood implementation details.
- The argument in favor of keeping it on the State object is that the various
*Assembler classes are responsible for accumulating things in the State
container. Taking the accumulated stuff out of the container is a separate use
case; having to instantiate a CodeAssembler just to be able to do that doesn't
seem to add much value.

Compromise idea: make it a static function!

[Igor Sheludko, 2016/11/15 14:02:53]

Awesome!

+
+ private:
+  friend class CodeAssembler;
+
+  CodeAssemblerState(Isolate* isolate, Zone* zone,
+                     CallDescriptor* call_descriptor, Code::Flags flags,
+                     const char* name);
+
+  std::unique_ptr<RawMachineAssembler> raw_assembler_;
+  Code::Flags flags_;
+  const char* name_;
+  bool code_generated_;
+  ZoneSet<CodeAssembler::Variable::Impl*> variables_;
+
+  DISALLOW_COPY_AND_ASSIGN(CodeAssemblerState);
+};
+
 }  // namespace compiler
 }  // namespace internal
 }  // namespace v8