seqan / seqan3

The modern C++ library for sequence analysis. Contains version 3 of the library and API docs.
https://www.seqan.de
Other
409 stars 82 forks source link

[FEATURE] Minor increase in FASTA performance. #3104

Closed smehringer closed 1 year ago

smehringer commented 1 year ago

I had this lying around when cleaning up my branches.

On my PC with GCC 11.3 in was a noisy 5% performance gain on reading FASTA from stream.

I still think it's fine to merge, as it's a small change and makes sense. It avoids an if clause in case the character is an alphabet character, which in a normal FASTA file is most often the case.

vercel[bot] commented 1 year ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
seqan3 ✅ Ready (Inspect) Visit Preview Nov 25, 2022 at 8:44AM (UTC)
codecov[bot] commented 1 year ago

Codecov Report

Base: 98.22% // Head: 98.22% // No change to project coverage :thumbsup:

Coverage data is based on head (b1709f3) compared to base (d98c12d). Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #3104 +/- ## ======================================= Coverage 98.22% 98.22% ======================================= Files 275 275 Lines 12192 12192 ======================================= Hits 11975 11975 Misses 217 217 ``` | [Impacted Files](https://codecov.io/gh/seqan/seqan3/pull/3104?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=seqan) | Coverage Δ | | |---|---|---| | [include/seqan3/utility/views/chunk.hpp](https://codecov.io/gh/seqan/seqan3/pull/3104/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=seqan#diff-aW5jbHVkZS9zZXFhbjMvdXRpbGl0eS92aWV3cy9jaHVuay5ocHA=) | `100.00% <ø> (ø)` | | | [include/seqan3/io/sequence\_file/format\_fasta.hpp](https://codecov.io/gh/seqan/seqan3/pull/3104/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=seqan#diff-aW5jbHVkZS9zZXFhbjMvaW8vc2VxdWVuY2VfZmlsZS9mb3JtYXRfZmFzdGEuaHBw) | `94.91% <100.00%> (ø)` | | | [...qan3/core/range/detail/inherited\_iterator\_base.hpp](https://codecov.io/gh/seqan/seqan3/pull/3104/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=seqan#diff-aW5jbHVkZS9zZXFhbjMvY29yZS9yYW5nZS9kZXRhaWwvaW5oZXJpdGVkX2l0ZXJhdG9yX2Jhc2UuaHBw) | `100.00% <0.00%> (ø)` | | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=seqan). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=seqan)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

smehringer commented 1 year ago

Replacing the for-loop with [...] works even better for me. Can you double check?

Only very little, could be noise for me but if it is better in your case, let's take it.

constexpr auto is_ignored = is_space || is_digit; in the beginning of the function and then

else if (is_ignored(*it))

also has some effect, but might just be noise

A little worse for me, probably noise. I left it out.