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

Add is_compatible method to event content enums to fix Any*Event deserialization #52

Merged
merged 5 commits into from Jun 15, 2020

Conversation

DevinR528
Copy link
Member

@DevinR528 DevinR528 commented Jun 13, 2020

Will something like this work?

@jplatte
Copy link
Member

@jplatte jplatte commented Jun 14, 2020

I wouldn't want to hardcode more knowledge about the specific event types in the macros. I was thinking more of adding methods to the generated event content enums to determine which variant to deserialize.

@DevinR528
Copy link
Member Author

@DevinR528 DevinR528 commented Jun 14, 2020

This would still require a manual/custom derive for the Any*Event enums to be deserialized so the new method could be called?

@jplatte
Copy link
Member

@jplatte jplatte commented Jun 14, 2020

I'm not sure it's worth making it a macro. It could just be one implementation for AnyEvent, one for AnyRoomEvent.

@jplatte jplatte changed the title Adds derive macro to generate custom deserialize impl for Any*Event enums Add derive macro to generate custom deserialize impl for Any*Event enums Jun 15, 2020
@DevinR528
Copy link
Member Author

@DevinR528 DevinR528 commented Jun 15, 2020

How do I get at the actual deserialized content?

// inside of AnyEvent Deserialize impl
let json = JsonValue::deserialize(deserializer)?;
let raw: Box<RawJsonValue> = util::get_field(&json, "content")?;
let ev_type: String = util::get_field(&json, "type")?;
let content: ?????? =
    ruma_events::EventContent::from_parts(&ev_type, raw).map_err(D::Error::custom)?;
// then call the Any*EventContent's new method to figure out the variant

Do I need to manually do what #[serde(untagged)] did for all the Any*EventContent enums as in if let Ok(content) = ...? Or is there a better way to go about this?

@jplatte
Copy link
Member

@jplatte jplatte commented Jun 15, 2020

I was thinking of something like

// for AnyRoomEvent
match ev_type.as_str() {
    "m.room.redaction" => Self::Redaction(from_json_value(json).map_err(...)?),
    t if AnyMessageEventContent::is_compatible(t) => Self::Message(from_json_value(json).map_err(...)?),
    t if AnyStateEventContent::is_compatible(t) => Self::State(from_json_value(json).map_err(...)?),
}

@@ -1,2 +1,2 @@
/target
target
Copy link
Member

@jplatte jplatte Jun 15, 2020

Choose a reason for hiding this comment

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

Copy link
Member Author

@DevinR528 DevinR528 Jun 15, 2020

Choose a reason for hiding this comment

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

criterion generates a ruma-events/target dir I figured this should be ignored?

Copy link
Member

@jplatte jplatte Jun 15, 2020

Choose a reason for hiding this comment

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

Sure. I don't like it doing that, but I guess we can't do much about it.

ruma-events-macros/src/content_enum.rs Outdated Show resolved Hide resolved
// Since we are in a workspace the default `cargo bench` still picks up
// the args passed to it to avoid this use the above command.
Copy link
Member

@jplatte jplatte Jun 15, 2020

Choose a reason for hiding this comment

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

I don't understand this comment. What's "the default cargo bench"? Also, it's not clear to me what is avoided.

Copy link
Member

@jplatte jplatte Jun 15, 2020

Choose a reason for hiding this comment

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

I'm guessing this is about the -- --save-baseline syntax, but that's just an established convention for unix-style CLI tools – don't process stuff after a -- as flags / options. We don't need to document that here.

Copy link
Member Author

@DevinR528 DevinR528 Jun 15, 2020

Choose a reason for hiding this comment

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

cargo bench works but if you use criterions suggestion cargo bench -- --save-baseline <name> it fails with the error cargo bench unknown option --save-baseline I'm assuming this has something to do with the fact that we are in a workspace? I should change it to say the above and that the workaround is
cargo bench --bench event_deserialize -- --save-baseline <name>

Copy link
Member

@jplatte jplatte Jun 15, 2020

Choose a reason for hiding this comment

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

I don't know what that error is about. Does it make a difference whether you run that command from the repo root vs ruma-events?

Copy link
Member Author

@DevinR528 DevinR528 Jun 15, 2020

Choose a reason for hiding this comment

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

Nope at first, I thought it did but it gave me the same error both times. I've had this happen before turning off the bench harness worked the other time I found this and used the [lib] harness = false so I have no idea exactly why it won't work but I'd figure workspaces still being a bit flaky with some things.

Copy link
Member Author

@DevinR528 DevinR528 Jun 15, 2020

Choose a reason for hiding this comment

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

This is what I have now

// `cargo bench` works but if you use `cargo bench -- --save-baseline <name>`
// or pass any other args to it it fails with the error
// `cargo bench unknown option --save-baseline`.
// To pass args to criterion use this form
// `cargo bench --bench <name of the bench> -- --save-baseline <name>`

ruma-events/benches/event_deserialize.rs Outdated Show resolved Hide resolved
ruma-events/benches/event_deserialize.rs Outdated Show resolved Hide resolved
ruma-events/src/enums.rs Show resolved Hide resolved
ruma-events/src/enums.rs Outdated Show resolved Hide resolved
ruma-events/tests/any_enums.rs Outdated Show resolved Hide resolved
ruma-events/tests/any_enums.rs Outdated Show resolved Hide resolved
ruma-events/tests/any_enums.rs Outdated Show resolved Hide resolved
Copy link
Member

@jplatte jplatte left a comment

Looks pretty good. What do the benchmark results look like on your machine?

ruma-events/benches/event_deserialize.rs Outdated Show resolved Hide resolved
@DevinR528 DevinR528 changed the title Add derive macro to generate custom deserialize impl for Any*Event enums Add is_compatible method to event content enums to fix Any*Event deserialization Jun 15, 2020
@DevinR528
Copy link
Member Author

@DevinR528 DevinR528 commented Jun 15, 2020

deserialize to `AnyEvent`                                                                             
                        time:   [12.192 us 12.216 us 12.244 us]
Found 1 outliers among 100 measurements (1.00%)
  1 (1.00%) high mild

deserialize to `AnyRoomEvent`                                                                             
                        time:   [11.973 us 11.986 us 12.000 us]
Found 2 outliers among 100 measurements (2.00%)
  1 (1.00%) high mild
  1 (1.00%) high severe

deserialize to `AnyStateEvent`                                                                             
                        time:   [6.9297 us 6.9388 us 6.9498 us]
Found 3 outliers among 100 measurements (3.00%)
  3 (3.00%) high mild

deserialize to `StateEvent<PowerLevelsEventContent>`                                                                             
                        time:   [6.8239 us 6.8303 us 6.8365 us]
Found 1 outliers among 100 measurements (1.00%)
  1 (1.00%) low mild

So fully generic is about 1/2 as fast as even SpecificEvent<AnyContent>.

The MessageEvent<MessageEventContent> bench that was removed was 4.something us

ruma-events/benches/event_deserialize.rs Outdated Show resolved Hide resolved
ruma-events/benches/event_deserialize.rs Outdated Show resolved Hide resolved
@DevinR528 DevinR528 force-pushed the any-enum-deser branch 2 times, most recently from ffb6fdf to c2600f9 Compare Jun 15, 2020
@jplatte jplatte merged commit 0381190 into ruma:master Jun 15, 2020
3 checks passed
@DevinR528 DevinR528 deleted the any-enum-deser branch May 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants