Skip to content

Fix memory leak when freeing a bindless extended material by fixing inverted binary search condition - #25848

Merged
alice-i-cecile merged 1 commit into
bevyengine:mainfrom
ssbr:bsearch
Sep 20, 2026
Merged

alice-i-cecile merged 1 commit into
bevyengine:mainfrom
ssbr:bsearch

Conversation

@ssbr

@ssbr ssbr commented Sep 19, 2026 •

Copy link
Copy Markdown
Contributor

Objective

Fix a memory leak when freeing a bindless extended material.

The old code used a binary search, as follows:

self
    .bindless_index_tables
    .binary_search_by(|bindless_index_table| {
        if bindless_index < bindless_index_table.index_range.start {
            Ordering::Less
        } else if bindless_index >= bindless_index_table.index_range.end {
            Ordering::Greater
        } else {
            Ordering::Equal
        }
    })

The problem is that binary_search_by is supposed to go the other way around:

The comparator function should return an order code that indicates whether its argument is Less, Equal or Greater the desired target.

Emphasis added. The code above returned whether the target (bindless_index) was less.

This led to the function failing to find anything if there was more than one bindless binding, which caused lookup to fail in free, and so it was skipped instead of freed.

// in MaterialBindlessSlab::free
let Some(bindless_index_table) = self.get_bindless_index_table(bindless_index) else {
    continue;
};

Solution

Swap the comparisons!

Testing

Two tests!

First: I added unit tests for the binary search. It did not have any before.

Secondly... I was actually paranoid that while all the docs said it was "sorted", and "sorted" conventionally means "sorted ascending" (as in [T]::is_sorted), maybe it idiosyncratically meant "sorted descending". So I manually tested that by just asserting that it was ascending at runtime when running the extended_material_bindless example.

// in MaterialBindlessSlab::new
if bindless_index_tables.len() > 1 {
    assert!(bindless_index_tables.is_sorted_by_key(|b| b.index_range.start));
}

This assertion failed if I negated it, so it really is sorted ascending, and the lookup really was backwards.


AI Diclosure

Both the code that originally triggered the bug, and the identification of the issue, came from an LLM. Which is pretty helpful, because debugging a GPU OOM sounds difficult. After that, I took over and the rest is human. I may not be a GPU expert, but I have taken CS 101 and I can understand what a broken binary search looks like. Based on the documented properties of the code, the original was broken, and I did my best to verify that the documentation matched reality. I believe I fully understand both the issue and the fix.

@ssbr ssbr changed the title Fix inverted binary search condition. ooooops Fix inverted binary search condition Sep 19, 2026
@ssbr

ssbr commented Sep 19, 2026

Copy link
Copy Markdown
Contributor Author

Oops, it stole the commit message for the PR title, which wasn't quite the right uh mood.

@Zeophlite Zeophlite added C-Bug An unexpected or incorrect behavior A-Rendering Drawing game state to the screen D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Sep 19, 2026
@github-project-automation github-project-automation Bot moved this to Needs SME Triage in Rendering Sep 19, 2026
@alice-i-cecile alice-i-cecile added this to the 0.20 milestone Sep 20, 2026
@alice-i-cecile alice-i-cecile changed the title Fix inverted binary search condition Fix memory leak when freeing a bindless extended material by fixing inverted binary search condition Sep 20, 2026
@alice-i-cecile alice-i-cecile added X-Uncontroversial This work is generally agreed upon S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Sep 20, 2026

@alice-i-cecile alice-i-cecile left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've spent a few minutes reviewing the surrounding code and understanding how this works, and had Sonnet 5 review for algorithmic correctness too since stupid mistakes here are easy. All good!

@alice-i-cecile
alice-i-cecile added this pull request to the merge queue Sep 20, 2026
Merged via the queue into bevyengine:main with commit 70c2b02 Sep 20, 2026
70 of 76 checks passed
@github-project-automation github-project-automation Bot moved this from Needs SME Triage to Done in Rendering Sep 20, 2026
gregcsokas pushed a commit to gregcsokas/bevy that referenced this pull request Sep 21, 2026
…nverted binary search condition (bevyengine#25848)

# Objective

Fix a memory leak when freeing a bindless extended material.

The old code used a binary search, as follows:

```rust
self
    .bindless_index_tables
    .binary_search_by(|bindless_index_table| {
        if bindless_index < bindless_index_table.index_range.start {
            Ordering::Less
        } else if bindless_index >= bindless_index_table.index_range.end {
            Ordering::Greater
        } else {
            Ordering::Equal
        }
    })
```

The problem is that [`binary_search_by` is supposed to go the other way
around](https://doc.rust-lang.org/std/vec/struct.Vec.html#method.binary_search_by):

> The comparator function should return an order code that indicates
whether **its argument** is `Less`, `Equal` or `Greater` the desired
target.

Emphasis added. The code above returned whether the _target_
(`bindless_index`) was less.

This led to the function failing to find anything if there was more than
one bindless binding, which caused lookup to fail in `free`, and so it
was skipped instead of freed.

```rust
// in MaterialBindlessSlab::free
let Some(bindless_index_table) = self.get_bindless_index_table(bindless_index) else {
    continue;
};
```

## Solution

Swap the comparisons!

## Testing

Two tests!

First: I added unit tests for the binary search. It did not have any
before.

Secondly... I was actually paranoid that while all the docs said it was
"sorted", and "sorted" conventionally means "sorted ascending" (as in
`[T]::is_sorted`), maybe it idiosyncratically meant "sorted descending".
So I manually tested that by just asserting that it was ascending at
runtime when running the `extended_material_bindless` example.

```rust
// in MaterialBindlessSlab::new
if bindless_index_tables.len() > 1 {
    assert!(bindless_index_tables.is_sorted_by_key(|b| b.index_range.start));
}
```

This assertion failed if I negated it, so it really is sorted ascending,
and the lookup really was backwards.

---

## AI Diclosure

Both the code that originally triggered the bug, and the identification
of the issue, came from an LLM. Which is pretty helpful, because
debugging a GPU OOM sounds difficult. After that, I took over and the
rest is human. I may not be a GPU expert, but I have taken CS 101 and I
can understand what a broken binary search looks like. Based on the
documented properties of the code, the original was broken, and I did my
best to verify that the documentation matched reality. I believe I fully
understand both the issue and the fix.
@ssbr
ssbr deleted the bsearch branch September 21, 2026 05:57
mockersf pushed a commit that referenced this pull request Sep 25, 2026
…nverted binary search condition (#25848)

# Objective

Fix a memory leak when freeing a bindless extended material.

The old code used a binary search, as follows:

```rust
self
    .bindless_index_tables
    .binary_search_by(|bindless_index_table| {
        if bindless_index < bindless_index_table.index_range.start {
            Ordering::Less
        } else if bindless_index >= bindless_index_table.index_range.end {
            Ordering::Greater
        } else {
            Ordering::Equal
        }
    })
```

The problem is that [`binary_search_by` is supposed to go the other way
around](https://doc.rust-lang.org/std/vec/struct.Vec.html#method.binary_search_by):

> The comparator function should return an order code that indicates
whether **its argument** is `Less`, `Equal` or `Greater` the desired
target.

Emphasis added. The code above returned whether the _target_
(`bindless_index`) was less.

This led to the function failing to find anything if there was more than
one bindless binding, which caused lookup to fail in `free`, and so it
was skipped instead of freed.

```rust
// in MaterialBindlessSlab::free
let Some(bindless_index_table) = self.get_bindless_index_table(bindless_index) else {
    continue;
};
```

## Solution

Swap the comparisons!

## Testing

Two tests!

First: I added unit tests for the binary search. It did not have any
before.

Secondly... I was actually paranoid that while all the docs said it was
"sorted", and "sorted" conventionally means "sorted ascending" (as in
`[T]::is_sorted`), maybe it idiosyncratically meant "sorted descending".
So I manually tested that by just asserting that it was ascending at
runtime when running the `extended_material_bindless` example.

```rust
// in MaterialBindlessSlab::new
if bindless_index_tables.len() > 1 {
    assert!(bindless_index_tables.is_sorted_by_key(|b| b.index_range.start));
}
```

This assertion failed if I negated it, so it really is sorted ascending,
and the lookup really was backwards.

---

## AI Diclosure

Both the code that originally triggered the bug, and the identification
of the issue, came from an LLM. Which is pretty helpful, because
debugging a GPU OOM sounds difficult. After that, I took over and the
rest is human. I may not be a GPU expert, but I have taken CS 101 and I
can understand what a broken binary search looks like. Based on the
documented properties of the code, the original was broken, and I did my
best to verify that the documentation matched reality. I believe I fully
understand both the issue and the fix.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-Rendering Drawing game state to the screen C-Bug An unexpected or incorrect behavior D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Uncontroversial This work is generally agreed upon

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants