mozilla / uniffi-rs

a multi-language bindings generator for rust
https://mozilla.github.io/uniffi-rs/
Mozilla Public License 2.0
2.68k stars 219 forks source link

Ability to completely remove logging code (likely with a feature flag) #2224

Open timboudreau opened 2 weeks ago

timboudreau commented 2 weeks ago

The Problem

I am using uniffi with some code that does DSP - called from C, Objective C and Swift, and using the os_log crate to integrate the log crate, which uniffi's generated code uses, with Mac OS's system-level logging.

The code uniffi generates logs every inbound call into Rust - due to the lines visible in this diff

That might seem reasonable, assuming logging is zero-cost when not enabled (it is not always, and it is not with os_log), and assuming calls across the FFI boundary are infrequent (not a safe assumption at all).

Imagine you have some legitimate, useful debug-level logging you need to see happening in your Rust code you are calling.

Now imagine the FFI wrapper code you generated is also doing debug-level logging ... of a call that is happening 768,000 times per second (8 frequency bands x 96,000 samples per second).

Good luck reading that log.

Not to mention, there is measurable impact on performance of doing anything "near-zero cost" ... if you do it 768,000 times a second.

The Solution

It seems to me that unasked for, unremovable without forking logging is a bug, not a feature. I can totally understand wanting logging of FFI calls sometimes. The logging code should, IMHO, be off-by-default but enablable with a feature flag.

Or, if that is likely to break existing code that expects it, at the very least, able to be completely excised with a feature flag.

badboy commented 2 weeks ago

These are about to become trace level, so at least some of the issues you report become less of a problem. As a workaround right now you could add a filter. We do that over in Glean as well. This should even avoid the overhead of calling into the underlying log implementation, because filtering is done before that.

mhammond commented 2 weeks ago

While @badboy has some great workarounds, I don't see any reason we wouldn't take a patch for a feature here. That might also dovetail into a future where people start asking how to use tracing or similar instead of log