Switch to using buffered IO for writing event stream in print IPC

RESOLVED FIXED in Firefox 58

Status

()

enhancement
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Alex_Gaynor, Assigned: Alex_Gaynor)

Tracking

Trunk
Firefox 58
Points:
---

Firefox Tracking Flags

(firefox58 fixed)

Details

Attachments

(1 attachment)

Assignee

Description

2 years ago
Follow up to bug 1409768.

Right now when we write the event steam to a file when we print, it's done using PRFileDesc*. This does not do buffered IO, which is sad, because it means we spend more time doing writes than we should. Example profile: https://perfht.ml/2ySakDn

Relevant code: https://searchfox.org/mozilla-central/source/layout/printing/DrawEventRecorder.h#47-52

Even the most trivial buffering imaginable would probably be a decent win. I'm hoping that somewhere in our codebase is something for buffered file i/o that can be instantiated from an existing file descriptor/HANDLE.
Assignee

Comment 2

2 years ago
Couldn't find an existing IO system that was both buffered and allow instantiation from an FD/HANDLE, so I threw my own simple one together.

It cuts write() time from the profile on macOS from ~50% down to ~15%.
Assignee: nobody → agaynor
Priority: -- → P1
Attachment #8922464 - Flags: review?(mconley) → review?(haftandilian)
Thanks for the patch! Redirecting to haik because I'm a bit overloaded at this time.

Comment 4

2 years ago
mozreview-review
Comment on attachment 8922464 [details]
Bug 1411984 - Use buffered IO in the content process when writing print data for IPC;

https://reviewboard.mozilla.org/r/193540/#review200582

::: layout/printing/DrawEventRecorder.h:17
(Diff revision 1)
>  
>  namespace mozilla {
>  namespace layout {
>  
>  class PRFileDescStream : public mozilla::gfx::EventStream {
> +  static const size_t kBufferSize = 1024;

Are the writes always much lower than 1024? Perhaps add a comment explaining 1024 and that writes larger than the buffer size are not buffered. Or realloc() the buffer if writes are larger than the buffer size.

::: layout/printing/DrawEventRecorder.h:46
(Diff revision 1)
>  
>    void Flush() {
> -    // For std::ostream this flushes any internal buffers. PRFileDesc's IO isn't
> -    // buffered, so nothing to do here.
> +    // We need to be API compatible with std::ostream, and so we silently handle
> +    // flushes on a closed FD.
> +    if (IsOpen() && mBufferPos > 0) {
> +      PR_Write(mFd, static_cast<const void*>(mBuffer.get()), mBufferPos);

We don't check the return value of PR_Write() here or in the existing code. That seems like an existing bug. Instead we could have failed writes set mGood and then update recording code to check good().

::: layout/printing/DrawEventRecorder.h:67
(Diff revision 1)
> +        Flush();
> -      PR_Write(mFd, static_cast<const void*>(aData), aSize);
> +        PR_Write(mFd, static_cast<const void*>(aData), aSize);
> +      // If our write could fit in our buffer, but doesn't because the buffer is
> +      // partially full, write to the buffer, flush the buffer, and then write
> +      // the rest of the data to the buffer.
> +      } else if (aSize > (kBufferSize - mBufferPos)) {

A macro or method for buffer available space would help readability.

::: layout/printing/DrawEventRecorder.h:69
(Diff revision 1)
> +      // If our write could fit in our buffer, but doesn't because the buffer is
> +      // partially full, write to the buffer, flush the buffer, and then write
> +      // the rest of the data to the buffer.
> +      } else if (aSize > (kBufferSize - mBufferPos)) {
> +        size_t length = kBufferSize - mBufferPos;
> +        WriteToBuffer(aData, length);

WriteToBuffer() call not necessary if length = 0 -- when the buffer is completely full.

::: layout/printing/DrawEventRecorder.h:81
(Diff revision 1)
>    void read(char* aOut, size_t aSize) {
>      PRInt32 res = PR_Read(mFd, static_cast<void*>(aOut), aSize);
>      mGood = res >= 0 && ((size_t)res == aSize);
>    }

Does read() also need to take the buffer into account? i.e., so that data written into the buffer and not flushed can be returned with read().

Is it implied that Seek() is always called in between a write() and read()?
Attachment #8922464 - Flags: review?(haftandilian) → review-
Comment hidden (mozreview-request)
Assignee

Comment 6

2 years ago
mozreview-review-reply
Comment on attachment 8922464 [details]
Bug 1411984 - Use buffered IO in the content process when writing print data for IPC;

https://reviewboard.mozilla.org/r/193540/#review200582

> We don't check the return value of PR_Write() here or in the existing code. That seems like an existing bug. Instead we could have failed writes set mGood and then update recording code to check good().

Do you want that in this patch? I don't want to expand the scope of this too much.

> WriteToBuffer() call not necessary if length = 0 -- when the buffer is completely full.

True, but it works fine, so I'm disinclined to care :-)

> Does read() also need to take the buffer into account? i.e., so that data written into the buffer and not flushed can be returned with read().
> 
> Is it implied that Seek() is always called in between a write() and read()?

Currently no users interleave reads and writes, but I added a `Flush()` call so it's always correct.

Comment 7

2 years ago
mozreview-review
Comment on attachment 8922464 [details]
Bug 1411984 - Use buffered IO in the content process when writing print data for IPC;

https://reviewboard.mozilla.org/r/193540/#review200680

r+ with nits.

::: layout/printing/DrawEventRecorder.h:72
(Diff revisions 1 - 2)
>          PR_Write(mFd, static_cast<const void*>(aData), aSize);
>        // If our write could fit in our buffer, but doesn't because the buffer is
>        // partially full, write to the buffer, flush the buffer, and then write
>        // the rest of the data to the buffer.
> -      } else if (aSize > (kBufferSize - mBufferPos)) {
> +      } else if (aSize > AvailableBufferSpace()) {
>          size_t length = kBufferSize - mBufferPos;

Use AvailableBufferSpace() here too.

::: layout/printing/DrawEventRecorder.h:102
(Diff revisions 1 - 2)
> +    return kBufferSize - mBufferPos;
> +  }
> +
>    void WriteToBuffer(const char* aData, size_t aSize) {
>      MOZ_ASSERT(aSize <= kBufferSize);
>      MOZ_ASSERT(aSize <= (kBufferSize - mBufferPos));

AvailableBufferSpace()
Attachment #8922464 - Flags: review?(haftandilian) → review+
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Keywords: checkin-needed

Comment 9

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/8e247550d6cf
Use buffered IO in the content process when writing print data for IPC; r=haik
Keywords: checkin-needed

Comment 10

2 years ago
Backout by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/3568e6d34bf6
Backed out changeset 8e247550d6cf for bustage.
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Keywords: checkin-needed

Comment 12

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/19903553695c
Use buffered IO in the content process when writing print data for IPC; r=haik
Keywords: checkin-needed

Comment 13

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/19903553695c
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
You need to log in before you can comment on or make changes to this bug.