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

Add multi-threaded test #23

Merged
merged 1 commit into from Jan 8, 2019
Merged

Conversation

ldorau
Copy link
Member

@ldorau ldorau commented Jan 3, 2019

This change is Reviewable

@codecov-io
Copy link

codecov-io commented Jan 3, 2019

Codecov Report

Merging #23 into master will increase coverage by 2.75%.
The diff coverage is 90.19%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #23      +/-   ##
==========================================
+ Coverage   35.82%   38.58%   +2.75%     
==========================================
  Files          21       22       +1     
  Lines        2127     2229     +102     
==========================================
+ Hits          762      860      +98     
- Misses       1365     1369       +4
Impacted Files Coverage Δ
tests/vmemcache_test_mt.c 90.19% <90.19%> (ø)
src/os_thread_posix.c 40% <0%> (+6.66%) ⬆️

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 ff46cdf...7e4be0f. Read the comment docs.

@janekmi
Copy link

janekmi commented Jan 4, 2019

Please rebase.

@ldorau
Copy link
Member Author

ldorau commented Jan 4, 2019

Rebased

Copy link

@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.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ldorau)


tests/vmemcache_test_mt.c, line 210 at r1 (raw file):

	}

	srand((unsigned)time(NULL));

I prefer to initialize pseudo-random generator with seed with guarantees a test to be repeatable. e.g. 0


tests/vmemcache_test_mt.c, line 224 at r1 (raw file):

		}

		memset(buffs[i].buff, 0xCC, buffs[i].size);

Maybe it is also a good idea to store various patterns in each buffer?


tests/vmemcache_test_mt.c, line 230 at r1 (raw file):

	if (threads == NULL) {
		FATAL("out of memory");
		goto exit_free_ctx;

exit_free_buffs

/* run all tests */
run_test_put(n_threads, threads, ctx);
run_test_get(cache, n_threads, threads, ctx);

Copy link
Member

Choose a reason for hiding this comment

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

it would be good to have also a mix workload (get/put) and mix with evict workload.

Copy link
Member Author

Choose a reason for hiding this comment

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

It will be added in the next PR, because it requires this fix: #27 to be merged first.

Copy link
Member

@pbalcer pbalcer 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, @ldorau, and @pbalcer)


tests/vmemcache_test_mt.c, line 210 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

I prefer to initialize pseudo-random generator with seed with guarantees a test to be repeatable. e.g. 0

👍 better yet, let's just make this an option that defaults to 0.

Copy link

@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.

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


tests/vmemcache_test_mt.c, line 250 at r1 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

it would be good to have also a mix workload (get/put) and mix with evict workload.

👍

@ldorau ldorau force-pushed the add-multi-threaded-test branch 2 times, most recently from 644ea61 to 8efa9dc Compare January 7, 2019 10:02
Copy link
Member Author

@ldorau ldorau 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: 2 of 3 files reviewed, 4 unresolved discussions (waiting on @janekmi and @pbalcer)


tests/vmemcache_test_mt.c, line 210 at r1 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

👍 better yet, let's just make this an option that defaults to 0.

Done.


tests/vmemcache_test_mt.c, line 224 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Maybe it is also a good idea to store various patterns in each buffer?

It does not matter in this case.


tests/vmemcache_test_mt.c, line 230 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

exit_free_buffs

Done.


tests/vmemcache_test_mt.c, line 250 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

👍

It will be added in the next PR

Copy link
Member Author

@ldorau ldorau 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: 2 of 3 files reviewed, 4 unresolved discussions (waiting on @janekmi and @pbalcer)


tests/vmemcache_test_mt.c, line 250 at r1 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

It will be added in the next PR

... because it requires this fix: #27 to be merged first.

Copy link

@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 1 of 1 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @pbalcer)

Copy link

@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:

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @pbalcer)

@ldorau
Copy link
Member Author

ldorau commented Jan 8, 2019

@pbalcer please resolve your 1 discussion

Copy link
Member

@pbalcer pbalcer 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: :shipit: complete! all files reviewed, all discussions resolved

Copy link

@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.

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@janekmi janekmi merged commit 9c72abf into pmem:master Jan 8, 2019
@ldorau ldorau deleted the add-multi-threaded-test branch January 8, 2019 12:09
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.

None yet

4 participants