I could be wrong, but it seems to arena checking is only done for
std::string in generated unsafe_arena_* code. I wonder if it is because
google internal version of protobuf has arena support for std::string, thus
the special treatment.
On Tuesday, May 5, 2020 at 5:09:01 PM UTC-7, King Kong wrote:
>
> unsafe_arena_set_allcoated* and unsafe_arena_release* API are "unsafe"
> APIs, callers should know what they are doing.
> Why not remove arena checking in all generated
> unsafe_arena_release* code? This will make unsafe_arena* APIs work for
> both arena-allocated and heap allocated objects. It seems a straightforward
> change to me, arena checking in unsafe_arena_release* does not always
> happen anyway.
>
> On Tuesday, May 5, 2020 at 4:20:33 PM UTC-7, King Kong wrote:
>>
>> Thanks for your reply.
>>
>> Yes, I have been using arena, but I wanted to do better than batching
>> heap allocation, when possible. The "scoped leasing" use case I described
>> above achieve 0 heap allocation all the time. I guess my code could use
>> GetArena() at runtime to decide how the "leasing" object is created and
>> which set of set&release APIs to use.
>>
>> On Tuesday, May 5, 2020 at 2:25:05 PM UTC-7, Josh Haberman wrote:
>>>
>>> The documentation for unsafe_arena_release_foo() says:
>>>
>>> > This method should only be used when the parent message is on the
>>> arena and the submessage is on the same arena, or an arena with equivalent
>>> lifetime.
>>>
>>> The DCHECK() you highlighted is checking the first part of this
>>> requirement.
>>>
>>> Stack/heap-allocated messages must always uniquely own their
>>> sub-messages, which must always be on the heap. Protobuf doesn't support
>>> the use case you are describing, where a message temporarily points to a
>>> message it doesn't own. If we allowed that, protobuf itself would crash if
>>> a message in this state was ever destroyed.
>>>
>>> If you are trying to avoid heap allocation overhead, the recommended
>>> solution is arenas.
>>>
>>> On Tuesday, May 5, 2020 at 12:44:32 PM UTC-7, King Kong wrote:
>>>>
>>>> I am using unsafe_arena_set_allocated_* and unsafe_arena_release_* APIs
>>>> to avoid
>>>> unnecessary allocations when the sub-objects are only used in a
>>>> function scope,
>>>> unsafe_arena_release_* is always called before leaving the scope. At
>>>> the beginning,
>>>>
>>>> I chose to always use the arena versions of unsafe APIs because the
>>>> non-arena version
>>>>
>>>> unsafe_release does extra allocation when the object is allocated on
>>>> arena, and the arena
>>>>
>>>> version does not check if the object is allocated in arena, based on
>>>> generated code like
>>>>
>>>> the following:
>>>>
>>>>
>>>> inline foo* outter1::unsafe_arena_release_foo() {
>>>> foo* temp = foo_;
>>>> foo_ = nullptr;
>>>> return temp;
>>>> }
>>>>
>>>>
>>>> However, I later also saw the following the generated code with arena
>>>> check, it means it is not okay to call for heap-allocated objects.
>>>>
>>>> inline void outter2::unsafe_arena_set_allocated_bar(
>>>> std::string* bar) {
>>>> GOOGLE_DCHECK(GetArenaNoVirtual() != nullptr);
>>>> ... ...
>>>> }
>>>>
>>>>
>>>> So, why the discrepancy?
>>>>
>>>>
>>>> The official documentation at
>>>> https://developers.google.com/protocol-buffers/docs/reference/arenas says
>>>> the correct set of APIs should be used
>>>>
>>>> according to how to the object is allocated, but having two different
>>>> sets of unsafe APIs makes it harder to use, and it makes the code less
>>>> robust to future changes. I am wondering if protobuf owners also realize
>>>> such an issue and start to remove arena checking gradually? It that the
>>>> case? I would like to hear how others think.
>>>>
>>>>
>>>> Thanks.
>>>>
>>>
--
You received this message because you are subscribed to the Google Groups
"Protocol Buffers" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To view this discussion on the web visit
https://groups.google.com/d/msgid/protobuf/52f265e3-595a-4b58-ac2c-9544026557b9%40googlegroups.com.
