Bamadapterfind Threaded Speed-Up

[whitwham]

Thread the slower alignment section of bamadapterfind without changing the final result.

[gt1]

Hi Andrew,

thank you for the patch. I have taken a quick look and the part starting at

https://github.com/gt1/biobambam2/pull/22/commits/67f99b3e20ed3f13b55e834d7c1bef39f29c8f63#diff-484e4673a5a53a831b6c409d261546dcR855

looks like it may need some rewriting. As far as I can see you hold a lock while running empty(), top() and pop() for write_queue, but not between these calls. I guess this means you have race conditions in there, for instance top() may fail because another thread emptied the queue between the empty() and top() call. You probably need an atomic function which takes a suitable element out of the queue.

Best wishes, German

[whitwham]

Hi German,

Thanks for taking a look at this.

You are right about the possible race conditions in general. In this specific case however there is only one thread that takes elements off the write_queue so there is no chance of the queue being emptied between checks.

The program runs with one input, multiple processing and one output thread.

In a more generic length limiting queue (one written for libmaus for example) then it would need the atomic remove element function.

I have put bamadapterfind through valgrind's helgrind and drd tools and neither detected any problems with the queue code (though neither tool is perfect).

Regards,

Andrew

On 22/06/16 12:14, German Tischler wrote:

Hi Andrew,

thank you for the patch. I have taken a quick look and the part starting at

https://github.com/gt1/biobambam2/pull/22/commits/67f99b3e20ed3f13b55e834d7c1bef39f29c8f63#diff-484e4673a5a53a831b6c409d261546dcR855

looks like it may need some rewriting. As far as I can see you hold a lock while running empty(), top() and pop() for write_queue, but not between these calls. I guess this means you have race conditions in there, for instance top() may fail because another thread emptied the queue between the empty() and top() call. You probably need an atomic function which takes a suitable element out of the queue.

Best wishes, German

The Wellcome Trust Sanger Institute is operated by Genome Research Limited, a charity registered in England with number 1021457 and a company registered in England with number 2742969, whose registered office is 215 Euston Road, London, NW1 2BE.

[gt1]

Hi Andrew,

ok, so that is not an issue.

It looks like you are instantiating a new AdapterFilter object for each work package. I have rewritten that class so it is more readily reusable ( see https://github.com/gt1/libmaus2/commit/918d514f9e4c393f1205f161ab64653653824b0d , caveat: I have not tested it ). Could you try using it? You would have const AdapterFilter and the only additional data necessary per thread is a libmaus2::fastx::AutoArrayWordPutObject .

Regards, German

[whitwham]

Hi German,

I will be happy to try it. I'll be away for a week so I probably will not be able to give it a proper go until after that.

Thanks,

Andrew

On 23/06/16 15:32, German Tischler wrote:

Hi Andrew,

ok, so that is not an issue.

It looks like you are instantiating a new AdapterFilter object for each work package. I have rewritten that class so it is more readily reusable ( see https://github.com/gt1/libmaus2/commit/918d514f9e4c393f1205f161ab64653653824b0d , caveat: I have not tested it ). Could you try using it? You would have const AdapterFilter and the only additional data necessary per thread is a libmaus2::fastx::AutoArrayWordPutObject .

Regards, German

--- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: https://github.com/gt1/biobambam2/pull/22#issuecomment-228068923

The Wellcome Trust Sanger Institute is operated by Genome Research Limited, a charity registered in England with number 1021457 and a company registered in England with number 2742969, whose registered office is 215 Euston Road, London, NW1 2BE.

[gt1]

Thanks Andrew. Would you be able to provide a pull request without merge conflicts?

Best, German

[whitwham]

Mistaken commit. I'll need to fix it.

[whitwham]

Merge conflicts fixed.

[gt1]

Thanks.