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: remove CephContext* from BmapEntry. #13651

Merged
merged 1 commit into from Mar 1, 2017

Conversation

Projects
None yet
5 participants
@rzarzynski
Contributor

rzarzynski commented Feb 25, 2017

Each BmapEntry instance stores a pointer to the same CephContext.
As we expect to have thousands of instances the overhead might
be too high. For example, serving 1 TiB SSD disk on x86-64,
while using the default settings, results in 32 MiB of extra
memory consumption:

  # assuming sizeof(unsigned long) * CHAR_BIT == 64
  >>> 1024 * 1024 * 1024 * 1024 / 4096 / 64
  4194304
  >>> 4194304 * 8 / 1024
  32768

Although memory is cheap, CPU's caches are not.

Signed-off-by: Radoslaw Zarzynski rzarzynski@mirantis.com

bluestore: remove CephContext* from BmapEntry.
Each BmapEntry instance stores a pointer to the same CephContext.
As we expect to have thousands of instances the overhead might
be too high. For instance, serving 1 TiB SSD disk on x86-64,
while using the default settings, results in 32 MiB of extra
memory consumption:

  # assuming sizeof(unsigned long) * CHAR_BIT == 64
  >>> 1024 * 1024 * 1024 * 1024 / 4096 / 64
  4194304
  >>> 4194304 * 8 / 1024
  32768

Although memory is cheap, CPU's caches are not.

Signed-off-by: Radoslaw Zarzynski <rzarzynski@mirantis.com>

@rzarzynski rzarzynski requested review from ifed01 and liewegas Feb 25, 2017

@liewegas liewegas changed the title from bluestore: remove CephContext* from BmapEntry. to os/bluestore: remove CephContext* from BmapEntry. Feb 26, 2017

@tchaikov

This comment has been minimized.

Show comment
Hide comment
@tchaikov

tchaikov Feb 27, 2017

Contributor
Consult 'bootstrap.log' for more details
CMake Error at /usr/share/cmake-2.8/Modules/FindBoost.cmake:1131 (message):
  Unable to find the requested Boost libraries.

  Unable to find the Boost header files.  Please set BOOST_ROOT to the root
  directory containing Boost or BOOST_INCLUDEDIR to the directory containing
  Boost's headers.
Call Stack (most recent call first):
  CMakeLists.txt:581 (find_package)

agent issue, retest this please.

Contributor

tchaikov commented Feb 27, 2017

Consult 'bootstrap.log' for more details
CMake Error at /usr/share/cmake-2.8/Modules/FindBoost.cmake:1131 (message):
  Unable to find the requested Boost libraries.

  Unable to find the Boost header files.  Please set BOOST_ROOT to the root
  directory containing Boost or BOOST_INCLUDEDIR to the directory containing
  Boost's headers.
Call Stack (most recent call first):
  CMakeLists.txt:581 (find_package)

agent issue, retest this please.

@tchaikov

This comment has been minimized.

Show comment
Hide comment
@tchaikov

tchaikov Feb 27, 2017

Contributor

retest this please.

Contributor

tchaikov commented Feb 27, 2017

retest this please.

@ifed01

ifed01 approved these changes Feb 27, 2017

@@ -123,8 +123,7 @@ void BmapEntry::_init_bit_mask()
BmapEntry::m_bit_mask_init = true;
}
BmapEntry::BmapEntry(CephContext* cct, bool full)
: cct(cct)
BmapEntry::BmapEntry(CephContext*, const bool full)

This comment has been minimized.

@ifed01

ifed01 Feb 27, 2017

Contributor

Suggest to remove CephContext parameter...

@ifed01

ifed01 Feb 27, 2017

Contributor

Suggest to remove CephContext parameter...

This comment has been minimized.

@rzarzynski

rzarzynski Mar 1, 2017

Contributor

You're right. I deferred the clean-up to not introduce dependencies between patches as I'm going to touch BitMapZone as well. Having CephContext* there costs us additional 2 MiB in the case of 1 TiB SSD. :-(

@rzarzynski

rzarzynski Mar 1, 2017

Contributor

You're right. I deferred the clean-up to not introduce dependencies between patches as I'm going to touch BitMapZone as well. Having CephContext* there costs us additional 2 MiB in the case of 1 TiB SSD. :-(

@markhpc markhpc added the performance label Mar 1, 2017

@liewegas liewegas merged commit ad5f382 into ceph:master Mar 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