From 87003ad3f6ff0928c0f5f5dcf0024450b0ee18cb Mon Sep 17 00:00:00 2001
From: Mike Ma <yanmin@google.com>
Date: Thu, 19 Mar 2020 12:31:29 -0700
Subject: [PATCH] TextDumpsysSection memory optimization

TextDumpsysSection used to inherent WorkerThreadSection, which
allocates 2x more memory than the dumped content. This change saves
the extra allocation by writing dumpsys content directly to FdBuffer.

Bug: 150311553
Test: Manually run "incident -p EXPLICIT 4000"
Change-Id: I9c0c0db75c8595822ee0711040e8865dd69378b6
---
 cmds/incidentd/src/Section.cpp | 54 ++++++++++++++++++++--------------
 cmds/incidentd/src/Section.h   |  4 +--
 2 files changed, 34 insertions(+), 24 deletions(-)

diff --git a/cmds/incidentd/src/Section.cpp b/cmds/incidentd/src/Section.cpp
index 60d1ac74f657c..c703c3ce09513 100644
--- a/cmds/incidentd/src/Section.cpp
+++ b/cmds/incidentd/src/Section.cpp
@@ -263,10 +263,6 @@ void sigpipe_handler(int signum) {
 }
 
 status_t WorkerThreadSection::Execute(ReportWriter* writer) const {
-    status_t err = NO_ERROR;
-    bool workerDone = false;
-    FdBuffer buffer;
-
     // Create shared data and pipe. Don't put data on the stack since this thread may exit early.
     sp<WorkerThreadData> data = new WorkerThreadData(this);
     if (!data->pipe.init()) {
@@ -289,6 +285,9 @@ status_t WorkerThreadSection::Execute(ReportWriter* writer) const {
     }).detach();
 
     // Loop reading until either the timeout or the worker side is done (i.e. eof).
+    status_t err = NO_ERROR;
+    bool workerDone = false;
+    FdBuffer buffer;
     err = buffer.read(data->pipe.readFd().get(), this->timeoutMs);
     if (err != NO_ERROR) {
         ALOGE("[%s] reader failed with error '%s'", this->name.string(), strerror(-err));
@@ -440,7 +439,7 @@ status_t DumpsysSection::BlockingCall(unique_fd& pipeWriteFd) const {
 
 // ================================================================================
 TextDumpsysSection::TextDumpsysSection(int id, const char* service, ...)
-    : WorkerThreadSection(id, REMOTE_CALL_TIMEOUT_MS), mService(service) {
+        :Section(id), mService(service) {
     name = "dumpsys ";
     name += service;
 
@@ -460,7 +459,7 @@ TextDumpsysSection::TextDumpsysSection(int id, const char* service, ...)
 
 TextDumpsysSection::~TextDumpsysSection() {}
 
-status_t TextDumpsysSection::BlockingCall(unique_fd& pipeWriteFd) const {
+status_t TextDumpsysSection::Execute(ReportWriter* writer) const {
     // checkService won't wait for the service to show up like getService will.
     sp<IBinder> service = defaultServiceManager()->checkService(mService);
     if (service == NULL) {
@@ -488,25 +487,36 @@ status_t TextDumpsysSection::BlockingCall(unique_fd& pipeWriteFd) const {
     });
 
     // Collect dump content
-    std::string content;
-    bool success = ReadFdToString(dumpPipe.readFd(), &content);
-    worker.join(); // Wait for worker to finish
-    dumpPipe.readFd().reset();
-    if (!success) {
-        ALOGW("[%s] failed to read data from pipe", this->name.string());
-        return -1;
-    }
-
+    FdBuffer buffer;
     ProtoOutputStream proto;
-    proto.write(util::TextDumpProto::COMMAND, std::string(name.string()));
-    proto.write(util::TextDumpProto::CONTENT, content);
-    proto.write(util::TextDumpProto::DUMP_DURATION_NS, int64_t(Nanotime() - start));
+    proto.write(TextDumpProto::COMMAND, std::string(name.string()));
+    proto.write(TextDumpProto::DUMP_DURATION_NS, int64_t(Nanotime() - start));
+    buffer.write(proto.data());
 
-    if (!proto.flush(pipeWriteFd.get()) && errno == EPIPE) {
-        ALOGE("[%s] wrote to a broken pipe\n", this->name.string());
-        return EPIPE;
+    sp<EncodedBuffer> internalBuffer = buffer.data();
+    internalBuffer->writeHeader((uint32_t)TextDumpProto::CONTENT, WIRE_TYPE_LENGTH_DELIMITED);
+    size_t editPos = internalBuffer->wp()->pos();
+    internalBuffer->wp()->move(8); // reserve 8 bytes for the varint of the data size
+    size_t dataBeginPos = internalBuffer->wp()->pos();
+
+    status_t readStatus = buffer.read(dumpPipe.readFd(), this->timeoutMs);
+    dumpPipe.readFd().reset();
+    writer->setSectionStats(buffer);
+    if (readStatus != OK || buffer.timedOut()) {
+        ALOGW("[%s] failed to read from dumpsys: %s, timedout: %s", this->name.string(),
+              strerror(-readStatus), buffer.timedOut() ? "true" : "false");
+        worker.detach();
+        return readStatus;
     }
-    return OK;
+    worker.join(); // wait for worker to finish
+
+    // Revisit the actual size from dumpsys and edit the internal buffer accordingly.
+    size_t dumpSize = buffer.size() - dataBeginPos;
+    internalBuffer->wp()->rewind()->move(editPos);
+    internalBuffer->writeRawVarint32(dumpSize);
+    internalBuffer->copy(dataBeginPos, dumpSize);
+
+    return writer->writeSection(buffer);
 }
 
 // ================================================================================
diff --git a/cmds/incidentd/src/Section.h b/cmds/incidentd/src/Section.h
index 6162b3ae2ceb1..2ce45ed66a327 100644
--- a/cmds/incidentd/src/Section.h
+++ b/cmds/incidentd/src/Section.h
@@ -130,12 +130,12 @@ private:
 /**
  * Section that calls text dumpsys on a system service, usually "dumpsys [service_name]".
  */
-class TextDumpsysSection : public WorkerThreadSection {
+class TextDumpsysSection : public Section {
 public:
     TextDumpsysSection(int id, const char* service, ...);
     virtual ~TextDumpsysSection();
 
-    virtual status_t BlockingCall(unique_fd& pipeWriteFd) const;
+    virtual status_t Execute(ReportWriter* writer) const;
 
 private:
     String16 mService;