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

test: add a test that checks performance without valgrind #5817

Merged
merged 1 commit into from
Jul 21, 2023

Conversation

osalyk
Copy link
Contributor

@osalyk osalyk commented Jul 21, 2023

This change is Reviewable

@osalyk osalyk added the sprint goal This pull request is part of the ongoing sprint label Jul 21, 2023
@janekmi janekmi added this to the 2.0.0 milestone Jul 21, 2023
@codecov
Copy link

codecov bot commented Jul 21, 2023

Codecov Report

Merging #5817 (71d8e52) into master (3b4d2cb) will increase coverage by 0.06%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #5817      +/-   ##
==========================================
+ Coverage   70.94%   71.00%   +0.06%     
==========================================
  Files         131      131              
  Lines       19175    19175              
  Branches     3193     3193              
==========================================
+ Hits        13603    13616      +13     
+ Misses       5572     5559      -13     

Copy link
Contributor

@janekmi janekmi left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @osalyk)


src/test/obj_sync/err0.log.match line 4 at r1 (raw file):

{$(nW)obj_sync.c:$(N) rwlock_$(nW)_worker} obj_sync$(nW)TEST0: pmemobj_rwlock_wrlock
{$(nW)obj_sync.c:$(N) cond_$(nW)_worker} obj_sync$(nW)TEST0: pmemobj_cond_signal
{$(nW)obj_sync.c:$(N) timed_$(nW)_worker} obj_sync$(nW)TEST0: pmemobj_mutex_timedlock: Invalid argument

I can't find an intentional attempt to break any of these calls. Are these errors truly as expected?


src/test/obj_sync/TEST0 line 29 at r1 (raw file):


rm -f $LOG && touch $LOG
rm -f $LOG_TEMP && touch $LOG_TEMP

I believe the $LOG file is recreated each time. So there is no need to delete it or create it.
The $LOG_TEMP is appended over and over again, so it is a good idea to delete it at the begining. But I believe creating it (touch) is not needed. The first append (>>) will create it. Am I right?

Suggestion:

rm -f $LOG_TEMP

src/test/obj_sync/TEST0 line 31 at r1 (raw file):

rm -f $LOG_TEMP && touch $LOG_TEMP

VALGRIND_DISABLED=y expect_normal_exit ./obj_sync$EXESUFFIX m 10 5

We had here 50 threads previously. I believe it is just a copy-paste error.

Suggestion:

VALGRIND_DISABLED=y expect_normal_exit ./obj_sync$EXESUFFIX m 50 5

src/test/obj_sync/TEST0 line 40 at r1 (raw file):

cat $LOG >> $LOG_TEMP
cat $ERR >> $ERR_TEMP
VALGRIND_DISABLED=y expect_normal_exit ./obj_sync$EXESUFFIX t 10 5

I think we can go higher. Considering TEST7 is brave enough to do so.

Suggestion:

VALGRIND_DISABLED=y expect_normal_exit ./obj_sync$EXESUFFIX t 50 5

Copy link
Contributor Author

@osalyk osalyk left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @janekmi)


src/test/obj_sync/err0.log.match line 4 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

I can't find an intentional attempt to break any of these calls. Are these errors truly as expected?

This was the case in the rest of the tests


src/test/obj_sync/TEST0 line 29 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

I believe the $LOG file is recreated each time. So there is no need to delete it or create it.
The $LOG_TEMP is appended over and over again, so it is a good idea to delete it at the begining. But I believe creating it (touch) is not needed. The first append (>>) will create it. Am I right?

According to my experience it should stay, otherwise the test crashes


src/test/obj_sync/TEST0 line 31 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

We had here 50 threads previously. I believe it is just a copy-paste error.

Done.


src/test/obj_sync/TEST0 line 40 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

I think we can go higher. Considering TEST7 is brave enough to do so.

Done.

Copy link
Contributor

@janekmi janekmi left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @osalyk)

@janekmi janekmi merged commit 1015c35 into pmem:master Jul 21, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sprint goal This pull request is part of the ongoing sprint
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants