Skip to content

Conversation

beicause
Copy link
Contributor

@beicause beicause commented Sep 24, 2025

This simplifies things and improves performance of ClassId::to_cow_str, which is overhead in CallContext::gd and check_rtti.

#[bench(repeat = 10000)]
fn class_id_str() -> std::borrow::Cow<'static, str> {
    godot::classes::Object::class_id().to_cow_str()
}

// In debug build
// before:
//    -- class_id_str               ...      0.148μs      0.156μs
// after:
//    -- class_id_str               ...      0.119μs      0.125μs

I think the only reason for using cstr might be initializing StringName from it is slightly faster than rust str, but this is not a hot path since initialization only happens once.

@GodotRust
Copy link

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-1334

@Bromeon Bromeon added c: core Core components performance Performance problems and optimizations labels Sep 24, 2025
@Bromeon Bromeon added this to the 0.4.x milestone Sep 24, 2025
Copy link
Member

@Bromeon Bromeon left a comment

Choose a reason for hiding this comment

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

Thank you, nice simplification! 🙂

@beicause beicause force-pushed the class-id-source-rust-str branch from 110ff74 to 71fd5c7 Compare September 25, 2025 07:45
@beicause beicause force-pushed the class-id-source-rust-str branch 2 times, most recently from e8ca64a to 0d57a89 Compare September 25, 2025 08:25
@beicause
Copy link
Contributor Author

Clippy CI failed strangely, but I can't reproduce it locally.

@Yarwin
Copy link
Contributor

Yarwin commented Sep 25, 2025

it is because the file in question is formatted differently on CI (it is oooneee big line). Why? No idea (details of macro formatting are black magic to me, sorry 🤷).

Code in question as it is formatted on mine and beicause's sides
    #[func]
    fn check_last_notrace(last_method_name: String, expected_callconv: String) -> bool {
        let last = godot::private::trace::pop();
        let mut ok = true;
        if last.class != "GenFfi" {
            godot_error!("Expected class GenFfi, got {}", last.class);
            ok = false;
        }
        if last.method != last_method_name {
            godot_error!("Expected method {}, got {}", last_method_name, last.method);
            ok = false;
        }
        if !last.is_inbound {
            godot_error!("Expected inbound call, got outbound");
            ok = false;
        }
        let expect_ptrcall = expected_callconv == "ptrcall";
        if last.is_ptrcall != expect_ptrcall {
            let actual = Self::to_string(last.is_ptrcall);
            godot_error!("Expected {expected_callconv}, got {actual}");
            ok = false;
        }
        ok
    }

To elaborate, this code:

fn check(a: bool, b: bool, c: bool) -> bool {
    let mut godot_error = false;
    if a {
        println!("a");
        godot_error = true;
    }
    if b {
        println!("b");
        godot_error = true;
    }
    if c {
        println!("c");
        godot_error = true;
    }
    godot_error
}

is fine and triggers no clippy warnings, meanwhile

fn check(a: bool, b: bool, c: bool) -> bool {
    let mut godot_error = false;
    if a {
        println!("a");
        godot_error = true;
    } if b {
        println!("b");
        godot_error = true;
    }
    if c {
        println!("c");
        godot_error = true;
    }
    godot_error
}

triggers following warning:

    Checking playground v0.0.1 (/playground)
warning: this looks like an `else if` but the `else` is missing
 --> src/main.rs:6:6
  |
6 |     } if b {
  |      ^
  |
  = note: to remove this lint, add the missing `else` or add a new line before the second `if`
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#suspicious_else_formatting
  = note: `#[warn(clippy::suspicious_else_formatting)]` on by default

@Yarwin
Copy link
Contributor

Yarwin commented Sep 25, 2025

EDIT: I'll address it, in this case simple #[allow(...)] will be fine

@Bromeon Bromeon force-pushed the class-id-source-rust-str branch from 0d57a89 to 82feab1 Compare September 27, 2025 09:05
Replace `ClassIdSource` with `Cow<'static, str>`
@Bromeon Bromeon force-pushed the class-id-source-rust-str branch from 82feab1 to 63b942e Compare September 27, 2025 18:25
@Bromeon Bromeon added this pull request to the merge queue Oct 8, 2025
Merged via the queue into godot-rust:master with commit 4a58210 Oct 8, 2025
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: core Core components performance Performance problems and optimizations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants