std::string not allocated in Arena memory

[mrpi]

Hi!

I have noticed that std::string objects are not constructed in the arena memory but only marked as owned by the arena.

I understand why the current design does not allow to put long strings completely into the arena, but why is the std::string object itself not in the arena?

This way, string values that fit into the short string optimization buffer would not cause any heap allocation outside the arena and long strings would only require one heap allocation instead of two.

The method ArenaStringPtr::CreateInstance currently looks this way:

void CreateInstance(::google::protobuf::Arena* arena,
                      const ::std::string* initial_value) {
    GOOGLE_DCHECK(initial_value != NULL);
    ptr_ = new ::std::string(*initial_value);
    if (arena != NULL) {
      arena->Own(ptr_);
    }
}

but I would have expected an implementation that looks something like this:

void CreateInstance(::google::protobuf::Arena* arena,
                      const ::std::string* initial_value) {
    GOOGLE_DCHECK(initial_value != NULL);
    // uses "new ::std::string" when arena is nullptr
    ptr_ = Arena::Create<::std::string>(arena, *initial_value);
}

Thank you!

P.S.: Support for std::string_view ([ctype = STRING_VIEW]) as a replacement of your internal StringPiece would be even better :-)

[arthur-tacca]

Related (but different): #1896 Zero copy API

[acozzette]

@mrpi That is a good point; we should be able to allocate the std::string object on the arena. Do you want to send us a pull request with your fix?

[mrpi]

@acozzette Yes, until now I have not tried this change by my self. I have just noticed that my application was doing more small allocations than expected.
I will have a closer look at it the next days and create a patch.

[mrpi]

Patched with #4239. Benchmarks look fine.

[mrpi]

The pull request #4239 is merged. Closing this issue.