@Trinkle23897 Could you please help me check if it is possible to separate of stack option and other abilities of ReplayBuffer? I have not done that part. And it is a little diffcult for me because I'm not familar with RNN stack usage. Thanks a lot.

Update: resolved

@duburcqa I have some trouble with the new buffer type's pickle. (I think the current code is clean but lack of unit test, will be fixed soon...)

Because the CachedReplayBuffer needs to perform getitem from many sub-buffers, if each sub-buffer's _meta is a separated batch (no memory contiguous), it will first getitem then Batch.cat all of the data, which may cause a lot of overhead. To avoid this, our approach is to create _meta on the top, and create batch slice and send to the sub-buffers by set_batch.

Here is an example: we now have a buffer that contains three sub-buffers, with each of size=10 (total=30). At this time, we add a new key-value pair called info["Timelimit.truncate"]: bool. It will allocate new memory as follows:

Step 1: one sub-buffer's `_add_to_buffer` will raise exception, and will call a hook function `_alloc` to
        require new memory for this key;
Step 2: The top of this buffer class receives this signal, and generate `np.zeros(30, np.bool_)` to 
        `top_buffer._meta.info["Timelimit.truncate"]`;
Step 3: Dispatch the whole `top_buffer._meta` to sub-buffers, i.e., 
        `subbuffer[0].set_batch(top_buffer._meta[0:10])`

The current implementation use alloc_fn hook as an input argument, but in VectorReplayBuffer and CachedReplayBuffer, this approach is not compatible with pickle, because the user-defined hook cannot be pickled. You can run the script under test/base/test_buffer.py to see this issue.

I haven't figure out the solution to this issue, could you please give us some suggestions?

Codecov Report

Merging #278 (b9385d8) into master (1eb6137) will increase coverage by 0.24%.
The diff coverage is 99.57%.

@@            Coverage Diff             @@
##           master     #278      +/-   ##
==========================================
+ Coverage   94.39%   94.64%   +0.24%     
==========================================
  Files          45       45              
  Lines        2892     3006     +114     
==========================================
+ Hits         2730     2845     +115     
+ Misses        162      161       -1     

Flags with carried forward coverage won't be shown. Click here to find out more.

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1eb6137...b9385d8. Read the comment docs.

If you think it is a special case, your workaround is fine

In my current thought it is a special case. I haven't come up with other demands or requirements similar to alloc_fn.

In my current thought it is a special case. I haven't come up with other demands or requirements similar to alloc_fn.

Why not then ! But may be the name is not very appropriate if it is a instance method (supposedly private) instead of a hook ?

commit 425c2bd made the speed from 1980 to 1900

What are you using for profiling ? Personally I'm now using py-spy and it is just mesmerising (open the interactive svg in webbrowser)

py-spy record --native -o profiling.svg -- python3 bench.py

I remove @staticmethod in the previous implementation and overwrite the sub-buffer's alloc_fn in the initialization of ReplayBuffers

Hum... that's look kind of hacky but not meaningless actually. Is it still pickle correctly after that change at instance-level ?

Hum... that's look kind of hacky but not meaningless actually.

I think so, but I haven't come up with other approaches better than this. (compatible with pickle)

Is it still pickle correctly after that change at instance-level ?

Sure. I add the related test under test_hdf5

But may be the name is not very appropriate if it is a instance method (supposedly private) instead of a hook ?

What's your suggestion? (Also I'm not sure if the name ReplayBuffers is good enough since it is only one character different than ReplayBuffer, but changing its name to ReplayBufferList will be further misleading because we already have ListReplayBuffer)

I'm not sure if the name ReplayBuffers is good enough since it is only one character different than ReplayBuffer, but changing its name to ReplayBufferList will be further misleading because we already have ListReplayBuffer

What about ReplayBufferManager, ReplayBufferSupervisor, ReplayBufferOrchestrator, ReplayBufferDispatcher ...

What's your suggestion?

I don't know, maybe just _buffer_allocator or _buffer_dynamic_allocator.

What are you using for profiling ?

Your profiling graph is polluted by non-significant method calls (such as import.load modules), you should use a script running for a longer duration.

I was using this tool before, but I find it less readable than py-spy at the end. But both are good :)

py-spy result profiling.zip

I can't find the part related to replaybuffer. Is there anything wrong?

I can't find the part related to replaybuffer. Is there anything wrong?

And yet it is here! Just use the search functionality integrated in the svg itself, and look for buffer.py

By looking at the time spend in this module, I don't think it is relevant to optimize it further x)

NB: Just disable --native option to avoid showing time spend in torch if you prefer.

I think cachedbuffer is ready now.

@duburcqa There is a question that makes me confused.

import numpy as np
from tianshou.data import ReplayBuffer, CachedReplayBuffer
buf = CachedReplayBuffer(ReplayBuffer(10000), 1, 10000)
buf.add(obs=[np.random.rand(4, 84, 84)], act=[1], rew=[1], done=[1])
buf = CachedReplayBuffer(ReplayBuffer(10000), 1, 10000)
buf.add(obs=[np.random.rand(4, 84, 84)], act=[1], rew=[1], done=[1])

Repeating executing the last two lines, you'll see that the actual memory is still increasing. That's weird because theoretically the reference count of previous buf = CachedReplayBuffer(...) is 0. However it seems like it doesn't do the garbage collection. Do you have any idea?

What about this ?

import gc
import numpy as np
from tianshou.data import ReplayBuffer, CachedReplayBuffer
for _ in range(100):
    buf = CachedReplayBuffer(ReplayBuffer(10000), 1, 10000)
    buf.add(obs=[np.random.rand(4, 84, 84)], act=[1], rew=[1], done=[1])
    gc.collect()

If there is an actual memory leak, you should be able to find the line that is responsible of it using memory_profiler. I'm using it frequently and it is quite good, even though time consuming to use. mprof especially is pretty useful.

gc.collect() works fine. Thanks~