[PATCH] rust_binder: move BC_FREE_BUFFER drop inside if statement

[PATCH] rust_binder: move BC_FREE_BUFFER drop inside if statement

[Alice Ryhl]

When looking at flamegraphs, there is a pretty large entry for the
function call drop_in_place::<Option<Allocation>> which in turn calls
drop_in_place::<Allocation>. Combined with the looper_need_return
condition, this means that the generated code looks like this:

	if let Some(buffer) = buffer {
	    if buffer.looper_need_return_on_free() {
	        self.inner.lock().looper_need_return = true;
	    }
	}
	drop_in_place::<Option<Allocation>>() { // not inlined
	    if let Some(buffer) = buffer {
	    	drop_in_place::<Allocation>(buffer);
	    }
	}

This kind of situation where you check X and then check X again is
normally optimized into a single condition, but in this case due to the
non-inlined function call to drop_in_place::<Option<Allocation>>, that
optimization does not happen.

Furthermore, the drop_in_place::<Allocation> call is only two-thirds of
the drop_in_place::<Option<Allocation>> call in the flamegraph. This
indicates that this double condition is not performing well. Also, last
time I looked at Binder perf, I remember finding that the destructor of
Allocation was involved with many branch mispredictions.

Thus, change this code to look like this:

	if let Some(buffer) = buffer {
	    if buffer.looper_need_return_on_free() {
	        self.inner.lock().looper_need_return = true;
	    }
	    drop_in_place::<Allocation>(buffer);
	}

by dropping the Allocation directly. Flamegraphs confirm that the
drop_in_place::<Option<Allocation>> call disappears from this change.

Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
 drivers/android/binder/thread.rs | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/android/binder/thread.rs b/drivers/android/binder/thread.rs
index 7e34ccd394f8049bab88562ffb4601739aea670a..1a8e6fdc0dc42369ee078e720aa02b2554fb7332 100644
--- a/drivers/android/binder/thread.rs
+++ b/drivers/android/binder/thread.rs
@@ -1323,12 +1323,12 @@ fn write(self: &Arc<Self>, req: &mut BinderWriteRead) -> Result {
                 }
                 BC_FREE_BUFFER => {
                     let buffer = self.process.buffer_get(reader.read()?);
-                    if let Some(buffer) = &buffer {
+                    if let Some(buffer) = buffer {
                         if buffer.looper_need_return_on_free() {
                             self.inner.lock().looper_need_return = true;
                         }
+                        drop(buffer);
                     }
-                    drop(buffer);
                 }
                 BC_INCREFS => {
                     self.process

---
base-commit: 211ddde0823f1442e4ad052a2f30f050145ccada
change-id: 20251029-binder-bcfreebuf-option-35276027ce11

Best regards,
-- 
Alice Ryhl <aliceryhl@google.com>

Re: [PATCH] rust_binder: move BC_FREE_BUFFER drop inside if statement

[Carlos Llamas]

On Wed, Oct 29, 2025 at 11:50:58AM +0000, Alice Ryhl wrote:
> When looking at flamegraphs, there is a pretty large entry for the
> function call drop_in_place::<Option<Allocation>> which in turn calls
> drop_in_place::<Allocation>. Combined with the looper_need_return
> condition, this means that the generated code looks like this:
> 
> 	if let Some(buffer) = buffer {
> 	    if buffer.looper_need_return_on_free() {
> 	        self.inner.lock().looper_need_return = true;
> 	    }
> 	}
> 	drop_in_place::<Option<Allocation>>() { // not inlined
> 	    if let Some(buffer) = buffer {
> 	    	drop_in_place::<Allocation>(buffer);
> 	    }
> 	}
> 
> This kind of situation where you check X and then check X again is
> normally optimized into a single condition, but in this case due to the
> non-inlined function call to drop_in_place::<Option<Allocation>>, that
> optimization does not happen.
> 
> Furthermore, the drop_in_place::<Allocation> call is only two-thirds of
> the drop_in_place::<Option<Allocation>> call in the flamegraph. This
> indicates that this double condition is not performing well. Also, last
> time I looked at Binder perf, I remember finding that the destructor of
> Allocation was involved with many branch mispredictions.
> 
> Thus, change this code to look like this:
> 
> 	if let Some(buffer) = buffer {
> 	    if buffer.looper_need_return_on_free() {
> 	        self.inner.lock().looper_need_return = true;
> 	    }
> 	    drop_in_place::<Allocation>(buffer);
> 	}
> 
> by dropping the Allocation directly. Flamegraphs confirm that the
> drop_in_place::<Option<Allocation>> call disappears from this change.
> 
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> ---

LGTM,

Acked-by: Carlos Llamas <cmllamas@google.com>