binder: send BC_REPLY and BC_FREE_BUFFER together

In current BR_TRANSACTION handling logic, BC_REPLY and BC_FREE_BUFFER
are sent to kernel seperately which may introduce latency for freeing
buffer.

I think it's better to free buffer asap, this change fixes this
by putting BC_REPLY and BC_FREE_BUFFER in the same buffer. And then
call waitForResponse() to send them to kernel together.

After this, function sendReply() will not be called by anyone. So
sendReply() is also removed.

Below is the test result of "binderThroughputTest -w 100"
Env: android 7.1.2, 2G ram
---------------------
--->  base:
iterations per sec: 31917.7
collecting results
average:2.83375ms worst:572.205ms best:0.054219ms
50%: 2.14844 90%: 5.27344 95%: 6.83594 99%: 12.3047

iterations per sec: 32142.3
collecting results
average:2.8186ms worst:550.884ms best:0.054948ms
50%: 2.53906 90%: 5.27344 95%: 6.83594 99%: 11.1328

iterations per sec: 31704.5
collecting results
average:2.85129ms worst:600.984ms best:0.053854ms
50%: 2.53906 90%: 5.27344 95%: 6.83594 99%: 12.3047

iterations per sec: 31932.3
collecting results
average:2.82549ms worst:563.098ms best:0.052396ms
50%: 2.53906 90%: 5.66406 95%: 6.83594 99%: 11.1328

---> patched:
iterations per sec: 32062.5
collecting results
average:2.85475ms worst:599.177ms best:0.054895ms
50%: 2.14844 90%: 5.27344 95%: 6.44531 99%: 12.6953

iterations per sec: 32241.7
collecting results
average:2.82851ms worst:634.984ms best:0.05651ms
50%: 2.53906 90%: 5.66406 95%: 6.83594 99%: 10.7422

iterations per sec: 31939
collecting results
average:2.86589ms worst:628.861ms best:0.058645ms
50%: 2.53906 90%: 5.66406 95%: 6.83594 99%: 11.1328

iterations per sec: 32149.4
collecting results
average:2.80785ms worst:658.889ms best:0.055573ms
50%: 2.53906 90%: 5.66406 95%: 6.83594 99%: 10.7422

It seems patched is better.

Change-Id: I0ef88864f4dbc63f3cdd2eba345b5dce6140b2d3
Suggested-by: Martijn Coenen <maco@google.com>
Signed-off-by: Ganesh Mahendran <opensource.ganesh@gmail.com>

2 files changed, 62 insertions, 63 deletions

diff --git a/libs/binder/IPCThreadState.cpp b/libs/binder/IPCThreadState.cpp
index ba9bf61f8c..50974ef74f 100644
--- a/libs/binder/IPCThreadState.cpp
+++ b/libs/binder/IPCThreadState.cpp
@@ -718,16 +718,6 @@ IPCThreadState::~IPCThreadState()
 {
 }
 
-status_t IPCThreadState::sendReply(const Parcel& reply, uint32_t flags)
-{
-    status_t err;
-    status_t statusBuffer;
-    err = writeTransactionData(BC_REPLY, flags, -1, 0, reply, &statusBuffer);
-    if (err < NO_ERROR) return err;
-
-    return waitForResponse(NULL, NULL);
-}
-
 status_t IPCThreadState::waitForResponse(Parcel *reply, status_t *acquireResult)
 {
     uint32_t cmd;
@@ -1050,68 +1040,78 @@ status_t IPCThreadState::executeCommand(int32_t cmd)
                 "Not enough command data for brTRANSACTION");
             if (result != NO_ERROR) break;
 
-            Parcel buffer;
-            buffer.ipcSetDataReference(
-                reinterpret_cast<const uint8_t*>(tr.data.ptr.buffer),
-                tr.data_size,
-                reinterpret_cast<const binder_size_t*>(tr.data.ptr.offsets),
-                tr.offsets_size/sizeof(binder_size_t), freeBuffer, this);
+            Parcel reply;
+            {
+                Parcel buffer;
+                const pid_t origPid = mCallingPid;
+                const uid_t origUid = mCallingUid;
+                const int32_t origStrictModePolicy = mStrictModePolicy;
+                const int32_t origTransactionBinderFlags = mLastTransactionBinderFlags;
+
+                buffer.ipcSetDataReference(
+                    reinterpret_cast<const uint8_t*>(tr.data.ptr.buffer),
+                    tr.data_size,
+                    reinterpret_cast<const binder_size_t*>(tr.data.ptr.offsets),
+                    tr.offsets_size/sizeof(binder_size_t), freeBuffer, this);
+
+                mCallingPid = tr.sender_pid;
+                mCallingUid = tr.sender_euid;
+                mLastTransactionBinderFlags = tr.flags;
+
+                //ALOGI(">>>> TRANSACT from pid %d uid %d\n", mCallingPid, mCallingUid);
+
+                status_t error;
+                IF_LOG_TRANSACTIONS() {
+                    TextOutput::Bundle _b(alog);
+                    alog << "BR_TRANSACTION thr " << (void*)pthread_self()
+                        << " / obj " << tr.target.ptr << " / code "
+                        << TypeCode(tr.code) << ": " << indent << buffer
+                        << dedent << endl
+                        << "Data addr = "
+                        << reinterpret_cast<const uint8_t*>(tr.data.ptr.buffer)
+                        << ", offsets addr="
+                        << reinterpret_cast<const size_t*>(tr.data.ptr.offsets) << endl;
+                }
+                if (tr.target.ptr) {
+                    // We only have a weak reference on the target object, so we must first try to
+                    // safely acquire a strong reference before doing anything else with it.
+                    if (reinterpret_cast<RefBase::weakref_type*>(
+                            tr.target.ptr)->attemptIncStrong(this)) {
+                        error = reinterpret_cast<BBinder*>(tr.cookie)->transact(tr.code, buffer,
+                                &reply, tr.flags);
+                        reinterpret_cast<BBinder*>(tr.cookie)->decStrong(this);
+                    } else {
+                        error = UNKNOWN_TRANSACTION;
+                    }
 
-            const pid_t origPid = mCallingPid;
-            const uid_t origUid = mCallingUid;
-            const int32_t origStrictModePolicy = mStrictModePolicy;
-            const int32_t origTransactionBinderFlags = mLastTransactionBinderFlags;
+                } else {
+                    error = the_context_object->transact(tr.code, buffer, &reply, tr.flags);
+                }
 
-            mCallingPid = tr.sender_pid;
-            mCallingUid = tr.sender_euid;
-            mLastTransactionBinderFlags = tr.flags;
+                //ALOGI("<<<< TRANSACT from pid %d restore pid %d uid %d\n",
+                //     mCallingPid, origPid, origUid);
 
-            //ALOGI(">>>> TRANSACT from pid %d uid %d\n", mCallingPid, mCallingUid);
+                if ((tr.flags & TF_ONE_WAY) == 0) {
+                    LOG_ONEWAY("Sending reply to %d!", mCallingPid);
+                    if (error < NO_ERROR) reply.setError(error);
 
-            Parcel reply;
-            status_t error;
-            IF_LOG_TRANSACTIONS() {
-                TextOutput::Bundle _b(alog);
-                alog << "BR_TRANSACTION thr " << (void*)pthread_self()
-                    << " / obj " << tr.target.ptr << " / code "
-                    << TypeCode(tr.code) << ": " << indent << buffer
-                    << dedent << endl
-                    << "Data addr = "
-                    << reinterpret_cast<const uint8_t*>(tr.data.ptr.buffer)
-                    << ", offsets addr="
-                    << reinterpret_cast<const size_t*>(tr.data.ptr.offsets) << endl;
-            }
-            if (tr.target.ptr) {
-                // We only have a weak reference on the target object, so we must first try to
-                // safely acquire a strong reference before doing anything else with it.
-                if (reinterpret_cast<RefBase::weakref_type*>(
-                        tr.target.ptr)->attemptIncStrong(this)) {
-                    error = reinterpret_cast<BBinder*>(tr.cookie)->transact(tr.code, buffer,
-                            &reply, tr.flags);
-                    reinterpret_cast<BBinder*>(tr.cookie)->decStrong(this);
+                    status_t statusBuffer;
+                    result = writeTransactionData(BC_REPLY, 0, -1, 0, reply, &statusBuffer);
                 } else {
-                    error = UNKNOWN_TRANSACTION;
+                    LOG_ONEWAY("NOT sending reply to %d!", mCallingPid);
                 }
 
-            } else {
-                error = the_context_object->transact(tr.code, buffer, &reply, tr.flags);
+                mCallingPid = origPid;
+                mCallingUid = origUid;
+                mStrictModePolicy = origStrictModePolicy;
+                mLastTransactionBinderFlags = origTransactionBinderFlags;
             }
 
-            //ALOGI("<<<< TRANSACT from pid %d restore pid %d uid %d\n",
-            //     mCallingPid, origPid, origUid);
-
-            if ((tr.flags & TF_ONE_WAY) == 0) {
-                LOG_ONEWAY("Sending reply to %d!", mCallingPid);
-                if (error < NO_ERROR) reply.setError(error);
-                sendReply(reply, 0);
-            } else {
-                LOG_ONEWAY("NOT sending reply to %d!", mCallingPid);
-            }
+            if (result != NO_ERROR)
+                break;
 
-            mCallingPid = origPid;
-            mCallingUid = origUid;
-            mStrictModePolicy = origStrictModePolicy;
-            mLastTransactionBinderFlags = origTransactionBinderFlags;
+            if ((tr.flags & TF_ONE_WAY) == 0)
+                waitForResponse(NULL, NULL);
 
             IF_LOG_TRANSACTIONS() {
                 TextOutput::Bundle _b(alog);
diff --git a/libs/binder/include/binder/IPCThreadState.h b/libs/binder/include/binder/IPCThreadState.h
index 245607e74e..9fb13bbe95 100644
--- a/libs/binder/include/binder/IPCThreadState.h
+++ b/libs/binder/include/binder/IPCThreadState.h
@@ -93,7 +93,6 @@ private:
                                 IPCThreadState();
                                 ~IPCThreadState();
 
-            status_t            sendReply(const Parcel& reply, uint32_t flags);
             status_t            waitForResponse(Parcel *reply,
                                                 status_t *acquireResult=NULL);
             status_t            talkWithDriver(bool doReceive=true);