[FIX] make a bgzf_thread_count a single variable #2752

SGSSGene · 2021-06-14T07:32:37Z

bgzf_thread_count was declared inline static outside of the scope of a
class.
In this case the inline allows the variable to break the one
definition rule. Meaning there can be multiple symbols across different
translation that all declare bgzf_thread_count. In this case the
linker will choose one of the defined symbols. (Without inline it would
throw a multiple definition error).

The static keyword (in this context) means that the created symbol is
only visible inside a current translation unit. Meaning, there are no symbols
for the linker to work with. This will cause every translation unit to have there
own bgzf_thread_count variable.

This combination leads to every translation unit having there own
bgzf_thread_count variable. The fix is simple, we remove the static
keyword. This should have the intended behavior.

Changelog entry (API change)
Adapt cookbook entry

vercel · 2021-06-14T07:32:41Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/seqan/seqan3/9deSNYGGQRf4KqYobzvAM3US4DQM
✅ Preview: https://seqan3-git-fork-sgssgene-fix-bgzfthreadcountvariable-seqan.vercel.app

codecov · 2021-06-14T07:47:13Z

Codecov Report

Merging #2752 (b778975) into master (0f5fb9d) will decrease coverage by 0.00%.
The diff coverage is n/a.

❗ Current head b778975 differs from pull request most recent head 1ec1bba. Consider uploading reports for the commit 1ec1bba to get more accurate results

@@            Coverage Diff             @@
##           master    #2752      +/-   ##
==========================================
- Coverage   98.28%   98.28%   -0.01%     
==========================================
  Files         267      266       -1     
  Lines       11462    11455       -7     
==========================================
- Hits        11265    11258       -7     
  Misses        197      197

Impacted Files	Coverage Δ
include/seqan3/utility/simd/algorithm.hpp	`100.00% <0.00%> (ø)`
include/seqan3/utility/tuple/pod_tuple.hpp	`100.00% <0.00%> (ø)`
test/include/seqan3/test/tmp_directory.hpp	`100.00% <0.00%> (ø)`
...nclude/seqan3/io/sam_file/input_format_concept.hpp	`100.00% <0.00%> (ø)`
...clude/seqan3/io/sam_file/output_format_concept.hpp	`100.00% <0.00%> (ø)`
...nclude/seqan3/alphabet/views/validate_char_for.hpp

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 0f5fb9d...1ec1bba. Read the comment docs.

simonsasse

👍

SGSSGene · 2021-07-12T15:30:41Z

Seqan2 "solves" this by using a macro/define instead of a global variable.

iostream_bgzf.h defines

#ifndef SEQAN_BGZF_NUM_THREADS
#define SEQAN_BGZF_NUM_THREADS 16
#endif

And then it is similar used similar to seqan3:


    basic_bgzf_streambuf(ostream_reference ostream_,
                         size_t numThreads = SEQAN_BGZF_NUM_THREADS,
                         size_t jobsPerThread = 8) :

taken from:
https://github.com/seqan/seqan/blob/f5f658343c366c9c3d44ba358ffc9317e78a09ed/include/seqan/stream/iostream_bgzf.h

include/seqan3/contrib/stream/bgzf_stream_util.hpp

smehringer · 2021-10-25T10:24:50Z

Core Meeting 25.10.2021 - This is not release critical.

marehr · 2021-10-25T10:38:59Z

include/seqan3/contrib/stream/bgzf_stream_util.hpp

@@ -38,7 +38,7 @@ namespace seqan3::contrib
 /*!\brief A static variable indicating the number of threads to use for the bgzf-streams.
 *       Defaults to std::thread::hardware_concurrency.
 */
-inline static uint64_t bgzf_thread_count = std::thread::hardware_concurrency();
+inline uint64_t bgzf_thread_count = std::thread::hardware_concurrency();


Maybe a hybrid idea: Use SeqAn 2 solution with macro:

#ifndef SEQAN3_BGZF_THREAD_COUNT #define SEQAN3_BGZF_THREAD_COUNT (std::thread::hardware_concurrency()) #define SEQAN3_BGZF_THREAD_COUNT (bgzf_thread_count) #endif // SEQAN3_BGZF_THREAD_COUNT basic_bgzf_streambuf(ostream_reference ostream_, size_t numThreads = SEQAN3_BGZF_THREAD_COUNT, size_t jobsPerThread = 8) :

This allows:

having consistent definition over all translation unit

making it configurable to a fixed number, to a function, to anything that reduces to a number

AND: It is a hack and will always be a hack.

I gave it some thought, but this doesn't really work.
The issue is if I declare -DSEQAN3_BGZF_THREAD_COUNT=(std::thread::hardware_concurrency()) It is required that the header includes #include <thread>. Same goes if I want to use a user defined variable.
It would require that other headers are being included before. I don't see the advantages.

with static: each translation unit gets it's own variable.
- you have to set it in each unit, cannot alter it globally
+ you can alter it for each unit

without static:
+ you set it globally
- different compilation unit cannot use different values
- if you set the variable in multiple places, the compiler chooses the variable at random

Irallia · 2021-11-24T14:21:09Z

@marehr friendly reminder.

Would you like to clarify this please? If I see correctly, this change means that I no longer unintentionally occupy all threads on the server with iGenVar, although I have specified much less.

smehringer · 2021-12-06T10:57:26Z

Core meeting 06.12.2021 - The common use case is that you want the same variable across your translation units. So removing static enables the user to set the variable globally for his/her application at runtime (e.g. via command line input).

bgzf_thread_count was declared `inline static` outside of the scope of a class. In this case the `inline` allows the variable to break the one definition rule. Meaning there can be multiple symbols across different translation that all declare `bgzf_thread_count`. In this case the linker will choose one of the defined symbols. (Without inline it would throw a multiple definition error). The `static` keyword (in this context) means that the created symbol is only visible inside a current translation unit. Meaning, there are no symbols for the linker to work with. This will cause every translation unit to have there own `bgzf_thread_count` variable. This combination leads to every translation unit having there own `bgzf_thread_count` variable, which is not what a user of seqan3 expects. The fix is simple, we remove the `static` keyword. This should have the intended behavior. Thought: maybe it should be declared `thread_local`.

vercel bot deployed to Preview June 14, 2021 07:32 View deployment

SGSSGene force-pushed the fix/bgzf_thread_count_variable branch from 223728b to 0c3c2f2 Compare June 14, 2021 07:33

SGSSGene requested review from a team and simonsasse and removed request for a team June 14, 2021 07:33

vercel bot deployed to Preview June 14, 2021 07:35 View deployment

simonsasse approved these changes Jun 15, 2021

View reviewed changes

marehr requested review from a team and marehr and removed request for a team August 9, 2021 08:50

SGSSGene commented Oct 25, 2021

View reviewed changes

include/seqan3/contrib/stream/bgzf_stream_util.hpp Outdated Show resolved Hide resolved

SGSSGene marked this pull request as draft October 25, 2021 10:24

marehr reviewed Oct 25, 2021

View reviewed changes

Irallia approved these changes Nov 24, 2021

View reviewed changes

SGSSGene marked this pull request as ready for review November 24, 2021 14:21

SGSSGene force-pushed the fix/bgzf_thread_count_variable branch from 0c3c2f2 to 646a685 Compare November 30, 2021 15:31

vercel bot deployed to Preview November 30, 2021 15:31 View deployment

vercel bot deployed to Preview December 6, 2021 10:48 View deployment

Irallia mentioned this pull request Dec 6, 2021

[MISC] Update seqan3 to get thread problem fixed. seqan/iGenVar#187

Closed

vercel bot deployed to Preview December 7, 2021 10:31 View deployment

SGSSGene force-pushed the fix/bgzf_thread_count_variable branch from b778975 to 1ec1bba Compare January 12, 2022 12:22

vercel bot deployed to Preview January 12, 2022 12:22 View deployment

SGSSGene requested a review from smehringer January 17, 2022 09:38

smehringer approved these changes Jan 18, 2022

View reviewed changes

smehringer merged commit f668060 into seqan:master Jan 18, 2022

eseiler mentioned this pull request Oct 20, 2022

bgzf threads not configurable seqan/product_backlog#304

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FIX] make a bgzf_thread_count a single variable #2752

[FIX] make a bgzf_thread_count a single variable #2752

SGSSGene commented Jun 14, 2021 •

edited

vercel bot commented Jun 14, 2021 •

edited

codecov bot commented Jun 14, 2021 •

edited

simonsasse left a comment

SGSSGene commented Jul 12, 2021 •

edited

smehringer commented Oct 25, 2021

marehr Oct 25, 2021 •

edited

SGSSGene Oct 25, 2021

smehringer Dec 6, 2021

Irallia commented Nov 24, 2021

smehringer commented Dec 6, 2021

[FIX] make a bgzf_thread_count a single variable #2752

[FIX] make a bgzf_thread_count a single variable #2752

Conversation

SGSSGene commented Jun 14, 2021 • edited

vercel bot commented Jun 14, 2021 • edited

codecov bot commented Jun 14, 2021 • edited

Codecov Report

simonsasse left a comment

Choose a reason for hiding this comment

SGSSGene commented Jul 12, 2021 • edited

smehringer commented Oct 25, 2021

marehr Oct 25, 2021 • edited

Choose a reason for hiding this comment

SGSSGene Oct 25, 2021

Choose a reason for hiding this comment

smehringer Dec 6, 2021

Choose a reason for hiding this comment

Irallia commented Nov 24, 2021

smehringer commented Dec 6, 2021

SGSSGene commented Jun 14, 2021 •

edited

vercel bot commented Jun 14, 2021 •

edited

codecov bot commented Jun 14, 2021 •

edited

SGSSGene commented Jul 12, 2021 •

edited

marehr Oct 25, 2021 •

edited