Add feature flag to minimize RecordLocation unknowns

[giarc3]

As proposed by @dpc here, this PR allows non-static modules and files to come through slog by leaking the memory.

I decided to put this behind a feature flag because I don't think that people depending on this should be forced into this change. I called the feature flag record_location_unstable but would be open to naming suggestions.

Let me know if there are other changes I should make for this. Thank you very much!

[dpc]

Awesome! Please follow up with a CHANGELOG update. Thanks!

[dpc]

Oh, wait. As things are right now, we are not reusing leaked values. That would mean the program will keep leaking memory, no?

We need to reuse values that were already leaked in the past. There are widely used off-the-shelve crates for that.

[giarc3]

Oh, wait. As things are right now, we are not reusing leaked values. That would mean the program will keep leaking memory, no?

We need to reuse values that were already leaked in the past. There are widely used off-the-shelve crates for that.

You're definitely right, my mistake! Do you have a recommendation for a crate to help accomplish that? I'm not familiar with common Rust crates yet.

[dpc]

I threw interning into lib.rs and:

https://lib.rs/crates/string_cache

seems well established?

Other than that no idea.

[giarc3]

@dpc I used the cached crate to accomplish only leaking each string once. The downside is that the macro is unable to accept a function signature that includes 'a, so I had to use to_string() before passing it in. The consequence of that is just the extra call regardless of whether the static string already exists. I did some testing with one of my projects locally and the memory usage no longer grows unbounded.

I added a changelog entry for 4.2.0 and also noticed that the one for 4.1.0 was missing, so I made an attempt at adding one for that as well.

Please let me know if there's more to do for pushing this forward!

[dpc]

BTW. I skimmed through cached and I don't understand why do you use it. Doesn't seem made for interning strings or anything. Can you explain what's the reasoning behind it?

[giarc3]

I've never done string interning in particular so I was looking for an ergonomic caching library which I thought would accomplish the same result.
I can do more digging on string_cache or string_interner crates if you'd prefer those, just let me know.

[dpc]

I'm not in depth on interning, but I think they are good reasons why there are specialized libraries for them).

[dpc]

The consequence of that is just the extra call regardless of whether the static string already exists.

We definitely don't want an unnecessary memory allocation if we can avoid it. They are kind of costly, given that logging statements keep repeating over and over.

[giarc3]

The consequence of that is just the extra call regardless of whether the static string already exists.

We definitely don't want an unnecessary memory allocation if we can avoid it. They are kind of costly, given that logging statements keep repeating over and over.

I worked with a co-worker trying out different string interning libraries but ran into issues with each one with getting a static string back out of the interner.

Instead I was able to fix up the cached solution so it only does the to_string() when necessary, so it won't be calling it when we already have a static string. I ran my tests again and confirmed that it only leaks the strings once.

[dpc]

Shouldn't this be non_static_info: Option<&'str>?

[dpc]

The point of this function is to turn &'a str into &'static str

[giarc3]

Unfortunately it can't be. If I understand correctly, it's because it can't cache based on a non_static string as the cache key would be dropped. This is similar to issues I had with the interning libraries.
So I have to clone it before it gets passed into this function. But it only does that clone in the case where it needs to (a static string doesn't already exist).

[giarc3]

@dpc would you be willing to take another look at this? I think it's in a reasonable place and gated behind a feature flag to warn consumers before enabling it.