core: Make `Field` keys more flexible

[hawkw]

Feature Request

Crates

tracing-core

Motivation

The Field type is used in two primary ways:

• formatting the string value of the Field's name,
• comparing for equality, either between two Fields or between a Field and an &str.

We would like both of these use cases to be as efficient as possible.

In an attempt to enable this, we currently represent Fields as an &'static reference to a FieldSet (a static array of strings generated by the macros for each callsite) and an integer index into that array. This allows us fast comparisons between two Fields by comparing the callsite pointers, and if they are equal, comparing the integer index, avoiding the string comparison. The static string can be accessed by indexing the FieldSet array with the integer index, and is used when formatting the field or when comparing a Field with a &str.

This approach is very nice in theory, as it gives subscribers a nice way to do tests for field equality without having to compare strings. However, in practice, this is almost impossible for subscriber implementations to actually use the fast comparison. Here's why:

1. In order to use fast integer comparisons rather than slow string comparisons, you need to be comparing two Fields; otherwise, it is necessary to fall back to string comparisons. The only way to get a field is with code like this:

   // where `meta` is the metadata for some span or event that I have access to
   let field = meta.field_set().field("some_field_i_care_about")?;

   This means that if a Subscriber or a Visit implementation wants to do fast comparisons for a particular field it cares about, it needs to implement the register_callsite method and wait for a span or event that has a field matching the name that it cares about to be registered. Once a span or event has been registered, it can store the Field returned by the Metadata and use it for fast comparisons in the future...in theory. However, because register_callsite is called with a &self reference, the subscriber needs some way to mutate itself to actually store the field. This means locking, both when the field is stored initially, and every time the field is used (although the latter could be a read lock). This overhead may be enough to make the fast comparison no longer "fast", and complicates the implementation.

2. Field keys are only equal if they came from the same callsite. Because they are represented as an index into a static array that's created per callsite, they are only considered equal if the callsite pointers are the same --- if two spans from separate callsites have a field named "foo", it might be at index 1 in the first callsite, but at index 4 in the other callsite. This means that if I want to do the fast comparison for a field that exists on multiple spans, I need to store all those fields.

   When this code was initially written, this seemed like an acceptable limitation --- I thought that we would generally only care about field keys from a particular span. This, it turns out, is definitely not the case. It turns out that perhaps the most common use case for field key comparisons is implementing special formatting or handling for fields named message (and, secondarily, other fields with conventional names that occur on multiple spans/events). For example:
   

   tracing/tracing-subscriber/src/field/display.rs

   Lines 60 to 64 in ee1e530

   	if field.name() == "message" {
   	self.0.record_debug(field, &format_args!("{}", value))
   	} else {
   	self.0.record_str(field, value)
   	}

   If we wanted to use "fast" field comparisons here, we would need to have a register_callsite hook that stores every field key named "message" that's registered by any callsite (probably in a giant RwLock<HashSet<Field>> or something), and test if it contains the visited field every time we visit a field. This is, once again, no longer fast --- the string comparison is more efficient at this point.

Proposal

I think we can solve all of these problems by changing how we represent Field keys internally. Rather than representing them as indices into a static array that's registered per-callsite, we could instead represent them with a form of string interning. We would have a global registry that stores a reference to a static string for each field name that exists. Fields would be represented internally by&'static strs in that registry. We would provide a new function, tracing::field::register (or something), taking an &'static str and returning a Field. If a string whose character content is equal to the provided string exists in the registry, this function would return a Field with that string in it, and if it doesn't, we would add it to the registry.

This way, comparing two Fields could be implemented with ptr::eq on the two &'static strs, since requiring the registry to provide all Fields ensures that there is a single canonical address for each name. The implementation would look something like this:

pub struct Field {
    name: &'static str,
}

impl Field {
    pub fn register(name: &'static str) -> Field {
        lazy_static! {
            static ref FIELD_REGISTRY: RwLock<HashSet<&'static str>> = RwLock::new(HashSet::new());
        }

        if let Some(name) = FIELD_REGISTRY.read().unwrap().get(name) {
            Self { name }
        } else {
            FIELD_REGISTRY.write().unwrap().insert(name);
            Self { name }
        }
    }
}

impl fmt::Display for Field {
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        f.pad(self.name)
    }
}

impl PartialEq<Self> for Field {
    fn eq(&self, other: &Self) -> bool {
        // since the registry ensures that all fields created from equivalent strings have
        // the same address, we can just do this.
        std::ptr::eq(self.name, other.name)
    }
}

impl PartialEq<str> for Field {
    fn eq(&self, other: &str) -> bool {
        if std::ptr::eq(self.name as *const _ as *const (), other.name as *const _ as *const ()) {
            // if the string has the same address as we do, it must've been dereferenced from 
            // another `Field` with our value, so we don't need to compare characters..
            return true;
        }

        // fall back to comparing the strings' actual contents
        self.name.eq(other)
    }
}

// ... and so on ...

Because the register function is globally available, Subscriber implementations (or Visit implementations, or tracing_subscriber::Layer implementations...) that care about specific field names can call it when they are constructed, rather than having to wait for a particular span or event which has that field name to be registered. A subscriber could write code like this:

struct MySubscriber {
    // let's say I care about messages.
    message,

    // ...
}

impl MySubscriber {
    fn new() -> Self {
        // add the field to the registry if it does not already exist, or
        // a `Field` with its canonical addr if it does.
        let message = Field::register("message"); 
        // ...

        Self {
          message,
          // ...
        }
    }
}

impl Subscriber for MySubscriber {

    // or whichever method, it doesn't matter
    fn new_span(&self, span: span::Attributes<'_>) -> span::Id {
        // we get to use fast pointer compares rather than slow string compares
        // here, no matter which callsite the fields came from!
        if span.fields().contains(self.message) {
            // ... do stuff ...
        }
    }

    // ...
}

The callsite macros would be changed to register their fields the first time they are hit, and use the registered keys when recording spans/events. Because this only happens in the initialization path, the lock shouldn't be contended in the hot path.

Also, this approach has the side benefit of making access to the Field's string value a little tiny bit faster: in the current implementation, accessing the string means dereferencing the FieldSet reference and indexing the FieldSet array by the integer key (including a bounds check) in order to access the &'static str which is then itself dereferenced to format it; in this design, the Field just stores the &'static str directly, so there's one dereference.

If we are very smart, we may even be able to make this change without breaking stuff.

Alternatives

• We could, of course, not do this, and continue with the way things are. That has all the flaws discussed above.
• The very simple registry design proposed here only works with &'static strs. There is no way to intern a dynamic string. This is fine with the current state of tracing, since we don't allow that anyway. However, depending on how nested map-like value structures are represented for in Value::from_serde ? #819, we might need a way to have dynamic field values. We could do that in a few ways:
  • We could change the registry to be pointers, rather than &'static refs, and "leak" new Strings into the registry the first time they are registered, but this would require unsafe code that might look scary to readers.
  • Alternatively, we could make the internal repr of Field similar to a Cow that is either an owned String or a &'static str, giving us interning performance for static strings, but falling back to passing Strings around for dynamic ones.
  • A third option is to do both: since some dynamic strings might occur frequently, but others are one-shots that will only exist once, we could provide the user implementing Value a way to generate a Field that owns a String and a way to intern a dynamic String into the registry...

[hawkw]

Tagging @carllerche, as I imagine he might have opinions on this proposal.

[nagisa]

Note: just pondering below, not an actual proposal.

If the field values are at all times known statically (as they are in e.g. calls to event! and span! families of macros), the interner could be avoided entirely and the linker could be made to do the work for us. In particular something like…

#[export_name="_rust_tracing_field_name_$fieldname"]
#[linkage="linkonce"]
static $fieldname_FIELD_NAME: [u8; _] = *b"$fieldname";

since the symbol names must be unique across the executable, there will only ever be one address for &MESSAGE_FIELD_NAME across the entire executable, no matter how many definitions of this static exist.

The two problems with this approach is that you also need #[linkage="linkonce"] (or weak) which is unstable… And also somehow only generate at most one such static per crate...

Less reliable (read: useless), but in a given codegen unit multiple equivalent string literals will end up having the same address...

[hawkw]

@nagisa Yeah, I also wondered about making the linker do it for us, which would definitely be a cool hack if we could make it work. I didn't look into it too deeply because of 1, stability, like you mentioned, and 2, I don't really know how these attributes work (or even if they work) on more unusual targets like WASM, or what happens with code that relies on linker behavior runs in Miri (which would be a shame to break for our users). It's definitely something to keep an eye on, though!

[carllerche]

The Field type is used in two primary ways:

Originally, it was discussed that a Field could be used as a Record "index". This would provide a way to extract values from a Record without having to visit the entire record.

my_record.get_u64(&my_field) -> Option<u64>.

Under the hood, values would be stored using an array and the Field would be able to directly access the requested value.

[hawkw]

The Field type is used in two primary ways:

Originally, it was discussed that a Field could be used as a Record "index". This would provide a way to extract values from a Record without having to visit the entire record.

my_record.get_u64(&my_field) -> Option<u64>.

Under the hood, values would be stored using an array and the Field would be able to directly access the requested value.

Yeah...the field indexing that @carllerche mentions here is not actually currently supported, but I think we can put it back pretty easily. This only works with the current per-callsite Field representation, with an integer index, not with Fields represented by an interned string.

However, there isn't really any reason we can't add the field-name interning and have Field indexing. We could add a new type (maybe FieldName?) for matching against any field with a given name, which would have the interned string representation. A FieldSet would become an array of FieldNames, and we would have an implementation of PartialEq for Field/FieldName comparisons that match the interned static string, but Fields would retain integer indices for indexing ValueSets. You could use a FieldSet to look up the Field matching a given FieldName for that specific callsite.

As a side note, it turns out that if we want to add the field-name interning thing non-breakingly, we might have to do it like this anyway, with a new type. When I wrote the original proposal, I had forgotten that the Field type also had this method, returning a callsite::Identifier, which we can't remove without a breaking change. It looks like Fields will have to remain callsite aware until tracing-core 0.2 regardless of whether or not we continue to use the integer-based repr, so I think we'll need a separate type for "any field named X" matching.

[carllerche]

Interning field names seems fine. I can't think of a reason to not do it.

I would keep Field bound to the call site though and have the interned string be part of the field.

[hawkw]

@str4d I imagine that if we went with the approach I proposed here for the interned string registry

• We could change the registry to be pointers, rather than &'static refs, and "leak" new Strings into the registry the first time they are registered, but this would require unsafe code that might look scary to readers.

it might make your life much easier when working with FFI? That way, rather than having to provide exclusively &'static str, you could convert any arbitrary C++ string into a Rust string, and rely on tracing to manage its lifetime once registered...

[davidbarsky]

A possible extension to this proposal (as well as #819, perhaps?) would be to allow tracing_core::Value to be implemented on T: dyn std::any::Any + 'static. This would allow layers to opt-in to handle formatting of arbitrary types without requiring crates such as hyperium/http to take a public dependency on tracing_core::Value. I haven't really thought through the ecosystem-wide implications of supporting this, but I suspect tracing-subscriber could support a registry of formatters for arbitrary types, such that subscribers and layers won't need to take an explicit dependency on a given formatter.

[hawkw]

@davidbarsky I think that proposal deserves its own issue.