Bug 233481 - SharedBufferChunkReader should be refactored
Summary: SharedBufferChunkReader should be refactored
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jean-Yves Avenard [:jya]
URL:
Keywords: InRadar
Depends on:
Blocks: 233030
  Show dependency treegraph
 
Reported: 2021-11-24 14:00 PST by Jean-Yves Avenard [:jya]
Modified: 2021-11-25 21:04 PST (History)
7 users (show)

See Also:


Attachments
Patch (31.16 KB, patch)
2021-11-25 17:44 PST, Jean-Yves Avenard [:jya]
no flags Details | Formatted Diff | Diff
Patch (31.38 KB, patch)
2021-11-25 20:09 PST, Jean-Yves Avenard [:jya]
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jean-Yves Avenard [:jya] 2021-11-24 14:00:08 PST
SharedBufferChunkReader is used to read the content of a SharedBuffer into a Vector.

It flattens the SharedBuffer which is inefficient.

It was first noted in bug 170956 that it should be rewritten and use the SharedBuffer's iterator instead.

Following bug 233030 where SharedBuffer are no longer mutable ; it required the SharedBufferChunkReader to keep a strong ref to a ContiguousSharedBuffer which would increase total memory use.
Comment 1 Radar WebKit Bug Importer 2021-11-24 14:00:54 PST
<rdar://problem/85733358>
Comment 2 Jean-Yves Avenard [:jya] 2021-11-25 17:19:13 PST
this task would allow for bug 233030 to be a tad smaller and is enough to do
Comment 3 Jean-Yves Avenard [:jya] 2021-11-25 17:44:02 PST
Created attachment 445164 [details]
Patch
Comment 4 Cameron McCormack (:heycam) 2021-11-25 19:22:05 PST
Comment on attachment 445164 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=445164&action=review

> Source/WebCore/ChangeLog:10
> +        SharedBufferChunkReader required the SharedBuffer to be flatten which
> +        would mutate the underlying SharedBuffer.

"to be flattened"

But I'd rewrite this to describe the change being made, e.g.:

Avoid flattening a SharedBuffer when reading reading it through SharedBufferChunkReader.

And maybe this can be the top line of the ChangeLog in place of "Rewrite SharedBufferChunkReader", which is a bit non-specific.

> Source/WebCore/ChangeLog:11
> +        the peek method was also incorrect if the data read overlapped multiple

Similarly change this to describe the change being made:

Fix a bug in the peek method where too many characters would be read if the requested size overlaps multiple segments.

> Source/WebCore/ChangeLog:17
> +        We add it to makefiles and export the header so that API tests can be written.

(Probably doesn't need to be mentioned.)

> Source/WebCore/platform/SharedBuffer.cpp:307
> +        size_t segmentLength = std::min(currentSegment->segment->size(), remaining);
> +        data.append(currentSegment->segment->data(), segmentLength);

"segmentLength" sounds like the length of the segment, but I don't immediately have a better suggestion.

> Source/WebCore/platform/SharedBufferChunkReader.cpp:78
>              auto currentCharacter = m_segment[m_segmentIndex++];
>              if (currentCharacter != m_separator[m_separatorIndex]) {
>                  if (m_separatorIndex > 0) {

The existing code to check for separators doesn't work correctly with arbitrary separator strings. Not sure if it matters. But for example if the buffer contains "aaab" and the separator string is "aab", then it won't detect it as a separator, since at m_segmentIndex = 2, it'll detect a mismatch, and reset m_segmentIndex to 0.

Leave a FIXME in here?

> Source/WebCore/platform/SharedBufferChunkReader.cpp:98
> +        if (++m_iteratorCurrent == m_iteratorEnd) {
>              m_reachedEndOfFile = true;

Maybe set m_segment to nullptr just to be clean in here.

> Source/WebCore/platform/SharedBufferChunkReader.cpp:137
>          if (segmentLength > requestedSize)
>              segmentLength = requestedSize;

Replace this with a std::min, since that's what we do elsewhere?

> Source/WebCore/platform/SharedBufferChunkReader.h:63
> +    const size_t m_bufferSize { 0 };

Can drop the initializer since both constructors initialize this. (And since it's const, the compiler will complain if a constructor stops initializing it.)

> Source/WebCore/platform/SharedBufferChunkReader.h:66
> +    bool m_reachedEndOfFile { false };

I think we can get rid of m_reachedEndOfFile, and check `m_iteratorCurrent != m_iteratorEnd` instead. (In a private helper function, if you like.)

> Tools/TestWebKitAPI/Tests/WebCore/SharedBuffer.cpp:237
> +    const char* const simpleText = "This is a stupid test.";

s/stupid/simple/ if you want to be more positive. :-)
Comment 5 Jean-Yves Avenard [:jya] 2021-11-25 20:09:20 PST
Created attachment 445166 [details]
Patch

Apply comment
Comment 6 EWS 2021-11-25 21:04:36 PST
Committed r286171 (244554@main): <https://commits.webkit.org/244554@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 445166 [details].