Issue 402 in protobuf: question for clear and parse

[prot...@googlecode.com]

Status: New
Owner: liuj...@google.com
Labels: Type-Defect Priority-Medium

New issue 402 by hlhhuang...@gmail.com: question for clear and parse
http://code.google.com/p/protobuf/issues/detail?id=402

What steps will reproduce the problem?
1.define a complex protobuf structrue (name: Record)
2.Recycling data:

Record* pRecord = new RecordSet;
while(1)
{
pRecord->clear();
//get a Serializing string data (string* pStr)
...
//deserialize
pRecord->ParseFromCodedStream(...);
...
}
3.there are two case:
if the hierarchy of Record is single, there is no memory leak.
if the hierarchy of Record is diffent, there is memory leak.

What is the expected output? What do you see instead?
Modify the above code to :

while(1)
{
Record* pRecord = new RecordSet;
//get a Serializing string data (string* pStr)
...
//deserialize
pRecord->ParseFromCodedStream(...);
...
delete pRecord;
}
there will be ok whatever the hierarchy of Record is.
there is a way to work not with delete ? delete is Time-consuming.
What version of the product are you using? On what operating system?
2.4.1 red-hat linux

Please provide any additional information below.
thank you

[prot...@googlecode.com]

Comment #1 on issue 402 by liuj...@google.com: question for clear and parse
http://code.google.com/p/protobuf/issues/detail?id=402

This is a quite common use-case, and there shouldn't be any memory leaks.
I'd suspect the leak happened somewhere else in your code. Can you provide
a complete code example(runnable) showing the leakage?

[prot...@googlecode.com]

Comment #2 on issue 402 by hlhhuang...@gmail.com: question for clear and
parse
http://code.google.com/p/protobuf/issues/detail?id=402

I can only give you these codes , hope it is clear.
thank you.

Attachments:
codes.rar 166 KB

[prot...@googlecode.com]

Comment #3 on issue 402 by hlhhuang...@gmail.com: question for clear and
parse
http://code.google.com/p/protobuf/issues/detail?id=402

I saw a little bit of source code, ParseFromCodedStream() calls
message::Clear(), but Clear() avoids freeing memory. you must delete it if
you want to free memory.

[prot...@googlecode.com]

Comment #4 on issue 402 by liuj...@google.com: question for clear and parse
http://code.google.com/p/protobuf/issues/detail?id=402

Protobuf is opensourced for over 4 years, and we have memory leak tests
(tc_malloc heap check) before each release. I'm quite confident that
repeatedly Clear() then ParseFromCodedInputStream() will not introduce
memory leaks inside protobuf library or protobuf generated code. It's more
likely that the leak happened in your application code. However, from the
code you provided, there isn't enough info that I can help. What tool did
you use to check memory leaks?

[prot...@googlecode.com]

Comment #5 on issue 402 by hlhhuang...@gmail.com: question for clear and
parse
http://code.google.com/p/protobuf/issues/detail?id=402

there is no need to use tool, because the memory is more and more large
through top order.
Why a single data don't cause that case ? I can't really understand the
diffence between delete(or distructor function) Clear()

[prot...@googlecode.com]

Comment #6 on issue 402 by liuj...@google.com: question for clear and parse
http://code.google.com/p/protobuf/issues/detail?id=402

Clear() resets the fields to their default values. So after a Clear(), you
can treat the message the same as a just created new message.(though we
cache sub message field for better allocation performance, but that should
be transparent). It will not delete the message. Did you really observe
memory leaks in your code example? Or did you allocate a new message,
assign it to the pointer after the Clear() and didn't free the memory in
your real code?

[prot...@googlecode.com]

Comment #7 on issue 402 by hlhhuang...@gmail.com: question for clear and
parse
http://code.google.com/p/protobuf/issues/detail?id=402

I am so sorry that i have no right to give you all the codes (In my
opinion, I think the problem lies in this part of the code. Maybe). I will
make further testing, and hope giving you more info to help me. I will
check codes Under your proposal. Thank you.

[prot...@googlecode.com]

Comment #8 on issue 402 by hlhhuang...@gmail.com: question for clear and
parse
http://code.google.com/p/protobuf/issues/detail?id=402

hi,master, the struction of Recordset class I define is Scalable and nested.
The parsed datas have different levels and fields. It can't always make
full use of allocated memoey every time.
Clear() is just resets the fields to their default values, but not frees
the memory.
There is new opreration in ParseFromCodedInputStream(), but never delete
operation (just only clear())
so, the momery is more and more large.
If the data is single, it can make full use of allocated memoey every time.
I was right ?

[prot...@googlecode.com]

Comment #9 on issue 402 by liuj...@google.com: question for clear and parse
http://code.google.com/p/protobuf/issues/detail?id=402

If we have such a message below, where Bar is a message type.

message Foo {
optional Bar bar = 1;
}

Then in C++ generated class, Foo class will have a Bar pointer for the
field, (Bar* bar;}
1) Before the first ParseFromCodedInputStream(), the bar pointer will be
NULL.
2) In the parsing function, we will new a Bar() instance and assign it to
the bar_ pointer.
3) After the foo.Clear(), we call bar.Clear() in the function, and bar will
still point to the message we allocated in the step 2.
4) The next ParseFromCodedInputStream(), we will directly merge the bytes
into the bar pointer, instead of creating a new one.
5) The allocated bar message will be deleted when the parent foo message is
destroyed.

That said, there will no memory leaks, but it's possible that you could
observe increasing memory usages in the repeated Clear() and ParseFrom().
(Imagine you have thousands of message fields, and each time there is some
message allocated) But those messages are just cached for the next
MergeFrom() to reduce the allocating cost and memory footprint. They will
be deleted once the parent message is deleted. If you find the memory usage
is increasing linearly and boundlessly, then there must be something wrong
in your code.

[prot...@googlecode.com]

Comment #10 on issue 402 by hlhhuang...@gmail.com: question for clear and
parse
http://code.google.com/p/protobuf/issues/detail?id=402

Thank you for your detailed explanation.

But there is still memory increasing:

RecordSet* pRS = new RecordSet;
int count = 0;
while(1)
{
pRS->Clear();

//get a string (pStr)
...
parse(*pRS,pStr);
...
count++;
if(1000 == count)
{
count = 0;
delete pRS;
pRS = new RecordSet;
}
}

now, I only have a way that to solve memory increasing (but it is
time-consuming):

while(1)
{
RecordSet* pRS = new RecordSet;
//get a string (pStr)
...
parse(*pRS,pStr);
...
delete pRS;
}
}

you said "The allocated bar message will be deleted when the parent foo
message is
destroyed", for complex data Repeatedly clear() and parsefrom(), this
relationship can be still maintained?

[prot...@googlecode.com]

Comment #11 on issue 402 by liuj...@google.com: question for clear and parse
http://code.google.com/p/protobuf/issues/detail?id=402

Yes, but the increasing rate will decelerate, and there will be a virtual a
cap for the total memory usage (when every optional field is allocated a
message).

Yes, in the desctructor, it deletes all the sub message field, which will
invoke the destructor of those sub message type. It's basically a
recursively deletion in a message tree.

[prot...@googlecode.com]

Comment #12 on issue 402 by hlhhuang...@gmail.com: question for clear and
parse
http://code.google.com/p/protobuf/issues/detail?id=402

By the way, I have Commented out codes as much as possible, staying parse()
only.

[prot...@googlecode.com]

Comment #13 on issue 402 by hlhhuang...@gmail.com: question for clear and
parse
http://code.google.com/p/protobuf/issues/detail?id=402

"the increasing rate will decelerate", you are right. but using memory is
still a little large. and I uncertain whether it continues to increase.

[prot...@googlecode.com]

Updates:
Status: WorkingAsIntended

Comment #14 on issue 402 by liuj...@google.com: question for clear and parse
http://code.google.com/p/protobuf/issues/detail?id=402

My suggestion is that you test your memory usage for a long period of time
to find out the cap. If the cap is too large, then you could delete the
message in every N runs, just as what your code does.

Closing the bug now, since it's not a memory leak. Please email the
discussion group if you have further questions.

[prot...@googlecode.com]

Comment #15 on issue 402 by hlhhuang...@gmail.com: question for clear and
parse
http://code.google.com/p/protobuf/issues/detail?id=402

thank you every much !