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 protobuf+unsubscr...@googlegroups.com.
To view this discussion on the web visit
https://groups.google.com/d/msgid/protobuf/52f265e3-595a-4b58-ac2c-9544026557b9%40googlegroups.com.
