rust: release cleanup #1071

notoriaga · 2021-09-17T21:37:18Z

Apologies in advance for the massive diff. Most of it is renaming/moving things around. The bulk of new changes are in wire_format.rs and sbp_string.rs.

Breaking API changes:

Removes get_ prefix from getters - https://rust-lang.github.io/api-guidelines/naming.html#getter-names-follow-rust-convention-c-getter
Renames SBP -> Sbp and SBPMessage -> SbpMessage. Also treats acronyms like GPS/NED as one word (i.e. Gps/Ned) - https://rust-lang.github.io/api-guidelines/naming.html#casing-conforms-to-rfc-430-c-case. The casing of JSON messages was left as is
Renames sbp_serde feature to just serde - https://rust-lang.github.io/api-guidelines/naming.html?highlight=fea#feature-names-are-free-of-placeholder-words-c-feature
Replaces the Error enum with two, DeserializeError/SerializeError. The finer grained types prevent you from having to deal with errors that can never happen
Marks Sbp enum as #[non_exhaustive] (not that anyone is realistically matching on all variants but you never know)
No longer reexports stuff from dencode/bytes. Reading messages is done via iter_messages like before. Writing messages now can be done with sbp::to_writer or sbp::to_vec. SbpEncoder is available for writing messages with buffered output. Previously you needed to use dencode::FramedWrite with the encoder, but now each encoder contains the FramedWrite
SBPTools -> SbpIterExt because that's what it is/that's normally what I see things like this called

Additions:

Adds preliminary support for the new string encodings.
Adds support for fixed size arrays (similar to what @jbangelo had here [WIP] Take advantage of const generics in the Rust parser #903). Haven't seen much of a perf change, but maybe it might help with the fragmentation issues @silverjam mentioned. Also feels more semantically correct to say that something like a velocity vector is an [f64; 3] not a Vec<f64>
Doc comments/some doc tests

Internal improvements:

Fixes generated docs to escape braces/wrap links in <link>
Rejiggers the auto generated tests to produce a single test binary rather than one for each
Removes all but one of the #[allow(..)] statements in the generated code by tweaking the code gen
Removes nom/thiserror/byteorder. Without any features enabled we dropped from 46 deps to 10, debug builds drop from ~22 -> ~9 seconds and release builds from ~43 -> ~11 seconds. It turns out bytes has all we need for parsing (mainly an easy way to pull LE bytes out of a buffer).
Removes SbpSerialize trait and replaces it with WireFormat (not sure this is the best name). It's very similar to SbpSerialize but also includes functions for reading the types. Unlike SbpSerialize this trait is not public. Well it's technically public but unnameable by downstream crates (https://rust-lang.github.io/api-guidelines/future-proofing.html). This was done because it's really an implementation detail. Plus, because WireFormat is a super trait of SbpMessage, it makes it impossible for downstream crates to implement SbpMessage which is probably a good idea.

Future things:

Expand on the SbpString stuff. Should add validation when parsing/writing that the string satisfies the format. Could also add functions to iterate over the parts of a multipart string for example.
When the docs refer to other messages like we could replace the names with links to the actual types.

All in all a lot of changes. Happy to undo any of them that people don't like/think are unnecessary

notoriaga · 2021-09-27T19:14:29Z

rust/sbp/src/wire_format.rs

@@ -0,0 +1,210 @@
+//! Functionality for parsing/writing SBP message payloads with in memory buffers


Some actual new-ish code. A combination of SbpSerialize and all the free standing parse_<type>() functions

notoriaga · 2021-09-27T19:15:36Z

rust/sbp/src/wire_format.rs

+        }
+    }
+
+    fn parse_unchecked(buf: &mut BytesMut) -> Self {


Probably a premature optimization. But, this is pretty much the example right out of the docs for MaybeUninit so I don't feel too bad about this touch of unsafe

notoriaga · 2021-09-27T19:16:46Z

rust/sbp/src/sbp_string.rs

@@ -0,0 +1,229 @@
+use std::fmt;


The other chunk of actually new stuff

silverjam · 2021-09-27T23:28:03Z

* Adds support for fixed size arrays (similar to what @jbangelo had here [[WIP] Take advantage of const generics in the Rust parser #903](https://github.com/swift-nav/libsbp/pull/903)). Haven't seen much of a perf change, but maybe it might help with the fragmentation issues @silverjam mentioned. Also feels more semantically correct to say that something like a velocity vector is an `[f64; 3]` not a `Vec<f64>`

Semantics look a lot better, but I think we'd need further changes for this to have a real impact on (potential) heap fragmentation issues. "Further changes" would probably mean the ability to allocate the whole SBP message struct on the stack with variable sized arrays backed by an "upper maximum" static array (similar to the new v4 libsbp API for C).

I tried to make a change to ensure that we're allocating consistently size buffers here: 64cafc3 to make things easier for heap reclamation -- but it did not have any impact on the specific console issue (which probably means it's just a leak, not a fragmentation issue).

silverjam

New code looks good

jbangelo

Changes look good to me.

jbangelo · 2021-09-27T23:18:21Z

generator/sbpg/targets/resources/sbp_messages_mod.rs

 use self::unknown::Unknown;

-use crate::serialize::SbpSerialize;
+mod lib {


Should this be called a prelude? Or is that term just for common imports from an external crate?

Hmm yeah I just took the idea from serde_json where they call it lib - https://docs.serde.rs/src/serde_json/lib.rs.html#366
Although they are doing it for a slightly different reason (still internal though)

jbangelo · 2021-09-27T23:20:05Z

generator/sbpg/targets/resources/sbp_messages_mod.rs

-pub trait ConcreteMessage: SbpMessage + std::convert::TryFrom<Sbp, Error = TryFromSbpError> + Clone + Sized {
+/// Implemented by messages who's message name and type are known at compile time.
+/// This is everything that implements [SbpMessage] except for [Sbp].
+pub trait ConcreteMessage: SbpMessage + TryFrom<Sbp, Error = TryFromSbpError> {


If ConcreteMessage is only for use in the sbp crate, should this be marked as pub(crate)?

Hmm it can't be marked private because it a appears in Event which needs to be public. Could maybe add #[doc(hidden)]. Depends on if we think having access to message names/ids at compile time is useful. Right now this is the only way to get that

john-michaelburke · 2021-09-28T17:42:56Z

generator/sbpg/targets/resources/sbp_tests_template.rs

            _ => panic!("Invalid message type! Expected a (((t.msg.name)))"),
        };
-        let frame = sbp_msg.to_frame().unwrap();
+        let frame = sbp::to_vec(&sbp_msg).unwrap();


nit. What is the idiomatic rule of thumb for using to_asdf vs as_asdf? Is it related to the cost of the action such as a string to slice using "as" vs a vector to string use "to"

Yeah pretty much - as_ should be cheap and not consume the original. to_ can be more expensive and still does not consume the original. Then into_ can be either I guess but it should consume the original value. So in this case I went with to_ because it allocates

john-michaelburke · 2021-09-28T17:44:42Z

generator/sbpg/targets/rust.py

+      s = s.replace(url, "<" + url + ">")
+  return s
+
+def escape_braces(s):


Maybe for a future addition, it would be neat to see if we could somehow get python docstrings embedded into the rust generated docs seeing as how these are related to the crate generation.

Yeah kind of like how the top of each message file tells you which yaml file it came from

notoriaga added 10 commits September 16, 2021 18:37

remove get_ prefixes

bd593bc

refactor tests to create one binary

a150889

use ConcreteMessage trait

fce7b6b

mark SBP as #[non_exhaustive]

b1103c4

snake case acronyms

87b8e06

more 🐍 casing

0f06e47

fix ci test gen

d457212

rename sbp_serde feature to serde

958975b

refactor

4f70dc3

Merge branch 'master' into steve/rust-release

306afa1

notoriaga force-pushed the steve/rust-release branch from 907492f to 306afa1 Compare September 27, 2021 18:49

update rust benchmark image

3f21635

notoriaga force-pushed the steve/rust-release branch from 2fd8aec to 3f21635 Compare September 27, 2021 19:18

notoriaga commented Sep 27, 2021

View reviewed changes

notoriaga requested review from jbangelo, john-michaelburke and silverjam and removed request for jbangelo and john-michaelburke September 27, 2021 19:28

silverjam approved these changes Sep 27, 2021

View reviewed changes

jbangelo approved these changes Sep 27, 2021

View reviewed changes

update swiftnav

568e4a0

john-michaelburke reviewed Sep 28, 2021

View reviewed changes

john-michaelburke approved these changes Sep 28, 2021

View reviewed changes

notoriaga merged commit 9b7ad95 into master Sep 28, 2021

notoriaga deleted the steve/rust-release branch September 28, 2021 18:37

		@@ -0,0 +1,210 @@
		//! Functionality for parsing/writing SBP message payloads with in memory buffers

rust: release cleanup #1071

rust: release cleanup #1071

Uh oh!

Conversation

notoriaga commented Sep 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

silverjam commented Sep 27, 2021

Uh oh!

silverjam left a comment

Choose a reason for hiding this comment

Uh oh!

jbangelo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

notoriaga commented Sep 17, 2021 •

edited

Loading