Limit number of messages in mempool, instead of total byte sizes #2073

objmagic · 2017-07-17T00:58:01Z

This PR limits size of memory pool by limiting number of messages in each type of memory pool, instead of bytes used.

Motivation

In the past two weeks, team at Twitter was investigating huge memory usage of stream manager of one topology. We printed out number of messages in memory pool, and sum of messages' size using SpaceUsed instead of sizeof. Thanks @ttim for pointing this out and I've confirmed with @congwang as well. We saw memory pool usage of HeronTupleSet2 can reach more than 70MB. This confirms that using sizeof is wrong, and if we want to limit memory pool usage, we should use SpaceUsed instead. To be more specific here, google::protobuf::Message allocates memory on heap, and has a fairly complex structure, which is why we should use SpaceUsed provided by library to know its size, instead of sizeof.

Why not using `SpaceUsed`

As documentation of SpaceUsed points out, each call can be quite expensive. Of course, one can argue to use a thread to update mempool usage continuously, but there will be thread-safety issue and seems unnecessarily complicated.

Why use `size` directly

This PR simply limits number of messages in each memory pool. Advantage here is that implementation is simple and release call can finish in O(1).

Why set maximum to be 512

Because based on what observed from the topology at Twitter, we see max number of messages in memory pool can be around ~630. It is reasonable to limit pool size to be 512. I am thinking to limit the size more here. Maybe 256? Please comment.

Side notes

Message->Clear() does not shrink Message. See here
Why Message can become bloated gradually? See here

This should resolve #1567 (cc @ajorgensen )

srkukarni · 2017-07-17T06:34:55Z

In principle I like this change, however I think you should add a heron internals config variable to control this parameter?

objmagic · 2017-07-17T17:57:43Z

@srkukarni Thanks. Add configuration variable takes time because there is circular dependency problems.

billonahill · 2017-07-17T18:02:36Z

heron/common/src/cpp/basics/mempool.cpp

@@ -21,5 +21,5 @@

 // TODO(nlu): get the pool size limit from config
 MemPool<google::protobuf::Message>* __global_protobuf_pool__ =
-                                   new MemPool<google::protobuf::Message>(50 * 1024 * 1024);
+                                   new MemPool<google::protobuf::Message>(512);


this review looks like you're just calculating size of the pool differently, but what causes the pool size logic to change from being based on number of bytes to number of message in the pool impl?

This is explained above: #2073 (comment)

Got it. Looks like your changing the 512 to be configurable. Shouldn't the config-driven value be used here now?

it is config driven. The initial value is 512, and will be overridden here: https://github.com/twitter/heron/pull/2073/files#diff-5ff5ed00c50afd2302f522a9181e50a1R90

As for why initial value cannot be read from config, see the discussion between Sanjeev and I about circular dependency.

Got it. Could you update the TODO nlu comment above to explain why this is config-driven, but hard coded here.

@billonahill Updated here: 01acca8

srkukarni · 2017-07-17T18:27:27Z

@objmagic could you please elaborate what these cir dep are?

objmagic · 2017-07-17T18:30:49Z

@srkukarni config-cxx -> network-cxx -> basics-cxx. Inside basics-cxx, mempool.h/cpp needs to use heron-internals-config-reader., which is in config-cxx. Thus circular dependency.

objmagic · 2017-07-17T18:34:59Z

@srkukarni I have added a method to set limit of memory pool externally. We then set limit of memory pool in stream manager's Init. So we can avoid the circular dependency problem here.

srkukarni · 2017-07-17T18:54:57Z

There might be ways to avoid this. One way is to actually send this parameter as part of the init method. Or have an explicit InitBuffer pool method inside main of stmgr/tmaster.

srkukarni · 2017-07-17T18:56:47Z

@objmagic Sorry, looks like our comments crossed each other. I see that you have done it. Let me take a look at this. Thanks!

nlu90 · 2017-07-17T21:40:11Z

heron/config/src/yaml/conf/yarn/heron_internals.yaml

@@ -65,6 +65,9 @@ heron.streammgr.cache.drain.size.mb: 100
 # For efficient acknowledgements
 heron.streammgr.xormgr.rotatingmap.nbuckets: 3

+# The max size of the memory pool for any type of message
+heron.streammgr.mempool.size.mb: 512


@objmagic

Is this config for limiting the # of messages in the mempool or the total byte size in this mempool? This config is different than the other above 9 config.

Could you make them consistent and update the comment and even the config name to avoid confusion?

oh you are right. this is typo.

nlu90

please check my above comments

congwang · 2017-07-17T23:23:39Z

This is still ugly. The memory leaks are leaks, adding a limit is just adding a cap to these leaks, doesn't resolve them at all. How would you expect users to figure out how many messages they actually need?

The real solution here is probably to explore the new protobuf arena API to see how we can integrate it with mempool. But that requires protobuf 3.0 at least, fortunately I already have this here: https://github.com/twitter/heron/tree/cwang/protobuf30

srkukarni · 2017-07-17T23:33:56Z

@congwang unless I'm mistaken, afaik, we are not talking about memory leak here. Its just about restricting the size of mempool from growing and remaining high for a long time.

objmagic · 2017-07-18T00:19:47Z

@congwang I agree. This solution does not look nice to me either.

@srkukarni @congwang I have a theory about the cause of "memory leak". If one does not call Clear on Message, used space in Message will not be zero'ed out. So next time we add tuple in it, Message thinks it does not have enough space and will enlarge the strings in itself. C++ STL will reserve extra space in string when you append (just like how it uses some heuristics to reserve extra space for vector). Thus we are always allocating extra memory here.

If my theory is correct, #2057 should solve the issues, along with this PR.

See our heap leak check graph below and backtrace we get from core dump for details.

srkukarni · 2017-07-18T05:07:42Z

@objmagic Thats not true. If you notice, acquire does a Clear before giving the proto structure to the client, so Clear is called.
You can do one expt. Remove the Clear from acquire to release.

ttim · 2017-07-18T05:27:39Z

@srkukarni @objmagic protobuf documentation says about this (https://developers.google.com/protocol-buffers/docs/cpptutorial, Optimization Tips):

Reuse message objects when possible. Messages try to keep around any memory they allocate for reuse, even when they are cleared. Thus, if you are handling many messages with the same type and similar structure in succession, it is a good idea to reuse the same message object each time to take load off the memory allocator. However, objects can become bloated over time, especially if your messages vary in "shape" or if you occasionally construct a message that is much larger than usual. You should monitor the sizes of your message objects by calling the SpaceUsed method and delete them once they get too big.

So even if you do Clear you still have some kind of memory leak (and therefore assumption @objmagic made still make sense).

Having limited number of objects in pool can fix this issue thou: they will be rotated over the time so most likely each message eventually will be deleted. Having big enough pool at the same time almost guarantee all messages will only become bigger and memory will keep increasing.

objmagic · 2017-07-18T17:03:25Z

@srkukarni You are right. Before your PR, Clear is already called properly for all messages.

…che#2073)

Limit number of messages.

4237907

objmagic added bug stmgr labels Jul 17, 2017

objmagic added this to the 0.15.0 milestone Jul 17, 2017

objmagic requested review from billonahill, srkukarni, nlu90 and huijunw July 17, 2017 00:58

billonahill reviewed Jul 17, 2017

View reviewed changes

objmagic added 2 commits July 17, 2017 11:31

Set num via configs.

c8705b9

Revert chagned config/BUILD.

fa85b19

srkukarni approved these changes Jul 17, 2017

View reviewed changes

Add comment.

01acca8

huijunw approved these changes Jul 17, 2017

View reviewed changes

nlu90 reviewed Jul 17, 2017

View reviewed changes

nlu90 suggested changes Jul 17, 2017

View reviewed changes

Remove .mb

47c2b3c

billonahill approved these changes Jul 17, 2017

View reviewed changes

objmagic added 3 commits July 17, 2017 15:55

Change names.

8effc7c

Remove .ds_store

13f6669

More name changes.

f1b9527

nlu90 approved these changes Jul 17, 2017

View reviewed changes

objmagic merged commit 8fc4455 into apache:master Jul 19, 2017

huijunw pushed a commit that referenced this pull request Aug 1, 2017

Limit number of messages in mempool, instead of total byte sizes (#2073)

1ae7d9b

huijunw pushed a commit that referenced this pull request Aug 2, 2017

Limit number of messages in mempool, instead of total byte sizes (#2073)

3022a90

objmagic added a commit to objmagic/heron that referenced this pull request Aug 8, 2017

Limit number of messages in mempool, instead of total byte sizes (apa…

442df0b

…che#2073)

objmagic mentioned this pull request Aug 23, 2017

mempool growth issue #2234

Open

nicknezis pushed a commit that referenced this pull request Sep 14, 2020

Limit number of messages in mempool, instead of total byte sizes (#2073)

4f0cd8c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Limit number of messages in mempool, instead of total byte sizes #2073

Limit number of messages in mempool, instead of total byte sizes #2073

objmagic commented Jul 17, 2017 •

edited

Loading

srkukarni commented Jul 17, 2017

objmagic commented Jul 17, 2017

billonahill Jul 17, 2017

objmagic Jul 17, 2017

billonahill Jul 17, 2017

objmagic Jul 17, 2017

billonahill Jul 17, 2017

objmagic Jul 17, 2017

srkukarni commented Jul 17, 2017

objmagic commented Jul 17, 2017 •

edited

Loading

objmagic commented Jul 17, 2017 •

edited

Loading

srkukarni commented Jul 17, 2017

srkukarni commented Jul 17, 2017

nlu90 Jul 17, 2017 •

edited

Loading

objmagic Jul 17, 2017

nlu90 left a comment

congwang commented Jul 17, 2017

srkukarni commented Jul 17, 2017

objmagic commented Jul 18, 2017 •

edited

Loading

srkukarni commented Jul 18, 2017

ttim commented Jul 18, 2017 •

edited

Loading

objmagic commented Jul 18, 2017

Limit number of messages in mempool, instead of total byte sizes #2073

Limit number of messages in mempool, instead of total byte sizes #2073

Conversation

objmagic commented Jul 17, 2017 • edited Loading

Motivation

Why not using SpaceUsed

Why use size directly

Why set maximum to be 512

Side notes

srkukarni commented Jul 17, 2017

objmagic commented Jul 17, 2017

billonahill Jul 17, 2017

Choose a reason for hiding this comment

objmagic Jul 17, 2017

Choose a reason for hiding this comment

billonahill Jul 17, 2017

Choose a reason for hiding this comment

objmagic Jul 17, 2017

Choose a reason for hiding this comment

billonahill Jul 17, 2017

Choose a reason for hiding this comment

objmagic Jul 17, 2017

Choose a reason for hiding this comment

srkukarni commented Jul 17, 2017

objmagic commented Jul 17, 2017 • edited Loading

objmagic commented Jul 17, 2017 • edited Loading

srkukarni commented Jul 17, 2017

srkukarni commented Jul 17, 2017

nlu90 Jul 17, 2017 • edited Loading

Choose a reason for hiding this comment

objmagic Jul 17, 2017

Choose a reason for hiding this comment

nlu90 left a comment

Choose a reason for hiding this comment

congwang commented Jul 17, 2017

srkukarni commented Jul 17, 2017

objmagic commented Jul 18, 2017 • edited Loading

srkukarni commented Jul 18, 2017

ttim commented Jul 18, 2017 • edited Loading

objmagic commented Jul 18, 2017

objmagic commented Jul 17, 2017 •

edited

Loading

Why not using `SpaceUsed`

Why use `size` directly

objmagic commented Jul 17, 2017 •

edited

Loading

objmagic commented Jul 17, 2017 •

edited

Loading

nlu90 Jul 17, 2017 •

edited

Loading

objmagic commented Jul 18, 2017 •

edited

Loading

ttim commented Jul 18, 2017 •

edited

Loading