Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow for compiler optimizations and logging configuration #300

Merged
merged 4 commits into from
Feb 18, 2025

Conversation

rustaceanrob
Copy link
Owner

@rustaceanrob rustaceanrob commented Feb 16, 2025

Closes #298

The format! macro requires a heap allocation, and the &str references are converted to String when sent as a Log::Dialog message. Because some applications might be more memory hungry than others, there should be a way to stop the node from allocating these String entirely. I thought about having this as a crate feature, but the language bindings used by BDK would not be able to make use of this performance gain, since they ship a binary. I think I strike the best of both worlds here and use a runtime variable that can still be optimized away by the compiler.

To prove this works, I made a simple example. Check out this in the Rust playground and select "ASM" using the three dots on the top left.

macro_rules! conditional_run {
    ($foo:expr, $expr:expr) => {
        match $foo.condition {
            Condition::Run => { $foo.do_foo($expr); }
            Condition::Skip => {} 
        }
    };
}

enum Condition {
    Run,
    Skip,
}

struct Foo {
    condition: Condition
}

impl Foo {
    fn do_foo(&self, message: String) {
        println!("{message}");
    }
}

fn main() {
    let condition = Condition::Skip;
    let foo = Foo { condition };
    conditional_run!(foo, format!("Allocated String!")); 
    println!("No allocation occurred!");
}

The compiler can remove the branching and either completely ignore the expression, thus not allocating the String, or execute the expression and send a debug message. Only downside to this approach is the Log::Dialog is still a member of Log even though we don't need it. Not sure how to get rid of it without introducing compile-time options though.

@rustaceanrob rustaceanrob force-pushed the log-2-15 branch 2 times, most recently from 307c3e2 to 07abf7e Compare February 16, 2025 19:31
@rustaceanrob
Copy link
Owner Author

While I was at it I changed Log::Dialog to a more reflective name in Log::Debug. I think this combined with LogLevel will be clear which message type is being omitted.

@nyonson
Copy link
Contributor

nyonson commented Feb 18, 2025

OK just runnin it back to make sure I get it, you are adding a conditional here based on a flag so the compiler can be smart and just toss out branches that will never be executed. Without this, things are always getting allocated on the heap even if the client wasn't running debug mode?

I think my one big question is how does the compiler optimize a runtime flag? In your playground example (btw pretty dope that godbolt asm stuff is just built right in to rust playground) won't the compiler pick up on the condition cause it is in fact available at compile time (aka hardcoded)?

@rustaceanrob
Copy link
Owner Author

rustaceanrob commented Feb 18, 2025

Right, the compiler is smart enough to remove branches that will not execute. Even though the log level is set at runtime, it is known for the entire duration of Node::run, so the compiler can still remove unused branches. To prove this to yourself you can checkout the branch and slap dbg! around any of the format! statements to see that it gets executed with LogLevel::Debug and does not execute with LogLevel::Warning.

This may already be common knowledge but I checkout from a detached head so there's no local branch hanging around. In this case:

git fetch upstream pull/300/head && git checkout FETCH_HEAD

@rustaceanrob
Copy link
Owner Author

Also just realized I normally compile all examples in release, so might need to slap a --release for the compiler to do it's magic.

Copy link
Contributor

@nyonson nyonson left a comment

Choose a reason for hiding this comment

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

ACK 8af5be1

Was a little thrown by the optimizations, but makes sense that a consistent conditional guard will protect against unnecessary log allocations.

@rustaceanrob rustaceanrob merged commit 12249e0 into master Feb 18, 2025
14 checks passed
@rustaceanrob rustaceanrob deleted the log-2-15 branch February 18, 2025 06:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Logging requires heap allocations
2 participants