Skip to content
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

LZMA threads #95

Merged
merged 8 commits into from May 28, 2019

Conversation

@jbonyun
Copy link
Contributor

commented May 26, 2019

Enables multithreaded LZMA compression in Boost.iostreams.

I have tested the build and behavior on a range of Ubuntu and RHEL systems. I have tested the build and behavior of all numbered liblzma release versions in their git repo.

Unit testing doesn't cover:

  • That multiple threads are actually used when appropriate. Because how would I do that in a unit test?
  • Behavior on larger amounts of data (which liblzma deploys more threads on). Because it would slow down the execution of tests significantly.
  • That the build correctly detects liblzma's capabilities. Because this is a build issue.
  • That the build correctly uses BOOST_IOSTREAMS_LZMA_NO_MULTITHREADED macro. Because this is a build issue.
jbonyun added 7 commits May 22, 2019
Use default constructor of lzma_params to choose default values
This allows the setting of defaults to happen all in one place,
instead of being spread across multiple places.
Factor out init_stream to reduce code duplication
There was identical code running in reset and do_init
functions. We centralize that. Which will make future
changes easier.
Add threads member to lzma_params
It is ignored in this commit, but this enables use of the new
API, and testing of the new API.
Add multithreaded_test to lzma_test
Verifies that various number of threads arrive at the
same compressed data. While it's doing that, it verifies
that various constructors compile and function properly.

It does not test that threading is actually occurring. That
would be challenging. And the correct behavior is not to
use multiple threads, if liblzma doesn't have that
capability.
Use multithreaded versions of liblzma functions
The previous constructor for the filter continues to work as
before. But you can now specify a threads value in the
lzma_mt parameter struct. Default value is 1, meaning
single-threaded.

If you ask for 0 threads, it will automatically detect the number
of cores and use that. To accomplish this, we leverage this capability
in liblzma already.

Liblzma versions before 5.2.0 didn't have multithreading
capabilities and didn't have the necessary functions. So we detect
the liblzma version using the LZMA_VERSION macro, and fallback to
the previous single-threaded implementation when necessary.

If liblzma was built with --disable-threads or --enable-threads=no,
then liblzma will not have the required multithreaded functions,
even if the version is sufficiently recent. We have no way to
detect this case at compile-time (no macro in liblzma's headers)
and it will lead to link-time errors. So we provide the
capability to disable the new multithreaded implementation by
defining BOOST_IOSTREAMS_LZMA_NO_MULTITHREADED.
@jbonyun

This comment has been minimized.

Copy link
Contributor Author

commented May 26, 2019

I'm happy for these commits to be squashed, if that's the preference of the maintainers. I tried to keep individual steps for easier review.

I'm also happy to hear more suggestions for unit tests. I've exercised the basic functionality and the various constructors/initializers.

@swatanabe

This comment has been minimized.

Copy link
Collaborator

commented May 26, 2019

@jeking3

This comment has been minimized.

Copy link
Collaborator

commented May 26, 2019

iostreams supports C++03 or later

@jbonyun

This comment has been minimized.

Copy link
Contributor Author

commented May 26, 2019

Very well, I can change that to be spelled out over 8 lines. It wasn't a syntax I was accustomed to either. It does build with all versions of gcc, intel, clang that I tried, for what it's worth. But I understand you'd like to stay within the standard.

@jbonyun

This comment has been minimized.

Copy link
Contributor Author

commented May 26, 2019

@swatanabe Actually, were you suggesting an initializer list without the "designated initializer" part of it? Or were you suggesting a one-member-at-a-time assignment? The first is concise, but I feel obfuscates. The second is verbose.

Initializer list:

const lzma_mt opt = { 0, threads_, 0, 1000, level_, nullptr, LZMA_CHECK_CRC32 };

One-member-at-a-time:

lzma_mt opt;                                                                                      
opt.flags = 0;                                                                                    
opt.threads = threads_;                                                                           
opt.block_size = 0;                                                                               
opt.timeout = 1000;                                                                               
opt.preset = level_;                                                                              
opt.filters = nullptr;                                                                            
opt.check = LZMA_CHECK_CRC32;

I'm happy to take the preference of the regular maintainers.

@jbonyun

This comment has been minimized.

Copy link
Contributor Author

commented May 26, 2019

I was feeling bad for not having updated the documentation... but lzma isn't really documented in iostreams in the first place. If it's alright with you guys, I'd rather leave that for now.

I imagine somebody ought to write that. The bzip2 stuff would be a good starting point to copy from. Maybe it can be an additional project I take on, after this.

@pdimov

This comment has been minimized.

Copy link
Contributor

commented May 26, 2019

const lzma_mt opt = { 0, threads_, 0, 1000, level_, nullptr, LZMA_CHECK_CRC32 }; should be fine, except that nullptr is also C++11.

Jeff Bonyun
Correct struct initialization to be C++03
- Designated initializers aren't standard C++ until C++20 (or
something).
- nullptr is C++11.
@jbonyun

This comment has been minimized.

Copy link
Contributor Author

commented May 26, 2019

Yep, nullptr is what broke the Appveyor build for Windows. And I was so proud of myself for using modern stuff!

I've pushed an update for nullptr and designated initializers. I would recommend that be squashed before merge, but I didn't want to force push if that's not the preferred approach.

@jbonyun

This comment has been minimized.

Copy link
Contributor Author

commented May 27, 2019

So... can anyone interpret that codecov.io break? Something related to my changes? It looks, to me, that it has to do with a gzip module for codecov not being available...

@jeking3

This comment has been minimized.

Copy link
Collaborator

commented May 27, 2019

Looks like something was missing from the build environment to run lcov. Perhaps lcov changed recently. I maintain the build wrapper around that. I'll take a look this week.

@jeking3
Copy link
Collaborator

left a comment

Missing documentation. I think I saw an email thread about doing that some other time since there is no documentation for lzma usage right now?

@@ -75,7 +82,7 @@ void lzma_error::check BOOST_PREVENT_MACRO_SUBSTITUTION(int error)
namespace detail {

lzma_base::lzma_base()
: stream_(new lzma_stream), level(lzma::default_compression)
: stream_(new lzma_stream), level_(lzma::default_compression), threads_(1)

This comment has been minimized.

Copy link
@jeking3

jeking3 May 28, 2019

Collaborator

I thought the default would be 0, meaning use as many cores as possible?

This comment has been minimized.

Copy link
@jbonyun

jbonyun May 28, 2019

Author Contributor

That would work, with just a change where you commented. I was trying to avoid surprising the users by changing the default behavior. If you think that 0 is the right answer, I can make that change.

This comment has been minimized.

Copy link
@jbonyun

jbonyun May 28, 2019

Author Contributor

Correction: a change here would be ignored. threads_ should always be overwritten by whatever is passed in lzma_params, even if that's the default constructed version of lzma_params. I would change both to 0.

@jbonyun

This comment has been minimized.

Copy link
Contributor Author

commented May 28, 2019

@jeking3 Re documentation, lzma isn't documented in iostreams at all, so there was nothing to update. I was proposing that adding lzma documentation be a separate step. One which I would consider doing, while it's fresh in my mind.

@swatanabe

This comment has been minimized.

Copy link
Collaborator

commented May 28, 2019

@jeking3

This comment has been minimized.

Copy link
Collaborator

commented May 28, 2019

When BOOST_IOSTREAMS_LZMA_NO_MULTITHREADED is present, a non-mt encoder is used, and threads_ is ignored. Shouldn't the default behavior be threads_(0), which means "use as many threads as advertised cores"? If we make the default 1, nobody will get the benefit until they specifically enable it in their projects.

@codecov

This comment has been minimized.

Copy link

commented May 28, 2019

Codecov Report

❗️ No coverage uploaded for pull request base (develop@7c627be). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##             develop      #95   +/-   ##
==========================================
  Coverage           ?   68.96%           
==========================================
  Files              ?       80           
  Lines              ?     3444           
  Branches           ?     1027           
==========================================
  Hits               ?     2375           
  Misses             ?      452           
  Partials           ?      617
Impacted Files Coverage Δ
src/lzma.cpp 90.9% <100%> (ø)
include/boost/iostreams/filter/lzma.hpp 68.08% <100%> (ø)

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 7c627be...6fa2d8d. Read the comment docs.

@jbonyun

This comment has been minimized.

Copy link
Contributor Author

commented May 28, 2019

@jeking3 Assuming that BOOST_IOSTREAMS_LZMA_NO_MULTITHREADED will not be defined in the default build of Boost, I think that's the same as saying 0 is the default number of threads. Because just changing the number of threads in the user's constructor is easier than defining the macro and recompiling Boost. The macro will/should only be used if Boost won't compile because --disable-threads has been used on their libxzma or if, for some reason, the multithreaded API to libxzma has broken.

I see @swatanabe's point that in a program where threading has been explicitly managed by the designer, suddenly using all the threads could be detrimental. I see @jeking3's point that we should give the extra speed to users who don't want to put extra thought into it.

My instinct was not to change the default behavior and therefore to use one thread by default. But we're back to being above my pay grade. I will be happy with either option, but don't feel that I'm qualified to resolve the difference of opinion.

@swatanabe

This comment has been minimized.

Copy link
Collaborator

commented May 28, 2019

@jeking3
Copy link
Collaborator

left a comment

Given the discussion, default behavior will be single threaded unless someone takes the time to make it use more cores.

@jeking3 jeking3 merged commit 06cf1df into boostorg:develop May 28, 2019

4 checks passed

codecov/patch No report found to compare against
Details
codecov/project No report found to compare against
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.