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

core: record Valuable values as valuable::Value #1881

Merged
merged 2 commits into from
Feb 2, 2022

Conversation

hawkw
Copy link
Member

@hawkw hawkw commented Feb 1, 2022

This changes the valuable integration so that record_value takes
instances of valuable::Value<'_>, rather than &dyn valuable::Valuable trait objects. The primary advantage of this is that
a Value can be produced by calling Valuable::as_value, so it allows
users to write code like this:

#[derive(Valuable, Debug)]
struct Foo {
    // ...
}

tracing::trace!(foo = foo.as_value());

rather than this:

#[derive(Valuable, Debug)]
struct Foo {
    // ...
}

tracing::trace!(foo = tracing::field::valuable(&foo));

which feels a bit more ergonomic. It also simplifies the code in
tracing-core, since we no longer need our own ValuableValue wrapper
type to turn things into trait objects.

It might also reduce boilerplate a bit on the visitor side, as
as_value() doesn't have to be called on the trait object, although
that's probably not as big a deal.

I didn't remove the field::valuable function, as I thought it was nice
to have for consistency with the existing field::debug and
field::display functions.

Performance Considerations

@carllerche pointed out that a Value<'_> might be slightly more bytes
to pass on the stack than a trait object (always two words). I believe
this is only the case when the Value is a Listable, Enumerable,
Structable, Mappable, or Tupleable, where the Value would be an
enum descriminant and a wide pointer to a trait object. However, in
the cases where the value is a primitive, Value will be two words if
the primitive is word-sized (e.g. u64 on 64-bit platforms), for the
enum descriminant + the value, or one word if the primitive is smaller
than word size (bool, char, etc). Also, for primitive Values,
there's no pointer dereference, which the trait object always requires.

I'm not sure how the enum dispatch compares to vtable dispatch when
calling visit on the value. However, if the tracing visitor is going
to call as_value() on the recorded value, this approach is better,
because calling as_value() in the macro prior to recording the
span/event will use the statically dispatched as_value() impl on a
known type, rather than the the dynamically dispatched as_value() impl
on the trait object. Since as_value impls are generally quite trivial,
I'd guess they usually (always?) will get inlined, which is never
possible with the dynamically dispatched call after passing a trait
object into tracing.

In practice I'm not sure if there's a huge perf diff either way, but it
was interesting to think through the implications.

This changes the `valuable` integration so that `record_value` takes
instances of `valuable::Value<'_>`, rather than `&dyn
valuable::Valuable` trait objects. The primary advantage of this is that
a `Value` can be produced by calling `Valuable::as_value`, so it allows
users to write code like this:

```rust
#[derive(Valuable, Debug)]
struct Foo {
    // ...
}

tracing::trace!(foo = foo.as_value());
```

rather than this:

```rust
#[derive(Valuable, Debug)]
struct Foo {
    // ...
}

tracing::trace!(foo = tracing::field::valuable(&foo));
```

which feels a bit more ergonomic. It also simplifies the code in
`tracing-core`, since we no longer need our own `ValuableValue` wrapper
type to turn things into trait objects.

It might also reduce boilerplate a bit on the visitor side, as
`as_value()` doesn't have to be called on the trait object, although
that's probably not as big a deal.

I didn't remove the `field::valuable` function, as I thought it was nice
to have for consistency with the existing `field::debug` and
`field::display` functions.

## Performance Considerations

@carllerche pointed out that a `Value<'_>` might be slightly more bytes
to pass on the stack than a trait object (always two words). I believe
this is only the case when the `Value` is a `Listable`, `Enumerable`,
`Structable`, `Mappable`, or `Tupleable`, where the `Value` would be an
enum descriminant _and_ a wide pointer to a trait object. However, in
the cases where the value is a primitive, `Value` will be two words if
the primitive is word-sized (e.g. `u64` on 64-bit platforms), for the
enum descriminant + the value, or one word if the primitive is smaller
than word size (`bool`, `char`, etc). Also, for primitive `Value`s,
there's no pointer dereference, which the trait object always requires.

I'm not sure how the enum dispatch compares to vtable dispatch when
calling `visit` on the value. However, if the `tracing` visitor is going
to call `as_value()` on the recorded value, this approach is better,
because calling `as_value()` in the macro _prior_ to recording the
span/event will use the statically dispatched `as_value()` impl on a
known type, rather than the the dynamically dispatched `as_value()` impl
on the trait object. Since `as_value` impls are generally quite trivial,
I'd guess they usually (always?) will get inlined, which is never
possible with the dynamically dispatched call after passing a trait
object into `tracing`.

In practice I'm not sure if there's a huge perf diff either way, but it
was interesting to think through the implications.
@hawkw hawkw requested a review from carllerche February 1, 2022 19:03
@hawkw hawkw requested a review from a team as a code owner February 1, 2022 19:03
@hawkw hawkw self-assigned this Feb 1, 2022
@hawkw
Copy link
Member Author

hawkw commented Feb 1, 2022

cc @xd009642

Copy link
Member

@davidbarsky davidbarsky left a comment

Choose a reason for hiding this comment

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

I think this is well-considered and I also think it requires the sign-off of others, but I'm happy to +1 this.

@carllerche
Copy link
Member

I wouldn't worry about the arg size thing. Taking Value seems better in general as it avoids dyn dispatch for primitives.

@hawkw hawkw enabled auto-merge (squash) February 2, 2022 21:53
@hawkw hawkw merged commit 532b61c into v0.1.x Feb 2, 2022
@hawkw hawkw deleted the eliza/valuable-by-value branch February 2, 2022 22:07
@hawkw
Copy link
Member Author

hawkw commented Feb 2, 2022

wait, the trait method should have taken Value<'_>, not &Value<'_>, because Value is Copy...how did i miss that?

hawkw added a commit that referenced this pull request Feb 2, 2022
In #1881, I accidentally had the `Visit` trait take a `&Value<'_>`. This
was probably wrong, since `Value` is `Copy` and should only be a couple
words, one of which might be a pointer...so it's probably better to just
pass the `pointer + enum descriminant` (or `integer + enum descriminant`)
instead of passing `pointer to (pointer + enum descriminant)`.

This probably also makes the API a little nicer.
hawkw added a commit that referenced this pull request Feb 2, 2022
In #1881, I accidentally had the `Visit` trait take a `&Value<'_>`. This
was probably wrong, since `Value` is `Copy` and should only be a couple
words, one of which might be a pointer...so it's probably better to just
pass the `pointer + enum descriminant` (or `integer + enum descriminant`)
instead of passing `pointer to (pointer + enum descriminant)`.

This probably also makes the API a little nicer.
hawkw added a commit that referenced this pull request Feb 3, 2022
Depends on #1881

Right now, the `valuable` stuff isn't very discoverable --- enabling the
feature just adds some trait impls and stuff that aren't particularly
visible in the documentation. This PR adds some top-level docs on using
`valuable`. In particular:

- Added a section to the `tracing` and `tracing-core` lib.rs docs
  explaining the unstable features versioning policy and how to turn on
  unstable features
- Added a section in the `field` module that explains how to use
  `valuable` to record fields.
- It turns out the `tracing::field` module didn't really have docs,
  since it doesn't re-export the `tracing_core::field` module but
  re-exports its _types_ in a new module (because it adds a trait). It
  had a single line of docs that just said something about "structured
  key-value data". I fixed this by coping the docs from `tracing-core`.
  :/
- Enabled unstable features in the documentation on docs.rs and netlify.
kaffarell pushed a commit to kaffarell/tracing that referenced this pull request May 22, 2024
This changes the `valuable` integration so that `record_value` takes
instances of `valuable::Value<'_>`, rather than `&dyn
valuable::Valuable` trait objects. The primary advantage of this is that
a `Value` can be produced by calling `Valuable::as_value`, so it allows
users to write code like this:

```rust
#[derive(Valuable, Debug)]
struct Foo {
    // ...
}

tracing::trace!(foo = foo.as_value());
```

rather than this:

```rust
#[derive(Valuable, Debug)]
struct Foo {
    // ...
}

tracing::trace!(foo = tracing::field::valuable(&foo));
```

which feels a bit more ergonomic. It also simplifies the code in
`tracing-core`, since we no longer need our own `ValuableValue` wrapper
type to turn things into trait objects.

It might also reduce boilerplate a bit on the visitor side, as
`as_value()` doesn't have to be called on the trait object, although
that's probably not as big a deal.

I didn't remove the `field::valuable` function, as I thought it was nice
to have for consistency with the existing `field::debug` and
`field::display` functions.

## Performance Considerations

@carllerche pointed out that a `Value<'_>` might be slightly more bytes
to pass on the stack than a trait object (always two words). I believe
this is only the case when the `Value` is a `Listable`, `Enumerable`,
`Structable`, `Mappable`, or `Tupleable`, where the `Value` would be an
enum descriminant _and_ a wide pointer to a trait object. However, in
the cases where the value is a primitive, `Value` will be two words if
the primitive is word-sized (e.g. `u64` on 64-bit platforms), for the
enum descriminant + the value, or one word if the primitive is smaller
than word size (`bool`, `char`, etc). Also, for primitive `Value`s,
there's no pointer dereference, which the trait object always requires.

I'm not sure how the enum dispatch compares to vtable dispatch when
calling `visit` on the value. However, if the `tracing` visitor is going
to call `as_value()` on the recorded value, this approach is better,
because calling `as_value()` in the macro _prior_ to recording the
span/event will use the statically dispatched `as_value()` impl on a
known type, rather than the the dynamically dispatched `as_value()` impl
on the trait object. Since `as_value` impls are generally quite trivial,
I'd guess they usually (always?) will get inlined, which is never
possible with the dynamically dispatched call after passing a trait
object into `tracing`.

In practice I'm not sure if there's a huge perf diff either way, but it
was interesting to think through the implications.
kaffarell pushed a commit to kaffarell/tracing that referenced this pull request May 22, 2024
In tokio-rs#1881, I accidentally had the `Visit` trait take a `&Value<'_>`. This
was probably wrong, since `Value` is `Copy` and should only be a couple
words, one of which might be a pointer...so it's probably better to just
pass the `pointer + enum descriminant` (or `integer + enum descriminant`)
instead of passing `pointer to (pointer + enum descriminant)`.

This probably also makes the API a little nicer.
kaffarell pushed a commit to kaffarell/tracing that referenced this pull request May 22, 2024
Depends on tokio-rs#1881

Right now, the `valuable` stuff isn't very discoverable --- enabling the
feature just adds some trait impls and stuff that aren't particularly
visible in the documentation. This PR adds some top-level docs on using
`valuable`. In particular:

- Added a section to the `tracing` and `tracing-core` lib.rs docs
  explaining the unstable features versioning policy and how to turn on
  unstable features
- Added a section in the `field` module that explains how to use
  `valuable` to record fields.
- It turns out the `tracing::field` module didn't really have docs,
  since it doesn't re-export the `tracing_core::field` module but
  re-exports its _types_ in a new module (because it adds a trait). It
  had a single line of docs that just said something about "structured
  key-value data". I fixed this by coping the docs from `tracing-core`.
  :/
- Enabled unstable features in the documentation on docs.rs and netlify.
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