[webkit-reviews] review granted: [Bug 18672] SQUIRRELFISH: codegen
fails with a large number of temporaries : [Attachment 20766]
Second run
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Apr 23 07:45:52 PDT 2008
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
More information about the webkit-reviews
mailing list