[Webkit-unassigned] [Bug 33178] Segmented SharedBuffer: SharedBuffer doesn't have to be a flat memory block

[bugzilla-daemon at webkit.org]

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


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #45821|review?                     |review-
               Flag|                            |




--- Comment #3 from Darin Adler <darin at apple.com>  2010-01-04 13:14:28 PST ---
(From update of attachment 45821)
Looks OK. But with no clients using the getSomeData function, it seems like
it's scaffolding for some future work and functions like getSomeData are not
tested. And this change will make things less efficient until the major clients
adopt.

Do you know what adding this temporary extra work does to page loading time? I
don't want to get stuck with that.

This patch also changes behavior when running out of memory. The new code will
abort when running out of memory, whereas the old code would drop data. I think
that probably should be changed in a separate patch rather than as a side
effect of this one.

> +#define SEGMENT_SIZE        (0x1000)
> +#define SEGMENT_POS_MASK    (0x0FFF)
> +#define GET_SEGMENT(position) ((position) / SEGMENT_SIZE)
> +#define GET_OFF_IN_SEGMENT(globalPosition) ((globalPosition) & SEGMENT_POS_MASK)

We use C++ constants and inline functions rather than macros for this sort of
thing.

> +    unsigned posInSegment = GET_OFF_IN_SEGMENT(m_size - m_buffer.size());

We try to avoid abbreviations. The existing code uses "len" but should use
"length". And here we would use "position" or "offset" rather than "pos" or
"off".

> +    unsigned toCopy = len < segmentFreeSpace ? len : segmentFreeSpace;

It's better to use a noun or adjective for this sort of variable rather than
the preposition "to". For example, "bytesToCopy" is probably a good name.

> +    RefPtr<SharedBuffer> clone(adoptRef(new SharedBuffer()));

No need for the parentheses here after SharedBuffer.

> +    clone->m_size = m_size;
> +    clone->m_buffer.append(m_buffer.data(), m_buffer.size());
> +    clone->m_segments.reserveCapacity(m_segments.size());
> +    for (Vector<char*>::const_iterator i = m_segments.begin(); i != m_segments.end(); ++i) {
> +        char* segment = allocateSegment();
> +        memcpy(segment, *i, SEGMENT_SIZE);
> +        clone->m_segments.append(segment);
> +    }
> +    return clone;

Why is it a good idea to clone all the segments into separate new segments
rather than making a single large segment?

> +        char* dest = m_buffer.data() + bufferSize;

Again, avoid abbreviation if possible.

> +        unsigned left = m_size - bufferSize;

The word "left" is a strange name for this. Maybe bytesLeft would be OK.

> +        for (Vector<char*>::const_iterator i = m_segments.begin(); i != m_segments.end(); ++i) {

We normally use array-style index iteration for vectors. The iterators are just
there for things like STL algorithms.

> +unsigned SharedBuffer::getSomeData(const char** someData, unsigned pos) const
> +{
> +    if (hasPlatformData() || m_purgeableBuffer) {
> +        *someData = data();
> +        return size();
> +    }

The above code seems clearly wrong when pos is non-zero.

> +    // Calling this function will force internal segmented buffers
> +    // to be merged into a flat buffer. Use getSomeData() whenever possible
> +    // for better performance.
>      const char* data() const;
>      unsigned size() const;

This comment comes before a paragraph of two functions. Please add a blank
line.

> +    // Calling this function will force internal segmented buffers
> +    // to be merged into a flat buffer. Use getSomeData() whenever possible
> +    // for better performance.
> +    const Vector<char> &buffer() const;

You should change this to "const Vector<char>& buffer()" -- the existing code
was formatted incorrectly.

> +    // Return the flat data size after "pos". "*data" will be the address
> +    // Return 0 when no more data left.

I suggest using "const char*&" for data instead of "const char**".

I don't think the term "flat data size" is clear.

> +    unsigned getSegment(unsigned position) const;
> +    unsigned getOffsetInSegment(unsigned globalPosition) const;

WebKit code normally does not use "get" in the names of functions that have
return values. The above functions should be named segmentIndex() and
offsetInSegment().

review- because of multiple issues above.

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