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

Thread save key detect #2113

Closed
wants to merge 5 commits into from
Closed

Conversation

daschuer
Copy link
Member

This fixes that a global buffers is used from the key detector object. Now every object used is own instance.

This PR also removes some intermediate buffers.

Currently we don't even need to copy the buffer locally if we store the but I have kept using it, because I do not want to alter the numbers from the original profiles which are taken from an academic paper.

@uklotzde
Copy link
Contributor

I still don't spot any thread-safety issues?? Those pseudo-constant static arrays should not cause any harm, neither before nor after your changes. The elements will be initialized before the first analysis starts and are never changed ever after.

@uklotzde
Copy link
Contributor

uklotzde commented May 14, 2019

Oh, no, now I see it. It's in MathUtilities::circShift()!! OMG, that's really, really, bad and dangerous (just wrong) code.

@uklotzde
Copy link
Contributor

uklotzde commented May 14, 2019

One more reason to stick with Rust! The compiler prevents most of the ridiculous bullshit a mediocre developer (like me) tries to write.

@uklotzde uklotzde closed this May 14, 2019
@uklotzde uklotzde reopened this May 14, 2019
@Be-ing
Copy link
Contributor

Be-ing commented May 15, 2019

Does this mean #2009 can be reverted?

@uklotzde
Copy link
Contributor

Not sure. The bug is not able to trigger a segfault and that's what I remember what we have experienced casually. This fix only ensures that no bogus values are returned.

@uklotzde
Copy link
Contributor

uklotzde commented May 15, 2019

There might be more and even more serious bugs hidden in the code, which looks like it has been ported literally from Mathlab without carefully considering those technical aspects.


m_MajProfileNorm = new double[m_BPO];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

m_BPO is just the constant 36. Static allocation would work for all those arrays as well as a single dynamic allocation with partitioning/slicing.

@uklotzde uklotzde added this to the 2.3.0 milestone May 15, 2019
@Be-ing
Copy link
Contributor

Be-ing commented May 15, 2019

#2009 was fixing a bug with incorrect analysis results, not a crash. So I think it's worth a try to revert that now.

@uklotzde
Copy link
Contributor

Thanks @Be-ing for checking this out. That's what I planned to do now.

So this will finally allow us to unleash the full power of the multi-threaded analysis!!

@uklotzde
Copy link
Contributor

Note: After this PR has been merged we can apply a revert commit from #2009. But without undoing the hotfix in #2010! I will take care of this later.

@uklotzde
Copy link
Contributor

Please rebase after #2112 has now been merged.

@daschuer
Copy link
Member Author

daschuer commented May 24, 2019

Done. CI has timed out.

@daschuer
Copy link
Member Author

daschuer commented Jun 3, 2019

This was superseded by #2136 and can be closed.

@daschuer daschuer closed this Jun 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants