Add option to set compression threads priority #1457

jmachowinski · 2023-09-04T12:21:34Z

Added option to the the nice value of the compression thread.

arneboe · 2023-09-05T13:23:11Z

@emersonknapp Could you review this (or find someone to to it)? This is an important feature that enables us to use rosbag2 in production environments without grinding the whole system to a halt when writing large bag files.

rosbag2_compression/src/rosbag2_compression/sequential_compression_writer.cpp

MichaelOrlov

@jmachowinski Thank you for your contribution.
I think we can move forward with this PR and the proposed approach but need to address some minor issues and add missing parts in the dependent rosbag2 packages.

In particular, will need to:

update rosbag2_py package with the new data structure field.
update rosbag2_transport::RecordOptions package with the new data structure field.
update ros2bag/verb/record.py with a new command line option for compression threads priority value.
Consider if it will be possible to add some unit tests to check if threads priority sets up as expected.

I will doublecheck yet Windows implementation and let you know if changes are needed.

rosbag2_compression/src/rosbag2_compression/sequential_compression_writer.cpp

jmachowinski · 2023-09-12T09:13:39Z

In particular, will need to:

1. update `rosbag2_py` package with the new data structure field.

2. update `rosbag2_transport::RecordOptions` package with the new data structure field.

3. update `ros2bag/verb/record.py` with a new command line option for compression threads priority value.

4. Consider if it will be possible to add some unit tests to check if threads priority sets up as expected.

The python stuff is nothing, were a commit from ME would meet any quality standards... @kohrt can you have a look at this ?

fujitatomoya

besides all comments, and

Manual page getpriority(2)
...
BUGS
       According  to POSIX, the nice value is a per-process setting.  However, under the current Linux/NPTL implementation of
       POSIX threads, the nice value is a per-thread attribute: different threads in the same process can have different nice
       values.   Portable  applications should avoid relying on the Linux behavior, which may be made standards conformant in
       the future.

IMO, i am not inclined to take this fix.

related REP(ROS Enhancement Proposal): ros-infrastructure/rep#385

rosbag2_compression/include/rosbag2_compression/compression_options.hpp

rosbag2_compression/src/rosbag2_compression/sequential_compression_writer.cpp

jmachowinski · 2023-09-14T08:35:16Z

2. update `rosbag2_transport::RecordOptions` package with the new data structure field.

Done

4. Consider if it will be possible to add some unit tests to check if threads priority sets up as expected.

Added a test case

MichaelOrlov · 2023-09-15T08:11:12Z

@jmachowinski Could you please rename compression_threads_nice_value parameter to something more generic like
compression_threads_priority and update descriptions in the header files?
Since on Windows we don't have such entities as nice value we do really have a thread priority.
I remember I've added a comment about it but it seems my comment disappeared in Github after you made a git push with --force parameter.

rosbag2_compression/src/rosbag2_compression/sequential_compression_writer.cpp

MichaelOrlov

@jmachowinski The selected approach for tests is not going to work. The test shall be rewritten.
See my comments in code.
I would propose creating a regular non-fake sequential_compression_writer and capture std::err output to verify logged error messages.
Something like

auto writer = ReaderWriterFactory::make_writer(record_options);
testing::internal::CaptureStderr();
writer->open(zero_cache_storage_options);
std::string test_output = testing::internal::GetCapturedStderr();
EXPECT_TRUE(
  test_output.find(
    "Could not set priority for compression thread to'") == std::string::npos);

and make two test cases with valid thread priority and invalid thread priority values.

Also please rename compression_threads_nice_value and thread_nice_value parameters to the compression_threads_priority and thread_priority respectively.

rosbag2_compression/test/rosbag2_compression/test_sequential_compression_writer.cpp

jmachowinski · 2023-09-18T13:02:14Z

@jmachowinski Could you please rename compression_threads_nice_value parameter to something more generic like compression_threads_priority and update descriptions in the header files? Since on Windows we don't have such entities as nice value we do really have a thread priority. I remember I've added a comment about it but it seems my comment disappeared in Github after you made a git push with --force parameter.

A priority is something special in the linux world, therefore I would not name it that.

MichaelOrlov · 2023-09-21T06:38:35Z

@jmachowinski As regards

A priority is something special in the linux world, therefore I would not name it that.

The thread priority is a generic term and will be applicable to the cross-platform implementation.
The fact that in the current implementation, the thread priority on Unix/Linux platforms will be settling up via nice value is an implementation details and would be considered as temporary workaround until [REP-2017] Thread attributes configuration support in rcl #385 will not be fully implemented.
Eventually, we will replace part of the code that is setting/getting nice value to the API from the [REP-2017]. But what is important is that the variables and parameter's names shall be agnostic to this temporary workaround.

jmachowinski · 2023-09-22T15:00:21Z

@MichaelOrlov
You latest changes trigger this warning :
Warning: class_loader.impl: SEVERE WARNING!!! A namespace collision has occurred with plugin factory for class FakeCompressor. New factory will OVERWRITE existing one. This situation occurs when libraries containing plugins are directly linked against an executable (the one running right now generating this message). Please separate plugins out into their own library or just don't link against the library and use either class_loader::ClassLoader/MultiLibraryClassLoader to open.

I suggest we move the check back into a class inside of the test function.

jmachowinski · 2023-09-22T15:03:21Z

@jmachowinski Could you please rename compression_threads_nice_value parameter to something more generic like compression_threads_priority

Done

MichaelOrlov · 2023-09-22T16:17:32Z

I suggest we move the check back into a class inside of the test function.

Hold on let me double-check. We didn't change the way how the library was linked.
This warning might have already existed before.

jmachowinski · 2023-09-22T16:39:00Z

I suggest we move the check back into a class inside of the test function.

Hold on let me double-check. We didn't change the way how the library was linked. This warning might have already existed before.

You added test/rosbag2_compression/fake_compressor.cpp to test_sequential_compression_writer this, triggers the warning

MichaelOrlov · 2023-09-22T23:24:30Z

@jmachowinski You can disregard this warning.
I made some investigation and found out that the case about what this warning signaling is safe to be used in this particular scenario.
I would prefer to have a such runtime warning in one test rather than duplicated class for fake_compressor.

If this warning is really bothering you and you want to get rid of it you can fix it by specifying "zstd" compression format instead of the DefaultTestCompressor in the open_succeeds_on_supported_compression_format test.

rosbag2/rosbag2_compression/test/rosbag2_compression/test_sequential_compression_writer.cpp

Lines 195 to 199 in 96c11b0

    
           TEST_F(SequentialCompressionWriterTest, open_succeeds_on_supported_compression_format) 
        
           { 
        
             rosbag2_compression::CompressionOptions compression_options{ 
        
               DefaultTestCompressor, rosbag2_compression::CompressionMode::FILE, 
        
               kDefaultCompressionQueueSize, kDefaultCompressionQueueThreads};

But in this case, will need to add rosbag2_compression_zstd as a runtime dependenises for the tests in the package.xml file

MichaelOrlov

@jmachowinski Thanks for the renames and fixes.
We do have currently uint8_t for thread_priority and it is totally aligned with the range of the nice value on Linux/POSIX since it varies from -19 to 20.
Although it doesn't align with the possible thread's priority values on Windows.
According to the https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-setthreadpriority one of the biggest values could be settled up is

THREAD_MODE_BACKGROUND_END 0x00020000

In decimal, this is 131 072. On Windows, the type is int but this is not good to use int type in data structures since not strictly defined length of the integer type. i.e. it could vary from different platforms and compilators.
I think we need to change the type for the thread's priority at least to the int32_t or even better int64_t to accommodate all possible values.
We can use a static cast to int when passing it to set priority functions.

MichaelOrlov · 2023-10-06T06:15:09Z

@jmachowinski Could you please fix DCO and cpplint warnings?

MichaelOrlov · 2023-12-11T20:08:47Z

@jmachowinski I am sorry, but we just merged another big PR and this branch has to be rebased and conflicts shall be resolved.
Could you please rebase your branch on top of the latest rolling ?

…read priority Signed-off-by: Janosch Machowinski <[email protected]>

Signed-off-by: Janosch Machowinski <[email protected]>

Co-authored-by: Janosch Machowinski <[email protected]> Signed-off-by: Michael Orlov <[email protected]>

Signed-off-by: Janosch Machowinski <[email protected]>

Signed-off-by: Michael Orlov <[email protected]>

jmachowinski · 2023-12-12T08:30:30Z

rebased

MichaelOrlov · 2023-12-12T21:47:21Z

@jmachowinski since we added recently a feature for composable recorder. This is when recorder_options values getting from a node params. We need to add compression_thread_priority to the relevant parser.

Could you please add

record_options.compression_threads_priority = param_utils::declare_integer_node_params<int32_t>(
    node, "compression_threads_priority", std::numeric_limits<int32_t>::min(),
    std::numeric_limits<int32_t>::max(), record_options.compression_threads_priority);

to the RecordOptions get_record_options_from_node_params(rclcpp::Node & node) and modify the relevant test to make sure that the value parsed correctly?

MichaelOrlov · 2023-12-13T20:58:52Z

@jmachowinski I've made the requested change in the node params parser and relevant test for the compression_thread_priority by myself.
Please review and merge or cherry-pick changes to your branch if you don't have any concerns.

jmachowinski · 2023-12-14T13:47:26Z

Looks good to me, another try to merge it ?

MichaelOrlov · 2023-12-14T16:11:35Z

Re-run CI after an attempt to fix Windows build errors and rebase.
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/13044

Linux
Linux-aarch64
Windows

MichaelOrlov · 2023-12-14T17:28:24Z

@jmachowinski Unfortunately Windows build still fails with the following error messages

:\ci\ws\src\cellumation\rosbag2\rosbag2_compression\test\rosbag2_compression\fake_compressor.cpp(26,1): warning C4273: 'FakeCompressor::FakeCompressor': inconsistent dll linkage [C:\ci\ws\build\rosbag2_compression\fake_plugin.vcxproj]

C:\ci\ws\src\cellumation\rosbag2\rosbag2_compression\test\rosbag2_compression\fake_compressor.hpp(29,3): message : see previous definition of '{ctor}' (compiling source file C:\ci\ws\src\cellumation\rosbag2\rosbag2_compression\test\rosbag2_compression\fake_compressor.cpp) [C:\ci\ws\build\rosbag2_compression\fake_plugin.vcxproj]

C:\ci\ws\src\cellumation\rosbag2\rosbag2_compression\test\rosbag2_compression\fake_compressor.cpp(38,1): warning C4273: 'FakeCompressor::compress_uri': inconsistent dll linkage [C:\ci\ws\build\rosbag2_compression\fake_plugin.vcxproj]

C:\ci\ws\src\cellumation\rosbag2\rosbag2_compression\test\rosbag2_compression\fake_compressor.hpp(31,15): message : see previous definition of 'compress_uri' (compiling source file C:\ci\ws\src\cellumation\rosbag2\rosbag2_compression\test\rosbag2_compression\fake_compressor.cpp) [C:\ci\ws\build\rosbag2_compression\fake_plugin.vcxproj]

C:\ci\ws\src\cellumation\rosbag2\rosbag2_compression\test\rosbag2_compression\fake_compressor.cpp(44,44): warning C4273: 'FakeCompressor::compress_serialized_bag_message': inconsistent dll linkage [C:\ci\ws\build\rosbag2_compression\fake_plugin.vcxproj]

C:\ci\ws\src\cellumation\rosbag2\rosbag2_compression\test\rosbag2_compression\fake_compressor.hpp(33,8): message : see previous definition of 'compress_serialized_bag_message' (compiling source file C:\ci\ws\src\cellumation\rosbag2\rosbag2_compression\test\rosbag2_compression\fake_compressor.cpp) [C:\ci\ws\build\rosbag2_compression\fake_plugin.vcxproj]

C:\ci\ws\src\cellumation\rosbag2\rosbag2_compression\test\rosbag2_compression\fake_compressor.cpp(47,1): warning C4273: 'FakeCompressor::get_compression_identifier': inconsistent dll linkage [C:\ci\ws\build\rosbag2_compression\fake_plugin.vcxproj]

C:\ci\ws\src\cellumation\rosbag2\rosbag2_compression\test\rosbag2_compression\fake_compressor.hpp(37,15): message : see previous definition of 'get_compression_identifier' (compiling source file C:\ci\ws\src\cellumation\rosbag2\rosbag2_compression\test\rosbag2_compression\fake_compressor.cpp) [C:\ci\ws\build\rosbag2_compression\fake_plugin.vcxproj]

     Creating library C:/ci/ws/build/rosbag2_compression/Release/fake_plugin.lib and object C:/ci/ws/build/rosbag2_compression/Release/fake_plugin.exp

fake_compressor.obj : error LNK2019: unresolved external symbol "__declspec(dllimport) public: __cdecl FakeCompressor::FakeCompressor(void)" (__imp_??0FakeCompressor@@QEAA@XZ) referenced in function "public: virtual class rosbag2_compression::BaseCompressorInterface * __cdecl class_loader::impl::MetaObject<class FakeCompressor,class rosbag2_compression::BaseCompressorInterface>::create(void)const " (?create@?$MetaObject@VFakeCompressor@@VBaseCompressorInterface@rosbag2_compression@@@impl@class_loader@@UEBAPEAVBaseCompressorInterface@rosbag2_compression@@XZ) [C:\ci\ws\build\rosbag2_compression\fake_plugin.vcxproj]

fake_compressor.obj : error LNK2019: unresolved external symbol "__declspec(dllimport) public: virtual __cdecl FakeCompressor::~FakeCompressor(void)" (__imp_??1FakeCompressor@@UEAA@XZ) referenced in function "public: virtual void * __cdecl FakeCompressor::`scalar deleting destructor'(unsigned int)" (??_GFakeCompressor@@UEAAPEAXI@Z) [C:\ci\ws\build\rosbag2_compression\fake_plugin.vcxproj]

fake_compressor.obj : error LNK2019: unresolved external symbol "__declspec(dllimport) const FakeCompressor::`vftable'" (__imp_??_7FakeCompressor@@6B@) referenced in function "public: __cdecl FakeCompressor::FakeCompressor(int &)" (??0FakeCompressor@@QEAA@AEAH@Z) [C:\ci\ws\build\rosbag2_compression\fake_plugin.vcxproj]

C:\ci\ws\build\rosbag2_compression\Release\fake_plugin.dll : fatal error LNK1120: 3 unresolved externals [C:\ci\ws\build\rosbag2_compression\fake_plugin.vcxproj]

jmachowinski · 2023-12-15T13:56:49Z

I just set up a windows machine....

Signed-off-by: Janosch Machowinski <[email protected]>

Signed-off-by: Michael Orlov <[email protected]>

jmachowinski · 2023-12-15T14:48:40Z

Compiles and tests now fine under windows.

MichaelOrlov · 2023-12-15T17:16:56Z

Re-run Windows CI job after the attempt to fix build errors.

Windows

ros-discourse · 2023-12-18T22:01:36Z

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/ros-2-tsc-meeting-minutes-for-2023-12-14/35153/1

ros-discourse · 2024-01-25T00:45:38Z

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/ros-2-tsc-meeting-minutes-for-2024-01-18/35779/1

jmachowinski requested a review from a team as a code owner September 4, 2023 12:21

jmachowinski requested review from gbiggs and james-rms and removed request for a team September 4, 2023 12:21

clalancette reviewed Sep 5, 2023

View reviewed changes

rosbag2_compression/src/rosbag2_compression/sequential_compression_writer.cpp Outdated Show resolved Hide resolved

jmachowinski force-pushed the rolling branch 2 times, most recently from fbe7081 to 77ce698 Compare September 5, 2023 15:33

MichaelOrlov requested changes Sep 8, 2023

View reviewed changes

jmachowinski force-pushed the rolling branch from 77ce698 to 62b4b31 Compare September 12, 2023 09:06

fujitatomoya requested changes Sep 13, 2023

View reviewed changes

jmachowinski force-pushed the rolling branch from 62b4b31 to 35c8cdb Compare September 14, 2023 08:23

MichaelOrlov reviewed Sep 15, 2023

View reviewed changes

rosbag2_compression/src/rosbag2_compression/sequential_compression_writer.cpp Outdated Show resolved Hide resolved

MichaelOrlov requested changes Sep 15, 2023

View reviewed changes

rosbag2_compression/test/rosbag2_compression/test_sequential_compression_writer.cpp Outdated Show resolved Hide resolved

rosbag2_compression/test/rosbag2_compression/test_sequential_compression_writer.cpp Outdated Show resolved Hide resolved

MichaelOrlov changed the title ~~feat(SequentialCompressionWriter): Added option to set compression th…~~ Add option to set compression threads priority Sep 18, 2023

MichaelOrlov mentioned this pull request Sep 21, 2023

Redesign test for the case when compression writer sets threads priority cellumation/rosbag2#1

Merged

jmachowinski force-pushed the rolling branch from 072ef7e to c8cdcba Compare September 22, 2023 14:48

MichaelOrlov reviewed Sep 28, 2023

View reviewed changes

jmachowinski force-pushed the rolling branch from 8fcccdb to c43e37d Compare October 6, 2023 15:22

Janosch Machowinski and others added 8 commits December 12, 2023 07:43

feat(SequentialCompressionWriter): Added option to set compression th…

e94ced8

…read priority Signed-off-by: Janosch Machowinski <[email protected]>

feat: Added compression_threads_priority option

a390c4a

Signed-off-by: Janosch Machowinski <[email protected]>

test: implemented test case for setting priority value

6ea1dcf

Signed-off-by: Janosch Machowinski <[email protected]>

Redesign test for the case when compression writer sets threads priority

32e5002

Co-authored-by: Janosch Machowinski <[email protected]> Signed-off-by: Michael Orlov <[email protected]>

fix: Fixed inverted check

fc749da

Signed-off-by: Janosch Machowinski <[email protected]>

chore: made compression_threads_priority an int32_t

497f806

Signed-off-by: Janosch Machowinski <[email protected]>

Enable thread priority for Windows

99c05a2

Signed-off-by: Michael Orlov <[email protected]>

Address grammar in log messages

3cc1b5b

Signed-off-by: Michael Orlov <[email protected]>

jmachowinski force-pushed the rolling branch from c16ea98 to ed3d4e4 Compare December 12, 2023 08:30

MichaelOrlov mentioned this pull request Dec 13, 2023

Add compression_threads_priority to the node params parser cellumation/rosbag2#3

Merged

MichaelOrlov mentioned this pull request Dec 14, 2023

Data loss when using compression (ros2 bag record) #978

Closed

Janosch Machowinski and others added 2 commits December 15, 2023 15:41

fix: fixed namespace collision warning in test

22c18d5

Signed-off-by: Janosch Machowinski <[email protected]>

Add compression_threads_priority to the node params parser

2f58113

Signed-off-by: Michael Orlov <[email protected]>

jmachowinski force-pushed the rolling branch from b0f0aa2 to 2f58113 Compare December 15, 2023 14:41

MichaelOrlov merged commit cea3fb0 into ros2:rolling Dec 16, 2023
14 checks passed

MichaelOrlov mentioned this pull request Jul 25, 2024

Add CLI option to set compression threads priority for Rosbag2 recorder #1760

Closed

MichaelOrlov mentioned this pull request Aug 9, 2024

Add cli option compression-threads-priority #1768

Merged

mergify bot mentioned this pull request Aug 9, 2024

[jazzy] Add cli option compression-threads-priority (backport #1768) #1778

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option to set compression threads priority #1457

Add option to set compression threads priority #1457

jmachowinski commented Sep 4, 2023 •

edited

Loading

arneboe commented Sep 5, 2023

MichaelOrlov left a comment

jmachowinski commented Sep 12, 2023

fujitatomoya left a comment

jmachowinski commented Sep 14, 2023

MichaelOrlov commented Sep 15, 2023

MichaelOrlov left a comment

jmachowinski commented Sep 18, 2023

MichaelOrlov commented Sep 21, 2023

jmachowinski commented Sep 22, 2023

jmachowinski commented Sep 22, 2023

MichaelOrlov commented Sep 22, 2023

jmachowinski commented Sep 22, 2023

MichaelOrlov commented Sep 22, 2023

MichaelOrlov left a comment

MichaelOrlov commented Oct 6, 2023

MichaelOrlov commented Dec 11, 2023

jmachowinski commented Dec 12, 2023

MichaelOrlov commented Dec 12, 2023

MichaelOrlov commented Dec 13, 2023

jmachowinski commented Dec 14, 2023

MichaelOrlov commented Dec 14, 2023

MichaelOrlov commented Dec 14, 2023

jmachowinski commented Dec 15, 2023

jmachowinski commented Dec 15, 2023

MichaelOrlov commented Dec 15, 2023

ros-discourse commented Dec 18, 2023

ros-discourse commented Jan 25, 2024

Add option to set compression threads priority #1457

Add option to set compression threads priority #1457

Conversation

jmachowinski commented Sep 4, 2023 • edited Loading

arneboe commented Sep 5, 2023

MichaelOrlov left a comment

Choose a reason for hiding this comment

jmachowinski commented Sep 12, 2023

fujitatomoya left a comment

Choose a reason for hiding this comment

jmachowinski commented Sep 14, 2023

MichaelOrlov commented Sep 15, 2023

MichaelOrlov left a comment

Choose a reason for hiding this comment

jmachowinski commented Sep 18, 2023

MichaelOrlov commented Sep 21, 2023

jmachowinski commented Sep 22, 2023

jmachowinski commented Sep 22, 2023

MichaelOrlov commented Sep 22, 2023

jmachowinski commented Sep 22, 2023

MichaelOrlov commented Sep 22, 2023

MichaelOrlov left a comment

Choose a reason for hiding this comment

MichaelOrlov commented Oct 6, 2023

MichaelOrlov commented Dec 11, 2023

jmachowinski commented Dec 12, 2023

MichaelOrlov commented Dec 12, 2023

MichaelOrlov commented Dec 13, 2023

jmachowinski commented Dec 14, 2023

MichaelOrlov commented Dec 14, 2023

MichaelOrlov commented Dec 14, 2023

jmachowinski commented Dec 15, 2023

jmachowinski commented Dec 15, 2023

MichaelOrlov commented Dec 15, 2023

ros-discourse commented Dec 18, 2023

ros-discourse commented Jan 25, 2024

jmachowinski commented Sep 4, 2023 •

edited

Loading