[Webkit-unassigned] [Bug 94712] High memory usage spike in AssemblerBuffer

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=94712





--- Comment #7 from Yong Li <yoli at rim.com>  2012-08-23 14:39:34 PST ---
(In reply to comment #6)

Not ready for review yet. But thanks for reviewing.

> (From update of attachment 160242 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=160235&action=review
> 
> Please add WTF tests for the SegmentedVector additions.
> 
> > Source/WTF/wtf/SegmentedVector.h:-44
> > -        ~SegmentedVectorIterator() { }
> 
> I think that was here so that some compiler do not generate a real function for the empty destructor.

I can recover it easily. But isn't that a compiler bug? as generating a default destructor is so fundamental in C++. If we do think this could happen, shouldn't there be a coding rule saying we have to explicitly provide a destructor for every class in WebKit? Actually there are tons of classes in WebKit that don't have dtor specified.

> 
> > Source/WTF/wtf/SegmentedVector.h:72
> > +            m_index += offset;
> > +            m_segment += m_index / SegmentSize;
> > +            m_index %= SegmentSize;
> 
> Use a temporary variable instead of abusing m_index.

OK.

> 
> > Source/WTF/wtf/SegmentedVector.h:79
> > +            // Points to the "end" symbol
> > +            m_segment = 0;
> > +            m_index = SegmentSize;
> 
> Missing period on the comment.
> Instead of duplicating the code, please consider making it an utility function.

Good call!

> 
> > Source/WTF/wtf/SegmentedVector.h:112
> > +        size_t indexInSegment() const { return m_index; }
> > +
> > +        void proceedToNextSegment()
> > +        {
> > +            if (++m_segment >= m_vector.m_segments.size()) {
> > +                // Points to the "end" symbol
> > +                m_segment = 0;
> > +                m_index = SegmentSize;
> > +            } else
> > +                m_index = 0;
> > +        }
> 
> This is weird. Isn't this the wrong abstraction?
Why?

> Why not give the "copy" capability to SegmentedVector?

Like SegmentedVector::copyTo(T* dest, size_t pos, size_t len)? yeah, probably better.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.