Skip to content
This repository has been archived by the owner on Mar 22, 2023. It is now read-only.

Add valgrind instrumentation to concurrent_hash_map #476

Merged
merged 1 commit into from
Oct 10, 2019

Conversation

vinser52
Copy link
Contributor

@vinser52 vinser52 commented Oct 7, 2019

The commit fix the issue with concurrent_hash_map_rehash_0_helgrind test


This change is Reviewable

@codecov
Copy link

codecov bot commented Oct 7, 2019

Codecov Report

Merging #476 into master will decrease coverage by 0.02%.
The diff coverage is 75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #476      +/-   ##
==========================================
- Coverage    96.7%   96.67%   -0.03%     
==========================================
  Files          34       34              
  Lines        4061     4065       +4     
==========================================
+ Hits         3927     3930       +3     
- Misses        134      135       +1
Flag Coverage Δ
#tests_clang_debug_cpp17 96.52% <75%> (-0.18%) ⬇️
#tests_gcc_debug 93.76% <75%> (-0.03%) ⬇️
Impacted Files Coverage Δ
...ude/libpmemobj++/container/concurrent_hash_map.hpp 96.1% <75%> (-0.11%) ⬇️

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 70f4e35...84d4983. Read the comment docs.

Copy link
Contributor

@igchor igchor left a comment

Choose a reason for hiding this comment

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

What about other places where we call mask().load()/store()? Shouldn;'t we have annotations also there?

Reviewed 1 of 1 files at r1.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

The commit fix the issue with concurrent_hash_map_rehash_0_helgrind test
@vinser52 vinser52 force-pushed the concurrent_hash_map_helgrind_fix branch from 9c8a138 to 84d4983 Compare October 8, 2019 12:29
Copy link
Contributor Author

@vinser52 vinser52 left a comment

Choose a reason for hiding this comment

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

I have added more annotations to places where we load with acquire.

Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on @igchor)

@pmem-bot
Copy link
Contributor

pmem-bot commented Oct 8, 2019

@igchor and @annamarcink please review this pull request

@igchor
Copy link
Contributor

igchor commented Oct 8, 2019

Could you make PR to stable-1.8?

Copy link
Contributor Author

@vinser52 vinser52 left a comment

Choose a reason for hiding this comment

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

Do you mean one more PR or just instead of this one?

Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on @igchor)

@igchor
Copy link
Contributor

igchor commented Oct 8, 2019

Just this one (we will later merge stable-1.8 to master)

@vinser52 vinser52 changed the base branch from master to stable-1.8 October 9, 2019 09:00
Copy link
Contributor Author

@vinser52 vinser52 left a comment

Choose a reason for hiding this comment

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

Done

Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on @igchor)

Copy link
Contributor

@igchor igchor left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on @igchor)

@igchor igchor merged commit d118e31 into pmem:stable-1.8 Oct 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants