Implement `Arc`, `Box` wrapper of message fields. #705

ldm0 · 2022-08-13T20:10:16Z

Closes #429
~~Marked as draft since tests haven't been added.(Go to bed now :-P)~~

ldm0 · 2022-10-18T12:35:49Z

Mostly done, add more tests later.

LucioFranco · 2022-10-18T15:09:08Z

Nice! Just want to let you know I see this but I have a bunch of high priority things on my plate atm so may take me a bit to get to a review of this.

80ROkWOC4j · 2024-03-26T03:07:57Z

How is this working out? It's sad that I have to call many Vec::clone() to send a message with repeated fields to thousands of clients 😥. I'm hoping this PR will solve my problem.

PSeitz · 2024-04-18T14:39:17Z

I just want to second this. Having Arc would be sweet, as memory consumption on some structs is reaching relative high levels in quickwit

Remediem · 2024-07-18T04:59:42Z

Any updates on this?

caspermeijn · 2024-07-26T15:10:50Z

Any updates on this?

Yes, this PR needs a reviewer and someone to bring it to the finish line. @Remediem Are you willing to do some of that work?

caspermeijn · 2024-09-11T07:10:42Z

@juanAngel When I said, “this PR needs a reviewer”, I didn't mean someone who hits the Approve button. I meant someone who provides useful comments about the large code changes.

Maybe you are willing to split this PR is multiple smaller PRs that are easier to review?

ldm0 · 2024-09-11T08:15:46Z

This PR is ready for review at 2022. The Box wrapper introduced in #1019 conflicts with this PR.

I can't split it into smaller PRs as it's quite self-contained, and most of the new code is from the tests.

@caspermeijn @LucioFranco Could you review the test code in this PR and let me know if it's acceptable? If you are still interested in the Arc wrapper for message fields, I can rebase this onto the master branch.

juanAngel · 2024-09-11T12:55:17Z

@caspermeijn

Don't think I didn't read the code before hitting the button
I can think of improvements to the code itself, but they are more optimizations than anything else. Like for example box and arc don't make much sense for most primitive types like u32

And I was implementing those improvements on my own.

As for the rest I don't know what kind of comments are needed if the code/tests seem to be correct

caspermeijn · 2024-09-13T06:21:57Z

Don't think I didn't read the code before hitting the button I can think of improvements to the code itself, but they are more optimizations than anything else. Like for example box and arc don't make much sense for most primitive types like u32

@juanAngel I'm sorry I reacted that way. There have been some low effort comments lately and I confused your approval with that. Thank you for explaining what you have reviewed, and thank you for the review.

caspermeijn

Arc is a much requested feature. Thank you for implementing it.

I like the concept of a wrapper type. I like that it is mutual exclusive, as that reduces the possible combinations. I like that it only contains allocating wrappers.

The diff will be smaller after merging this code path with the existing boxed code.

Please add user-facing documentation.

caspermeijn · 2024-09-13T06:33:01Z

prost-build/src/lib.rs

@@ -421,6 +444,35 @@ impl Config {
        self
    }

+    fn field_wrapper<I, S>(&mut self, paths: I, wrapper: Wrapper)


Thinking out loud: Is it beneficial to make field_wrapper public instead of arc and box? That way, it is clear that they are mutual exclusive.

caspermeijn · 2024-09-13T06:34:23Z

prost-build/src/code_generator.rs

+        let field_wrapper = self
+            .config
+            .field_wrappers
+            .get_first_field(fq_message_name, oneof.name())


Do we want to error out if there are conflicting field wrappers defined for a single field?

caspermeijn · 2024-09-13T06:39:30Z

prost-build/src/code_generator.rs

        let boxed = !repeated
+            && field_wrapper != Some(Wrapper::Box)


The message graph boxed detection is for solving recursive types. If a recursive type is detected, then it is automatically placed in a Box to make sure the struct size is known at compile time. Arc will have the same function.

I don't think message graph will take the explicit boxed into account. Maybe it should with more field wrappers?

caspermeijn · 2024-09-13T06:42:44Z

prost-build/src/path.rs

 pub(crate) struct PathMap<T> {
    // insertion order might actually matter (to avoid warning about legacy-derive-helpers)
    // see: https://doc.rust-lang.org/rustc/lints/listing/warn-by-default.html#legacy-derive-helpers
    pub(crate) matchers: Vec<(String, T)>,
 }

+// Remove this after <https://github.com/rust-lang/rust/issues/26925> is fixed.


Interesting that this trigger without changing the struct. I would like to see a comment to explain why this is needed (without reading the GitHub issue).

caspermeijn · 2024-09-13T06:44:27Z

prost-derive/src/field/group.rs

@@ -25,6 +29,8 @@ impl Field {
                set_bool(&mut group, "duplicate group attributes")?;
            } else if word_attr("boxed", attr) {
                set_bool(&mut boxed, "duplicate boxed attributes")?;
+            } else if let Some(w) = wrapper_attr(attr) {


I think the boxed code path should be merged into the wrapper code path.

caspermeijn · 2024-09-13T06:58:59Z

prost-derive/src/field/mod.rs

+        match wrapper {
+            Some(Wrapper::Arc) => {
+                quote!(::prost::alloc::sync::Arc::get_mut(&mut self.#ident).ok_or_else(|| {
+                    ::prost::DecodeError::new("Cannot get mutable reference to a Arc field")


When can this happen? During decoding, there will never be another Arc to the same allocation, right? Is make_mut useful here?

caspermeijn · 2024-09-13T07:02:31Z

prost-derive/src/field/mod.rs

-pub(super) fn tag_attr(attr: &Meta) -> Result<Option<u32>, Error> {
+// TODO pub(crate)?
+#[derive(Clone, Copy, Debug, PartialEq, Eq)]
+pub enum Wrapper {


Should the Wrapper type and helper functions be in a separate module to reduce file size?

caspermeijn · 2024-09-13T07:05:03Z

prost-derive/src/field/mod.rs

+/// Get reference of a field.
+fn field_ref(wrapper: &Option<Wrapper>, ident: TokenStream) -> TokenStream {
+    if wrapper.is_some() {
+        quote!(&*#ident)


I feel like the * could make a copy of the contents. Maybe as_ref() can help here. Would it be possible to replace field_ref by as_ref?

caspermeijn · 2024-09-13T07:09:55Z

prost-derive/src/field/scalar.rs

+                        let pushed = wrapper.wrap_type(quote!(pushed));
+                        quote! {
+                            let mut pushed = (*#field).clone();
+                            pushed.push(value as i32);
+                            self.#ident = #pushed;
+                        }


Can you explain why this is needed?

caspermeijn · 2024-09-13T07:16:40Z

tests/field-wrappers/Cargo.toml

@@ -0,0 +1,10 @@
+[package]
+name = "field-wrappers"


I would like the tests to be part of crate tests as that crate is run for every edition and with no-std.

juanAngel · 2024-09-13T07:44:45Z

prost-build/src/code_generator.rs

+        }
+
+        self.buf.push_str("::core::option::Option<");
+        self.buf.push_str(&name);


Wouldn't it be better if arc was inside the option?

Normally you will try to clone the content of the option, not the whole option. And cloning a some or none I don't think will be a big cost.

juanAngel · 2024-09-13T07:57:30Z

prost-build/src/code_generator.rs

+        self.buf.push_str("=\"");
+        self.buf.push_str(&key_tag);
+        self.buf.push_str(", ");
+        self.buf.push_str(&value_tag);


Here it might be interesting to add an extra arc/box just for the value
That way you can share the individual elements

For the key, from my point of view it is less necessary since they are usually easily copied elements and are mostly only used for searches or insertions

juanAngel · 2024-09-13T08:10:04Z

prost-build/src/code_generator.rs

+            .config
+            .field_wrappers
+            .get_first_field(fq_message_name, field.name())
+            .copied();


In my opinion it is interesting to add a filter here.
Since you will never want an enum wrapped in arc, or an integer
Since they are much cheaper to clone than arc itself, in addition to implementing automatic copying

nothing2sayi · 2024-10-17T06:19:40Z

Is there any progress on this issue? When is it expected to be merged? thanks.

ldm0 · 2024-10-17T06:42:08Z

Is there any progress on this issue? When is it expected to be merged? thanks.

@nothing2sayi Yeah, I am working on this. I have successfully rebased this MR locally. But there are several bugs in the current implementation of boxed(in the master branch), which need to be solved before this MR could be merged. It may take several weeks.

ldm0 force-pushed the ldm_arc branch from 3272993 to c732a9f Compare August 14, 2022 18:13

ldm0 force-pushed the ldm_arc branch from c732a9f to e9ba810 Compare October 17, 2022 18:56

ldm0 changed the title ~~Implement Arc, Rc, Box wrapper of message fields.~~ Implement Arc, Box wrapper of message fields. Oct 17, 2022

ldm0 force-pushed the ldm_arc branch 4 times, most recently from 255b101 to 154c338 Compare October 18, 2022 06:27

ldm0 force-pushed the ldm_arc branch 3 times, most recently from 064005a to 6547129 Compare October 18, 2022 19:25

ldm0 marked this pull request as ready for review October 18, 2022 19:25

ldm0 force-pushed the ldm_arc branch from 6547129 to 92a0906 Compare October 22, 2022 15:38

ldm0 force-pushed the ldm_arc branch from 92a0906 to fc26feb Compare November 4, 2022 16:22

ldm0 force-pushed the ldm_arc branch from fc26feb to d6e60d1 Compare November 25, 2022 09:52

ldm0 force-pushed the ldm_arc branch from d6e60d1 to 6f51225 Compare March 31, 2023 16:52

Implement Arc, Box wrapper of message fields.

ddda5ee

ldm0 force-pushed the ldm_arc branch from 6f51225 to ddda5ee Compare March 31, 2023 17:53

juanAngel approved these changes Sep 10, 2024

View reviewed changes

caspermeijn requested changes Sep 13, 2024

View reviewed changes

juanAngel reviewed Sep 13, 2024

View reviewed changes

ldm0 mentioned this pull request Sep 13, 2024

fix(prost-build): Remove derived(Copy) on boxed fields #1157

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement `Arc`, `Box` wrapper of message fields. #705

Implement `Arc`, `Box` wrapper of message fields. #705

ldm0 commented Aug 13, 2022 •

edited

Loading

ldm0 commented Oct 18, 2022

LucioFranco commented Oct 18, 2022

80ROkWOC4j commented Mar 26, 2024

PSeitz commented Apr 18, 2024

Remediem commented Jul 18, 2024

caspermeijn commented Jul 26, 2024

caspermeijn commented Sep 11, 2024

ldm0 commented Sep 11, 2024

juanAngel commented Sep 11, 2024

caspermeijn commented Sep 13, 2024

caspermeijn left a comment

caspermeijn Sep 13, 2024

caspermeijn Sep 13, 2024

caspermeijn Sep 13, 2024

caspermeijn Sep 13, 2024

caspermeijn Sep 13, 2024

caspermeijn Sep 13, 2024

caspermeijn Sep 13, 2024

caspermeijn Sep 13, 2024

caspermeijn Sep 13, 2024

caspermeijn Sep 13, 2024

juanAngel Sep 13, 2024

juanAngel Sep 13, 2024

juanAngel Sep 13, 2024

nothing2sayi commented Oct 17, 2024

ldm0 commented Oct 17, 2024

Implement Arc, Box wrapper of message fields. #705

Are you sure you want to change the base?

Implement Arc, Box wrapper of message fields. #705

Conversation

ldm0 commented Aug 13, 2022 • edited Loading

ldm0 commented Oct 18, 2022

LucioFranco commented Oct 18, 2022

80ROkWOC4j commented Mar 26, 2024

PSeitz commented Apr 18, 2024

Remediem commented Jul 18, 2024

caspermeijn commented Jul 26, 2024

caspermeijn commented Sep 11, 2024

ldm0 commented Sep 11, 2024

juanAngel commented Sep 11, 2024

caspermeijn commented Sep 13, 2024

caspermeijn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nothing2sayi commented Oct 17, 2024

ldm0 commented Oct 17, 2024

Implement `Arc`, `Box` wrapper of message fields. #705

Implement `Arc`, `Box` wrapper of message fields. #705

ldm0 commented Aug 13, 2022 •

edited

Loading