New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

os/bluestore: memory and dereference clean-up in the BitAllocator #13811

Merged
merged 6 commits into from Apr 1, 2017

Conversation

Projects
None yet
3 participants
@rzarzynski
Contributor

rzarzynski commented Mar 6, 2017

No description provided.

Show outdated Hide outdated src/os/bluestore/BitAllocator.cc
}
BitMapAreaList *list = new BitMapAreaList(children, num_child);
BitMapAreaList *list = new BitMapAreaList(std::move(children));

This comment has been minimized.

@ifed01

ifed01 Mar 6, 2017

Contributor

By 'typedefing' BitMapAreaList (see below) you might store items directly to this container instead of using interim children here and throughout..

@ifed01

ifed01 Mar 6, 2017

Contributor

By 'typedefing' BitMapAreaList (see below) you might store items directly to this container instead of using interim children here and throughout..

Show outdated Hide outdated src/os/bluestore/BitAllocator.h
@@ -246,26 +246,20 @@ class BitMapArea {
class BitMapAreaList {
private:
BitMapArea **m_items;
int64_t m_num_items;
std::vector<BitMapArea*> m_items;

This comment has been minimized.

@ifed01

ifed01 Mar 6, 2017

Contributor

Such an implementation (BitMapAreaList encapsulating a vector) seems excessive to me. IMO it brings nothing usefull.
What's about making a typedef and cleanup corresponding access points, e.g. get_nth_item?

@ifed01

ifed01 Mar 6, 2017

Contributor

Such an implementation (BitMapAreaList encapsulating a vector) seems excessive to me. IMO it brings nothing usefull.
What's about making a typedef and cleanup corresponding access points, e.g. get_nth_item?

This comment has been minimized.

@liewegas

liewegas Mar 15, 2017

Member

I agree that using a straight std::vector (even without a typedef) is simpler and makes the code easier to understand.

@liewegas

liewegas Mar 15, 2017

Member

I agree that using a straight std::vector (even without a typedef) is simpler and makes the code easier to understand.

This comment has been minimized.

@rzarzynski

rzarzynski Mar 28, 2017

Contributor

Guys, terribly sorry for lacking a comment with justification. The BitMapAreaList has been kept due to next part of BitMap allocator rework I will make. The idea is to have BitMapAreaLeaf aggregating BitMapZones directly (not pointers to them) while, in general, share the code with BitMapAreaIN and similar. Hopefully templatizing BitMapAreaList will provide such possibility.

@rzarzynski

rzarzynski Mar 28, 2017

Contributor

Guys, terribly sorry for lacking a comment with justification. The BitMapAreaList has been kept due to next part of BitMap allocator rework I will make. The idea is to have BitMapAreaLeaf aggregating BitMapZones directly (not pointers to them) while, in general, share the code with BitMapAreaIN and similar. Hopefully templatizing BitMapAreaList will provide such possibility.

@liewegas

This comment has been minimized.

Show comment
Hide comment
@liewegas

liewegas Mar 29, 2017

Member

needs rebase

Member

liewegas commented Mar 29, 2017

needs rebase

rzarzynski added some commits Mar 1, 2017

os/bluestore: move the CephContext* from BitMapArea to BitAllocator.
Saves over 2 MiB on x86-64 running 1 TiB with the default configuration.

Signed-off-by: Radoslaw Zarzynski <rzarzynski@mirantis.com>
os/bluestore: remove the unused BitMapArea::m_type and related stuff.
Signed-off-by: Radoslaw Zarzynski <rzarzynski@mirantis.com>
os/bluestore: remove unused BitMapAreaList::m_marker_mutex.
Signed-off-by: Radoslaw Zarzynski <rzarzynski@mirantis.com>
os/bluestore: BitMapAreaList became a thin wrapper over std::vector.
Signed-off-by: Radoslaw Zarzynski <rzarzynski@mirantis.com>
os/bluestore: remove yet another indirection in BitMapAreaIN::m_child…
…_list.

Signed-off-by: Radoslaw Zarzynski <rzarzynski@mirantis.com>
os/bluestore: drop unused & duplicated BitMapAreaIN::m_num_child.
Signed-off-by: Radoslaw Zarzynski <rzarzynski@mirantis.com>
@rzarzynski

This comment has been minimized.

Show comment
Hide comment
@rzarzynski

rzarzynski Mar 29, 2017

Contributor

@liewegas: rebased. The conflicts were trivial and related to the override-fixing.

Contributor

rzarzynski commented Mar 29, 2017

@liewegas: rebased. The conflicts were trivial and related to the override-fixing.

@liewegas liewegas merged commit c4ba267 into ceph:master Apr 1, 2017

3 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
default Build finished.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment