On Wed, 13 Apr 2016, Юрий Соколов wrote:
> Hello, all.
> 
> I suggest couple of improvements to CrushWrapper.
> 
> Mostly safe change is replace std::map with btree::btree_map.
> It is straightforward change and it looks to be safe.

+1
 
> More complicated change is replacing std::string with custom allocated strings:
> - use custom allocator binded to CrushWrapper, - just a bump allocator
> with from list of blobs,
> - custom strings, used in CrushWrapper's maps are allocated from this
> allocator and never freed in one-by-one manner
> - custom data-structure for maps to be able to search by std::string
> (or tuned btree::btree_map)
> - for CrushWrapper's api results, newly allocated std::string are
> returned (with C++11 this is already a case, cause reference counting
> is forbidden)
> - whole allocator with its blobs is freed together with CrushWrapper.
> This approach is workable if and only if CrushWrapper is not changed
> after construction.
> I'm not too confident with Ceph sources to be sure about this property.

This is a lot more work, but might be worth it.  Most of the time the map 
*is* immutable.  And when it is updated, we do it by decoding a new 
version of the map...

> Is CrushWrapper "immutable" after it is fully constructed?

The problem is that we also have a bunch of mutators (set_*() methods) 
that are called in OSDMonitor.cc when making updates, and they need to be 
supported to.  I would figure out how to support those as well before 
moving forward.

It is probably worth doing some analysis to determine that these 
allocations are really that expensive, too.  Half of the allocations are 
for the map structs themselves, and those aren't going away.

sage