[webkit-reviews] review granted: [Bug 18672] SQUIRRELFISH: codegen fails with a large number of temporaries : [Attachment 20766] Second run

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has granted Oliver Hunt <oliver at apple.com>'s
request for review:
Bug 18672: SQUIRRELFISH: codegen fails with a large number of temporaries
http://bugs.webkit.org/show_bug.cgi?id=18672

Attachment 20766: Second run
http://bugs.webkit.org/attachment.cgi?id=20766&action=edit

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
+		A7C31DA90DBEBA4300FDF8EB /* SegmentedVector.h in Headers */ =
{isa = PBXBuildFile; fileRef = A7C31DA80DBEBA4300FDF8EB /* SegmentedVector.h
*/; };
+		A7C31DAA0DBEBA4300FDF8EB /* SegmentedVector.h in Headers */ =
{isa = PBXBuildFile; fileRef = A7C31DA80DBEBA4300FDF8EB /* SegmentedVector.h
*/; };

SegmentedVector is really a component of the compiler, not the VM. I think it
belongs in the compiler folder.

+	     if (!(m_size % SegmentSize) && m_size >= SegmentSize)

Would

if (!(m_size % SegmentSize) && m_size > 0)

do the same job? If so, I think it would be clearer.

+	 void removeLast()
+	 {
+	     ASSERT(m_size);
+	     m_size--;
+	     m_buffers.last()->removeLast();
+	     if (!(m_size % SegmentSize) && m_size >= SegmentSize) {
+		 delete m_buffers.last();
+		 m_buffers.removeLast();
+	     }
+	 }

Technically, if you kept adding and removing an item at the segement boundary,
this algorithm would give you really bad performance, because it would
repeatedly malloc/free a large block. You could fix the problem by keeping a
size and a capacity, just like Vector does. I don't think this is a blocker to
landing, though, since you'd have to get pretty unlucky to hit this case during

codegen.

+	     if (index < SegmentSize)
+		 return m_inlineBuffer[index];
+	     return m_buffers[index / SegmentSize]->at(index % SegmentSize);

Why do we need to special-case the inline buffer here? Shouldn't the math "just

work"? I'd recommend removing the special case.

+	 void shrink(size_t size)
+	 {
...
+	 }
+
+	 void grow(size_t size)
+	 {
...
+	 }

These methods look a little long for inlining. I'd recommend moving them out of

the class declaration.

+	 void grow(size_t size)
+	 {
+	     ASSERT(size > m_size);
+	     if (size <= SegmentSize) {
+		 m_inlineBuffer.resize(size);

You can call "grow" here instead of "resize". Same thing in a few other places
in this function.

+	 Segment m_inlineBuffer;
+	 Vector<Segment*, 32> m_buffers;

Quibble: is it a "segment" or a "buffer"? I would call these "m_inlineSegment"
and "m_segments".

Nice work.

r=me