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

Make log crate dependency and logging an optional feature #2227

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

timboudreau
Copy link

Addresses #2224 - even disabled logging can have overhead depending with some backends, and in very high frequency calls, generates unacceptable overhead and log-spam.

Those tests that can run without kotlinc installed pass.

I cannot verify that it is problem-free until I backport it to 0.26.1 as using 0.28.1 with my code fails at build time, with or without the patch, as follows:

thread 'main' panicked at /Users/tim/.cargo/git/checkouts/uniffi-rs-cc388e82c08ac837/ddc23a3/uniffi/src/lib.rs:30:21:
called `Result::unwrap()` on an `Err` value: Failed to extract data from archive member `curve.curve.e27b66861dd0975a-cgu.0.rcgu.o`

Caused by:
    Constructor return type must be Arc<Self>

Note this patch entirely removes the log dependency - both call logging and panic logging. If that seems like overkill, it could probably be split into two flags, one to remove logging entirely, and one just to remove FFI-call logging.

Addresses mozilla#2224 - even disabled logging can have overhead depending with some backends, and
in very high frequency calls, generates unacceptable overhead and log-spam.
@timboudreau timboudreau requested a review from a team as a code owner August 30, 2024 00:08
@timboudreau timboudreau requested review from mhammond and removed request for a team August 30, 2024 00:08
@bendk
Copy link
Contributor

bendk commented Aug 30, 2024

Completely unrelated to this patch, but I believe your build issue is caused by mixing UniFFI versions. That error message was in 0.26.x and 0.27.x, but removed in 0.28.x. My guess is that you're using 0.26.x to generate your bindings.

@bendk
Copy link
Contributor

bendk commented Aug 30, 2024

This feels similar in spirit to my PR to add the ffi-trace feature #2211. What do others think about putting these logging statements behind that feature flag?

In general, my feeling is that these kind of logging statements are only really useful for debugging failures when writing scaffolding/bindings code. There's not a lot of point in logging them when a library is being used in some consumer application.

@timboudreau
Copy link
Author

Would appreciate it if any obstacles to merging this could be brought up, and if not, it could be merged.

I wound up ripping uniffi out of the codebase I created this patch to solve problems in - the marshalling overhead was significant compared with calling the same code via the C ABI, and removing it cut 5 minutes off my build times (parts that touched uniffi get built multiple times with and without macro generated metadata used to generate C/C++/Swift code).

I'm happy to follow up and get this patch to the finish line, if there are any objections or suggested improvements, though - I will probably use uniffi again in the future.

@badboy
Copy link
Member

badboy commented Sep 3, 2024

Would appreciate it if any obstacles to merging this could be brought up, and if not, it could be merged.

There was a weekend and a holiday in between. Sometimes things just take a bit of extra time. I'll see that we can get this forward this week.

@badboy badboy self-requested a review September 3, 2024 15:29
@badboy
Copy link
Member

badboy commented Sep 4, 2024

@timboudreau after talking with the others yesterday we decided to go ahead. Could you add a changelog entry?
Just add a new ### What's new? heading under here:

[All changes in [[UnreleasedUniFFIVersion]]](https://github.com/mozilla/uniffi-rs/compare/v0.28.1...HEAD).

Then add a line a la "- Added a new default-disabled feature log that enables some internal debugg logging" (or similar if you find a better phrasing)

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.

3 participants