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

Remove RepeatedField #503

Closed
Tracked by #518
Ekleog opened this issue Jul 10, 2020 · 5 comments
Closed
Tracked by #518

Remove RepeatedField #503

Ekleog opened this issue Jul 10, 2020 · 5 comments

Comments

@Ekleog
Copy link

Ekleog commented Jul 10, 2020

The documentation of RepeatedField claims that it is a

Wrapper around vector to avoid deallocations on clear.

However, the documentation of Vec::clear states that

Note that this method has no effect on the allocated capacity of the vector.

Which means that Vec::clear already does not deallocate.

As such, I think it'd make sense to just remove RepeatedField altogether, for version 3.

Related issue: #84

@stepancheg
Copy link
Owner

Vec::clear does not decallocate, but it invokes destructors of all fields, causing deallocation of vecs in nested messages.

RepeatedField preserves allocation of nested messages.

This gives a little speedup when reusing existing messages to read in loop, like this:

let mut m = MyMessage::new();
loop {
  m.clear();
  m.merge_from(is);
}

Maybe it is overengineering and not really needed, but it would be hard to remove RepeatedField without breaking backwards compatibility.

@Ekleog
Copy link
Author

Ekleog commented Jul 28, 2020

Hmm just to make sure I'm understanding correctly, do you mean that merge_from should be able to reuse the space freed from clear()? I could find no function in here that looks like it'd allow doing that.

I totally agree without it being hard to remove RepeatedField without breaking backwards compatibility, but maybe this issue could be kept around until a 3.0 gets out, at which point it could be removed indeed :)

@stepancheg
Copy link
Owner

merge_from should be able to reuse the space freed from clear()?

Basically, this operation is done without allocations:

let mut message: MyMessage = Message::new();
loop {
  message.clear();
  message.merge_from(...)?;
  message.check_initialized()?;
  do_something_with_message(&message);
}

That said, this feature is used very infrequently, and RepeatedField and SingularField is removed in master, and RepeatedPtrField no longer preserved the allocated memory.

This changes won't be backported to version 3 (because of backwards compatibility), instead, version 3 will be released soon.

@Ekleog
Copy link
Author

Ekleog commented Aug 13, 2020

Great, thank you!

@stepancheg
Copy link
Owner

Maybe rust-protobuf 3 need an option to generate RepeatedField while keeping Vec by default.

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

No branches or pull requests

2 participants