obj: async post-commit #1671

Merged
merged 8 commits into from May 16, 2017

Conversation

Projects
None yet
3 participants
@pbalcer
Contributor

pbalcer commented Feb 13, 2017

Ref: pmem/issues#381


This change is Reviewable

@pbalcer pbalcer changed the title from obj: add simple ringbuf implementation to obj: async post-commit Feb 14, 2017

@krzycz

This comment has been minimized.

Show comment
Hide comment
@krzycz

krzycz Feb 22, 2017

Contributor

Reviewed 14 of 14 files at r1.
Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.


src/libpmemobj/obj.c, line 1027 at r1 (raw file):

		obj_pool_init();

		if ((pop->tx_postcommit_tasks = ringbuf_new(0)) == NULL) {

0 ?


src/libpmemobj/ringbuf.c, line 78 at r1 (raw file):

		return NULL;

	sem_init(&rbuf->nfree, 0, length);

missing error handling (same applies to all the calls to sem_*() APIs - sem_wait/sem_post/...)


src/libpmemobj/ringbuf.h, line 34 at r1 (raw file):

/*
 * ringbuf.h -- internal definitions for mpsc ring buffer

mp_M_c?


src/libpmemobj/tx.c, line 806 at r1 (raw file):

tx_get_pop(void)
{
	return tx.pop;

Hmmm, don't we need to check if stage is NONE?


src/libpmemobj/tx.c, line 1279 at r1 (raw file):

/*
 * tx_postcommit_cleanup -- performs all the necessery cleanup on a lane after

necessary


Comments from Reviewable

Contributor

krzycz commented Feb 22, 2017

Reviewed 14 of 14 files at r1.
Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.


src/libpmemobj/obj.c, line 1027 at r1 (raw file):

		obj_pool_init();

		if ((pop->tx_postcommit_tasks = ringbuf_new(0)) == NULL) {

0 ?


src/libpmemobj/ringbuf.c, line 78 at r1 (raw file):

		return NULL;

	sem_init(&rbuf->nfree, 0, length);

missing error handling (same applies to all the calls to sem_*() APIs - sem_wait/sem_post/...)


src/libpmemobj/ringbuf.h, line 34 at r1 (raw file):

/*
 * ringbuf.h -- internal definitions for mpsc ring buffer

mp_M_c?


src/libpmemobj/tx.c, line 806 at r1 (raw file):

tx_get_pop(void)
{
	return tx.pop;

Hmmm, don't we need to check if stage is NONE?


src/libpmemobj/tx.c, line 1279 at r1 (raw file):

/*
 * tx_postcommit_cleanup -- performs all the necessery cleanup on a lane after

necessary


Comments from Reviewable

@krzycz

This comment has been minimized.

Show comment
Hide comment
@krzycz

krzycz Feb 22, 2017

Contributor

Debug traces might be helpful sometimes...


Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.


Comments from Reviewable

Contributor

krzycz commented Feb 22, 2017

Debug traces might be helpful sometimes...


Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.


Comments from Reviewable

@pbalcer

This comment has been minimized.

Show comment
Hide comment
@pbalcer

pbalcer Feb 27, 2017

Contributor

I'll update this patch once CTL is merged. I've provided some preliminary performance results here:
pmem/issues#381

Contributor

pbalcer commented Feb 27, 2017

I'll update this patch once CTL is merged. I've provided some preliminary performance results here:
pmem/issues#381

@plebioda

This comment has been minimized.

Show comment
Hide comment
@plebioda

plebioda Mar 15, 2017

Contributor

Can't review this patch because it is not completed.


Reviewed 13 of 14 files at r1.
Review status: all files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.


src/libpmemobj/obj.c, line 1027 at r1 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

0 ?

bump ?


src/libpmemobj/ringbuf.h, line 46 at r1 (raw file):

struct ringbuf;

struct ringbuf *ringbuf_new(unsigned length);

size_t ?


Comments from Reviewable

Contributor

plebioda commented Mar 15, 2017

Can't review this patch because it is not completed.


Reviewed 13 of 14 files at r1.
Review status: all files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.


src/libpmemobj/obj.c, line 1027 at r1 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

0 ?

bump ?


src/libpmemobj/ringbuf.h, line 46 at r1 (raw file):

struct ringbuf;

struct ringbuf *ringbuf_new(unsigned length);

size_t ?


Comments from Reviewable

@pbalcer

This comment has been minimized.

Show comment
Hide comment
@pbalcer

pbalcer Mar 20, 2017

Contributor

It is complete to the point to which it it possible.


Review status: all files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.


Comments from Reviewable

Contributor

pbalcer commented Mar 20, 2017

It is complete to the point to which it it possible.


Review status: all files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.


Comments from Reviewable

@pbalcer

This comment has been minimized.

Show comment
Hide comment
@pbalcer

pbalcer Apr 18, 2017

Contributor

I have updated this patch with the CTL entry point and related changes. I'll separate out lane-related optimizations to a different patch and fix the windows build soon.

Contributor

pbalcer commented Apr 18, 2017

I have updated this patch with the CTL entry point and related changes. I'll separate out lane-related optimizations to a different patch and fix the windows build soon.

@krzycz

This comment has been minimized.

Show comment
Hide comment
@krzycz

krzycz Apr 21, 2017

Contributor

Reviewed 13 of 16 files at r2.
Review status: 16 of 19 files reviewed at latest revision, 6 unresolved discussions, some commit checks broke.


Comments from Reviewable

Contributor

krzycz commented Apr 21, 2017

Reviewed 13 of 16 files at r2.
Review status: 16 of 19 files reviewed at latest revision, 6 unresolved discussions, some commit checks broke.


Comments from Reviewable

@krzycz

This comment has been minimized.

Show comment
Hide comment
@krzycz

krzycz May 3, 2017

Contributor
  1. It would be nice to have some example and/or benchmark that illustrates how to use this feature and/or measures the performance gain.
    My first thought was to simply re-use the existing examples/benchmarks, but run it with some non-default CTL settings (env var), but I realized it's not that easy in case of async post-commit, as you need to explicitly run the PC thread.

  2. Please, use LOG/ERR macros!!!


Reviewed 4 of 10 files at r3, 42 of 42 files at r4.
Review status: all files reviewed at latest revision, 37 unresolved discussions, some commit checks failed.


doc/libpmemobj.3.md, line 2262 at r4 (raw file):

tx.post_commit.queue_depth | rw | - | int | int | integer

Controls the depth of the post-commit tasks queue. A post-commit task is the

What if I configure queue_depth to some value, but no PC thread is running?
Would it fall-back to the normal mode, like w/o any post-commit queue?


doc/libpmemobj.3.md, line 2276 at r4 (raw file):

will happen if the queue depth value is too large.

As a general rule, this value should be set to around 1024 - average number of

...and it must be a power of two


doc/libpmemobj.3.md, line 2283 at r4 (raw file):

currently executed.

Always returns 0.

unless the ringbuf allocation fails...
or if the value is not a power of two...


doc/libpmemobj.3.md, line 2285 at r4 (raw file):

Always returns 0.

tx.post_commit.worker | r- | - | void * | - | -

It appears a bit counter-intuitive to me that one need to issue read (get) to trigger the working function. For me, a write (set) would be better (value could be ignored). Same applies to tx.post_commit.stop.
Or, if it's get, then let it return something (i.e. a number of committed transactions, etc.)


doc/libpmemobj.3.md, line 2294 at r4 (raw file):

Always returns 0.

tx.post_commit.worker | r- | - | void * | - | -

.stop ?


doc/libpmemobj.3.md, line 2296 at r4 (raw file):

tx.post_commit.worker | r- | - | void * | - | -

This function forces all the post-commit worker functions to exit and return

What happens with the tasks in the post-commit queue? Does it have to be empty?
Shall we handle it somehow?


src/libpmemobj/lane.c, line 446 at r2 (raw file):

}

void

missing brief


src/libpmemobj/ringbuf.c, line 71 at r4 (raw file):

{
	/* length must be a power of two due to masking */
	ASSERT(util_popcount(length) <= 1);

Was util_popcount() added only to be used in this assertion?
What about: ASSERTeq(length & (~length + 1), length); /* is power of 2 */


src/libpmemobj/ringbuf.c, line 122 at r4 (raw file):

	/* XXX just unlock all waiting threads somehow... */
	for (uint64_t i = 0; i < 1024; ++i)

1024? Don't we have a named constant for that. Or better read the number of lanes from the pool descriptor.


src/libpmemobj/ringbuf.c, line 134 at r4 (raw file):

	os_semaphore_delete(rbuf->nfree);
	os_semaphore_delete(rbuf->nused);
	Free(rbuf);

What if there are any tasks enqueued? An assertion is a minimum:
ASSERTeq(read-pos, write_pos); ?


src/libpmemobj/ringbuf.c, line 162 at r4 (raw file):

 */
int
ringbuf_enqueue(struct ringbuf *rbuf, void *data)

Is it actually used anywhere (except the unit tests)? Isn't "tryenqueue" enough?


src/libpmemobj/ringbuf.c, line 222 at r4 (raw file):

 */
void *
ringbuf_dequeue(struct ringbuf *rbuf)

not used in other files (could be static / merged with dequeue_s)


src/libpmemobj/ringbuf.c, line 239 at r4 (raw file):

 */
void *
ringbuf_trydequeue(struct ringbuf *rbuf)

not used (except for unit test)


src/libpmemobj/ringbuf.c, line 255 at r4 (raw file):

 */
void *
ringbuf_trydequeue_s(struct ringbuf *rbuf, size_t data_size)

not used


src/libpmemobj/ringbuf.c, line 268 at r4 (raw file):

 */
void *
ringbuf_dequeue_s(struct ringbuf *rbuf, size_t data_size)

As I can see, only the "_s" variant is actually used in tx.c. So, what's the point in having "ringbuf_dequeue"?


src/libpmemobj/ringbuf.c, line 271 at r4 (raw file):

{
	void *r = ringbuf_dequeue(rbuf);
	VALGRIND_ANNOTATE_NEW_MEMORY(r, data_size);

Could you add more info/comment on why this is needed?


src/libpmemobj/tx.c, line 2211 at r4 (raw file):

	int arg_in = *(int *)arg;

	struct ringbuf *ntasks = ringbuf_new((unsigned)arg_in);

Any check if arg_in value is a power of two?
There is an assertion in ringbuf_new(), but no other handling of invalid length.


src/libpmemobj/tx.c, line 2216 at r4 (raw file):

	if (pop->tx_postcommit_tasks != NULL) {
		ringbuf_delete(pop->tx_postcommit_tasks);

What if it wasn't empty?


src/test/obj_async_postcommit/obj_async_postcommit.c, line 34 at r4 (raw file):

/*
 * obj_async_postcommit.c -- tests for the ctl entry points: prefault

Pls, fix the description.
Also, some more detailed description of the test scenario is welcome.
As I understand, you first spawn 4 PC threads, waiting for TX to be committed. Then another 4 worker threads, each performing 1000 transactions (4000 in total).
PC threads are expected to do post-commit cleanups in parallel to the worker threads, but how this test verifies it really takes place? How does it verify the non-zero queue depth has any effect?


src/test/obj_async_postcommit/obj_async_postcommit.c, line 87 at r4 (raw file):

	if (argc != 2)
		UT_FATAL("usage: %s file-name",

no need to break the line


src/test/obj_async_postcommit/obj_async_postcommit.c, line 95 at r4 (raw file):

	if ((pop = pmemobj_create(path, LAYOUT, PMEMOBJ_MIN_POOL * 10,
			S_IWUSR | S_IRUSR)) == NULL)
			UT_FATAL("!pmemobj_create: %s", path);

unindent by 1 tab


src/test/obj_async_postcommit/obj_async_postcommit.c, line 99 at r4 (raw file):

	pthread_t pc_worker[PC_WORKERS];

	if (PC_WORKERS) {

if (true) ?


src/test/obj_async_postcommit/obj_async_postcommit.c, line 103 at r4 (raw file):

		pmemobj_ctl_set(pop, "tx.post_commit.queue_depth", &qdepth);
		for (int i = 0; i < PC_WORKERS; ++i) {
			pthread_create(&pc_worker[i],

PTHREAD_CREATE()


src/test/obj_async_postcommit/obj_async_postcommit.c, line 128 at r4 (raw file):

	}

	if (PC_WORKERS) {

ditto


src/test/obj_async_postcommit/obj_async_postcommit.filters, line 1 at r4 (raw file):

<?xml version="1.0" encoding="utf-8"?>

*.vcxproj.filters"


src/test/obj_async_postcommit/obj_async_postcommit.vcxproj, line 69 at r4 (raw file):

    <ClCompile>
      <PrecompiledHeader>NotUsing</PrecompiledHeader>
      <CompileAs>CompileAsCpp</CompileAs>

Is that really needed (compiling as C++)?


src/test/obj_async_postcommit/TEST0.PS1, line 41 at r4 (raw file):

    $DIR = ""
    )
$ENV:UNITTEST_NAME = "obj_pvector\TEST0"

obj_pvector ?


src/test/obj_ringbuf/obj_ringbuf.filters, line 1 at r4 (raw file):

<?xml version="1.0" encoding="utf-8"?>

"obj_ringbuf.vcxproj.filters"


src/test/obj_ringbuf/obj_ringbuf.vcxproj, line 50 at r4 (raw file):

  <ItemDefinitionGroup Condition="'$(Configuration)|$(Platform)'=='Debug|x64'">
    <ClCompile>
      <Optimization>Disabled</Optimization>

optimization settings should be taken from test_xxx.props


src/test/obj_ringbuf/obj_ringbuf.vcxproj, line 93 at r4 (raw file):

      <Project>{492baa3d-0d5d-478e-9765-500463ae69aa}</Project>
    </ProjectReference>
    <ProjectReference Include="..\..\libpmemobj\libpmemobj.vcxproj">

Does it still depend on libpmemobj, if most (if not all) of the obj files are compiled/linked directly?


src/test/obj_ringbuf/TEST0, line 44 at r4 (raw file):


require_test_type medium

require_fs_type any


Comments from Reviewable

Contributor

krzycz commented May 3, 2017

  1. It would be nice to have some example and/or benchmark that illustrates how to use this feature and/or measures the performance gain.
    My first thought was to simply re-use the existing examples/benchmarks, but run it with some non-default CTL settings (env var), but I realized it's not that easy in case of async post-commit, as you need to explicitly run the PC thread.

  2. Please, use LOG/ERR macros!!!


Reviewed 4 of 10 files at r3, 42 of 42 files at r4.
Review status: all files reviewed at latest revision, 37 unresolved discussions, some commit checks failed.


doc/libpmemobj.3.md, line 2262 at r4 (raw file):

tx.post_commit.queue_depth | rw | - | int | int | integer

Controls the depth of the post-commit tasks queue. A post-commit task is the

What if I configure queue_depth to some value, but no PC thread is running?
Would it fall-back to the normal mode, like w/o any post-commit queue?


doc/libpmemobj.3.md, line 2276 at r4 (raw file):

will happen if the queue depth value is too large.

As a general rule, this value should be set to around 1024 - average number of

...and it must be a power of two


doc/libpmemobj.3.md, line 2283 at r4 (raw file):

currently executed.

Always returns 0.

unless the ringbuf allocation fails...
or if the value is not a power of two...


doc/libpmemobj.3.md, line 2285 at r4 (raw file):

Always returns 0.

tx.post_commit.worker | r- | - | void * | - | -

It appears a bit counter-intuitive to me that one need to issue read (get) to trigger the working function. For me, a write (set) would be better (value could be ignored). Same applies to tx.post_commit.stop.
Or, if it's get, then let it return something (i.e. a number of committed transactions, etc.)


doc/libpmemobj.3.md, line 2294 at r4 (raw file):

Always returns 0.

tx.post_commit.worker | r- | - | void * | - | -

.stop ?


doc/libpmemobj.3.md, line 2296 at r4 (raw file):

tx.post_commit.worker | r- | - | void * | - | -

This function forces all the post-commit worker functions to exit and return

What happens with the tasks in the post-commit queue? Does it have to be empty?
Shall we handle it somehow?


src/libpmemobj/lane.c, line 446 at r2 (raw file):

}

void

missing brief


src/libpmemobj/ringbuf.c, line 71 at r4 (raw file):

{
	/* length must be a power of two due to masking */
	ASSERT(util_popcount(length) <= 1);

Was util_popcount() added only to be used in this assertion?
What about: ASSERTeq(length & (~length + 1), length); /* is power of 2 */


src/libpmemobj/ringbuf.c, line 122 at r4 (raw file):

	/* XXX just unlock all waiting threads somehow... */
	for (uint64_t i = 0; i < 1024; ++i)

1024? Don't we have a named constant for that. Or better read the number of lanes from the pool descriptor.


src/libpmemobj/ringbuf.c, line 134 at r4 (raw file):

	os_semaphore_delete(rbuf->nfree);
	os_semaphore_delete(rbuf->nused);
	Free(rbuf);

What if there are any tasks enqueued? An assertion is a minimum:
ASSERTeq(read-pos, write_pos); ?


src/libpmemobj/ringbuf.c, line 162 at r4 (raw file):

 */
int
ringbuf_enqueue(struct ringbuf *rbuf, void *data)

Is it actually used anywhere (except the unit tests)? Isn't "tryenqueue" enough?


src/libpmemobj/ringbuf.c, line 222 at r4 (raw file):

 */
void *
ringbuf_dequeue(struct ringbuf *rbuf)

not used in other files (could be static / merged with dequeue_s)


src/libpmemobj/ringbuf.c, line 239 at r4 (raw file):

 */
void *
ringbuf_trydequeue(struct ringbuf *rbuf)

not used (except for unit test)


src/libpmemobj/ringbuf.c, line 255 at r4 (raw file):

 */
void *
ringbuf_trydequeue_s(struct ringbuf *rbuf, size_t data_size)

not used


src/libpmemobj/ringbuf.c, line 268 at r4 (raw file):

 */
void *
ringbuf_dequeue_s(struct ringbuf *rbuf, size_t data_size)

As I can see, only the "_s" variant is actually used in tx.c. So, what's the point in having "ringbuf_dequeue"?


src/libpmemobj/ringbuf.c, line 271 at r4 (raw file):

{
	void *r = ringbuf_dequeue(rbuf);
	VALGRIND_ANNOTATE_NEW_MEMORY(r, data_size);

Could you add more info/comment on why this is needed?


src/libpmemobj/tx.c, line 2211 at r4 (raw file):

	int arg_in = *(int *)arg;

	struct ringbuf *ntasks = ringbuf_new((unsigned)arg_in);

Any check if arg_in value is a power of two?
There is an assertion in ringbuf_new(), but no other handling of invalid length.


src/libpmemobj/tx.c, line 2216 at r4 (raw file):

	if (pop->tx_postcommit_tasks != NULL) {
		ringbuf_delete(pop->tx_postcommit_tasks);

What if it wasn't empty?


src/test/obj_async_postcommit/obj_async_postcommit.c, line 34 at r4 (raw file):

/*
 * obj_async_postcommit.c -- tests for the ctl entry points: prefault

Pls, fix the description.
Also, some more detailed description of the test scenario is welcome.
As I understand, you first spawn 4 PC threads, waiting for TX to be committed. Then another 4 worker threads, each performing 1000 transactions (4000 in total).
PC threads are expected to do post-commit cleanups in parallel to the worker threads, but how this test verifies it really takes place? How does it verify the non-zero queue depth has any effect?


src/test/obj_async_postcommit/obj_async_postcommit.c, line 87 at r4 (raw file):

	if (argc != 2)
		UT_FATAL("usage: %s file-name",

no need to break the line


src/test/obj_async_postcommit/obj_async_postcommit.c, line 95 at r4 (raw file):

	if ((pop = pmemobj_create(path, LAYOUT, PMEMOBJ_MIN_POOL * 10,
			S_IWUSR | S_IRUSR)) == NULL)
			UT_FATAL("!pmemobj_create: %s", path);

unindent by 1 tab


src/test/obj_async_postcommit/obj_async_postcommit.c, line 99 at r4 (raw file):

	pthread_t pc_worker[PC_WORKERS];

	if (PC_WORKERS) {

if (true) ?


src/test/obj_async_postcommit/obj_async_postcommit.c, line 103 at r4 (raw file):

		pmemobj_ctl_set(pop, "tx.post_commit.queue_depth", &qdepth);
		for (int i = 0; i < PC_WORKERS; ++i) {
			pthread_create(&pc_worker[i],

PTHREAD_CREATE()


src/test/obj_async_postcommit/obj_async_postcommit.c, line 128 at r4 (raw file):

	}

	if (PC_WORKERS) {

ditto


src/test/obj_async_postcommit/obj_async_postcommit.filters, line 1 at r4 (raw file):

<?xml version="1.0" encoding="utf-8"?>

*.vcxproj.filters"


src/test/obj_async_postcommit/obj_async_postcommit.vcxproj, line 69 at r4 (raw file):

    <ClCompile>
      <PrecompiledHeader>NotUsing</PrecompiledHeader>
      <CompileAs>CompileAsCpp</CompileAs>

Is that really needed (compiling as C++)?


src/test/obj_async_postcommit/TEST0.PS1, line 41 at r4 (raw file):

    $DIR = ""
    )
$ENV:UNITTEST_NAME = "obj_pvector\TEST0"

obj_pvector ?


src/test/obj_ringbuf/obj_ringbuf.filters, line 1 at r4 (raw file):

<?xml version="1.0" encoding="utf-8"?>

"obj_ringbuf.vcxproj.filters"


src/test/obj_ringbuf/obj_ringbuf.vcxproj, line 50 at r4 (raw file):

  <ItemDefinitionGroup Condition="'$(Configuration)|$(Platform)'=='Debug|x64'">
    <ClCompile>
      <Optimization>Disabled</Optimization>

optimization settings should be taken from test_xxx.props


src/test/obj_ringbuf/obj_ringbuf.vcxproj, line 93 at r4 (raw file):

      <Project>{492baa3d-0d5d-478e-9765-500463ae69aa}</Project>
    </ProjectReference>
    <ProjectReference Include="..\..\libpmemobj\libpmemobj.vcxproj">

Does it still depend on libpmemobj, if most (if not all) of the obj files are compiled/linked directly?


src/test/obj_ringbuf/TEST0, line 44 at r4 (raw file):


require_test_type medium

require_fs_type any


Comments from Reviewable

@pbalcer

This comment has been minimized.

Show comment
Hide comment
@pbalcer

pbalcer May 4, 2017

Contributor
  1. The benefit from this is very workload dependent, and so the benchmarks we have won't show much improvements. I'll create a simple example as a separate pull request. The test shows you how to use it.

  2. I don't see much benefit in using LOG macros at the entrance of every function, we are effectively creating a poor man'd gdb, but fine, I'll try to remember about this ;)
    Error strings and errno should be only set at the outermost layer of the library, otherwise we will end up with a mess of error indicators being overwritten all over the place.


Review status: 40 of 53 files reviewed at latest revision, 37 unresolved discussions.


doc/libpmemobj.3.md, line 2262 at r4 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

What if I configure queue_depth to some value, but no PC thread is running?
Would it fall-back to the normal mode, like w/o any post-commit queue?

Why would it? It does what you would expect: there's no special behavior associated. And how would you detect it anyway?

Maybe I'm not understanding your question?

If you don't launch the worker entry point, you will never cleanup the lanes data and thus will end up several lanes locked up until you do launch the threads. I think that is the least surprising thing to do.


doc/libpmemobj.3.md, line 2276 at r4 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

...and it must be a power of two

Done.


doc/libpmemobj.3.md, line 2283 at r4 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

unless the ringbuf allocation fails...
or if the value is not a power of two...

Done.


doc/libpmemobj.3.md, line 2285 at r4 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

It appears a bit counter-intuitive to me that one need to issue read (get) to trigger the working function. For me, a write (set) would be better (value could be ignored). Same applies to tx.post_commit.stop.
Or, if it's get, then let it return something (i.e. a number of committed transactions, etc.)

write functions can be set from config and require a parser for the argument. In the very first version of the CTL patch (and in the initial design) there was a specific "trigger" type, but it got axed when everyone suggested to use separate ctl_get/ctl_set.


doc/libpmemobj.3.md, line 2294 at r4 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

.stop ?

Done.


doc/libpmemobj.3.md, line 2296 at r4 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

What happens with the tasks in the post-commit queue? Does it have to be empty?
Shall we handle it somehow?

It doesn't have to be empty, and nothing would have happened, the lanes would have been simply cleaned up on recovery.
But nevertheless, I've changed the implementation a bit so that the stop method now waits for all pending tasks to be completed.


src/libpmemobj/lane.c, line 446 at r2 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

missing brief

Done.


src/libpmemobj/obj.c, line 1027 at r1 (raw file):

Previously, plebioda (Pawel Lebioda) wrote…

bump ?

Done.


src/libpmemobj/ringbuf.c, line 78 at r1 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

missing error handling (same applies to all the calls to sem_*() APIs - sem_wait/sem_post/...)

Done.


src/libpmemobj/ringbuf.c, line 71 at r4 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

Was util_popcount() added only to be used in this assertion?
What about: ASSERTeq(length & (~length + 1), length); /* is power of 2 */

Yes. Does it hurt us to add this function?
I'd argue my way is more readable and faster.


src/libpmemobj/ringbuf.c, line 122 at r4 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

1024? Don't we have a named constant for that. Or better read the number of lanes from the pool descriptor.

This is generic code, I don't want to make a dependency on lanes.
It's still not ideal, but I don't think there's a better way that doesn't involve adding extraneous variables or timed waits.


src/libpmemobj/ringbuf.c, line 134 at r4 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

What if there are any tasks enqueued? An assertion is a minimum:
ASSERTeq(read-pos, write_pos); ?

Initially I wanted to reply that this is implementation dependent, and the user might want to delete the ringbuf with its contents - the ring buffer itself doesn't associate any of its own resources with the user data and so there's nothing that might leak.
But then I decided that in 99% of the use cases, we would only want to call delete on an empty container, so I added it, we can always change it.


src/libpmemobj/ringbuf.c, line 162 at r4 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

Is it actually used anywhere (except the unit tests)? Isn't "tryenqueue" enough?

All variants are provided for consistency sake.


src/libpmemobj/ringbuf.c, line 271 at r4 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

Could you add more info/comment on why this is needed?

Done.


src/libpmemobj/ringbuf.h, line 34 at r1 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

mp_M_c?

Done.


src/libpmemobj/ringbuf.h, line 46 at r1 (raw file):

Previously, plebioda (Pawel Lebioda) wrote…

size_t ?

Why?


src/libpmemobj/tx.c, line 806 at r1 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

Hmmm, don't we need to check if stage is NONE?

Nope, the pop variable will be NULL if there's no transaction.


src/libpmemobj/tx.c, line 1279 at r1 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

necessary

Done.


src/libpmemobj/tx.c, line 2211 at r4 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

Any check if arg_in value is a power of two?
There is an assertion in ringbuf_new(), but no other handling of invalid length.

Done.


src/libpmemobj/tx.c, line 2216 at r4 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

What if it wasn't empty?

Done.


src/test/obj_async_postcommit/obj_async_postcommit.c, line 34 at r4 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

Pls, fix the description.
Also, some more detailed description of the test scenario is welcome.
As I understand, you first spawn 4 PC threads, waiting for TX to be committed. Then another 4 worker threads, each performing 1000 transactions (4000 in total).
PC threads are expected to do post-commit cleanups in parallel to the worker threads, but how this test verifies it really takes place? How does it verify the non-zero queue depth has any effect?

I've reworked this test somehow to fail in a more significant way if something breaks. If the post commit workers don't work it will now hang and eventually time out.
Apart from intercepting somehow library internals, there's really no other way to test if the cleanups are actually deferred. Or maybe you have any ideas?


src/test/obj_async_postcommit/obj_async_postcommit.c, line 87 at r4 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

no need to break the line

Done.


src/test/obj_async_postcommit/obj_async_postcommit.c, line 95 at r4 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

unindent by 1 tab

Done.


src/test/obj_async_postcommit/obj_async_postcommit.c, line 99 at r4 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

if (true) ?

Done.


src/test/obj_async_postcommit/obj_async_postcommit.c, line 103 at r4 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

PTHREAD_CREATE()

Done.


src/test/obj_async_postcommit/obj_async_postcommit.c, line 128 at r4 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

ditto

Done.


src/test/obj_async_postcommit/obj_async_postcommit.vcxproj, line 69 at r4 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

Is that really needed (compiling as C++)?

Does it matter? I copy/pasted the test project.


src/test/obj_async_postcommit/TEST0.PS1, line 41 at r4 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

obj_pvector ?

Done.


src/test/obj_ringbuf/obj_ringbuf.vcxproj, line 50 at r4 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

optimization settings should be taken from test_xxx.props

I copy pasted your changes, it's like that in every project file I checked.


src/test/obj_ringbuf/obj_ringbuf.vcxproj, line 93 at r4 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

Does it still depend on libpmemobj, if most (if not all) of the obj files are compiled/linked directly?

Again, does it matter? It's like that everywhere.


src/test/obj_ringbuf/TEST0, line 44 at r4 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

require_fs_type any

Done.


src/test/obj_async_postcommit/obj_async_postcommit.filters, line 1 at r4 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

*.vcxproj.filters"

Done.


src/test/obj_ringbuf/obj_ringbuf.filters, line 1 at r4 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

"obj_ringbuf.vcxproj.filters"

Done.


Comments from Reviewable

Contributor

pbalcer commented May 4, 2017

  1. The benefit from this is very workload dependent, and so the benchmarks we have won't show much improvements. I'll create a simple example as a separate pull request. The test shows you how to use it.

  2. I don't see much benefit in using LOG macros at the entrance of every function, we are effectively creating a poor man'd gdb, but fine, I'll try to remember about this ;)
    Error strings and errno should be only set at the outermost layer of the library, otherwise we will end up with a mess of error indicators being overwritten all over the place.


Review status: 40 of 53 files reviewed at latest revision, 37 unresolved discussions.


doc/libpmemobj.3.md, line 2262 at r4 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

What if I configure queue_depth to some value, but no PC thread is running?
Would it fall-back to the normal mode, like w/o any post-commit queue?

Why would it? It does what you would expect: there's no special behavior associated. And how would you detect it anyway?

Maybe I'm not understanding your question?

If you don't launch the worker entry point, you will never cleanup the lanes data and thus will end up several lanes locked up until you do launch the threads. I think that is the least surprising thing to do.


doc/libpmemobj.3.md, line 2276 at r4 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

...and it must be a power of two

Done.


doc/libpmemobj.3.md, line 2283 at r4 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

unless the ringbuf allocation fails...
or if the value is not a power of two...

Done.


doc/libpmemobj.3.md, line 2285 at r4 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

It appears a bit counter-intuitive to me that one need to issue read (get) to trigger the working function. For me, a write (set) would be better (value could be ignored). Same applies to tx.post_commit.stop.
Or, if it's get, then let it return something (i.e. a number of committed transactions, etc.)

write functions can be set from config and require a parser for the argument. In the very first version of the CTL patch (and in the initial design) there was a specific "trigger" type, but it got axed when everyone suggested to use separate ctl_get/ctl_set.


doc/libpmemobj.3.md, line 2294 at r4 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

.stop ?

Done.


doc/libpmemobj.3.md, line 2296 at r4 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

What happens with the tasks in the post-commit queue? Does it have to be empty?
Shall we handle it somehow?

It doesn't have to be empty, and nothing would have happened, the lanes would have been simply cleaned up on recovery.
But nevertheless, I've changed the implementation a bit so that the stop method now waits for all pending tasks to be completed.


src/libpmemobj/lane.c, line 446 at r2 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

missing brief

Done.


src/libpmemobj/obj.c, line 1027 at r1 (raw file):

Previously, plebioda (Pawel Lebioda) wrote…

bump ?

Done.


src/libpmemobj/ringbuf.c, line 78 at r1 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

missing error handling (same applies to all the calls to sem_*() APIs - sem_wait/sem_post/...)

Done.


src/libpmemobj/ringbuf.c, line 71 at r4 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

Was util_popcount() added only to be used in this assertion?
What about: ASSERTeq(length & (~length + 1), length); /* is power of 2 */

Yes. Does it hurt us to add this function?
I'd argue my way is more readable and faster.


src/libpmemobj/ringbuf.c, line 122 at r4 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

1024? Don't we have a named constant for that. Or better read the number of lanes from the pool descriptor.

This is generic code, I don't want to make a dependency on lanes.
It's still not ideal, but I don't think there's a better way that doesn't involve adding extraneous variables or timed waits.


src/libpmemobj/ringbuf.c, line 134 at r4 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

What if there are any tasks enqueued? An assertion is a minimum:
ASSERTeq(read-pos, write_pos); ?

Initially I wanted to reply that this is implementation dependent, and the user might want to delete the ringbuf with its contents - the ring buffer itself doesn't associate any of its own resources with the user data and so there's nothing that might leak.
But then I decided that in 99% of the use cases, we would only want to call delete on an empty container, so I added it, we can always change it.


src/libpmemobj/ringbuf.c, line 162 at r4 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

Is it actually used anywhere (except the unit tests)? Isn't "tryenqueue" enough?

All variants are provided for consistency sake.


src/libpmemobj/ringbuf.c, line 271 at r4 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

Could you add more info/comment on why this is needed?

Done.


src/libpmemobj/ringbuf.h, line 34 at r1 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

mp_M_c?

Done.


src/libpmemobj/ringbuf.h, line 46 at r1 (raw file):

Previously, plebioda (Pawel Lebioda) wrote…

size_t ?

Why?


src/libpmemobj/tx.c, line 806 at r1 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

Hmmm, don't we need to check if stage is NONE?

Nope, the pop variable will be NULL if there's no transaction.


src/libpmemobj/tx.c, line 1279 at r1 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

necessary

Done.


src/libpmemobj/tx.c, line 2211 at r4 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

Any check if arg_in value is a power of two?
There is an assertion in ringbuf_new(), but no other handling of invalid length.

Done.


src/libpmemobj/tx.c, line 2216 at r4 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

What if it wasn't empty?

Done.


src/test/obj_async_postcommit/obj_async_postcommit.c, line 34 at r4 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

Pls, fix the description.
Also, some more detailed description of the test scenario is welcome.
As I understand, you first spawn 4 PC threads, waiting for TX to be committed. Then another 4 worker threads, each performing 1000 transactions (4000 in total).
PC threads are expected to do post-commit cleanups in parallel to the worker threads, but how this test verifies it really takes place? How does it verify the non-zero queue depth has any effect?

I've reworked this test somehow to fail in a more significant way if something breaks. If the post commit workers don't work it will now hang and eventually time out.
Apart from intercepting somehow library internals, there's really no other way to test if the cleanups are actually deferred. Or maybe you have any ideas?


src/test/obj_async_postcommit/obj_async_postcommit.c, line 87 at r4 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

no need to break the line

Done.


src/test/obj_async_postcommit/obj_async_postcommit.c, line 95 at r4 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

unindent by 1 tab

Done.


src/test/obj_async_postcommit/obj_async_postcommit.c, line 99 at r4 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

if (true) ?

Done.


src/test/obj_async_postcommit/obj_async_postcommit.c, line 103 at r4 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

PTHREAD_CREATE()

Done.


src/test/obj_async_postcommit/obj_async_postcommit.c, line 128 at r4 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

ditto

Done.


src/test/obj_async_postcommit/obj_async_postcommit.vcxproj, line 69 at r4 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

Is that really needed (compiling as C++)?

Does it matter? I copy/pasted the test project.


src/test/obj_async_postcommit/TEST0.PS1, line 41 at r4 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

obj_pvector ?

Done.


src/test/obj_ringbuf/obj_ringbuf.vcxproj, line 50 at r4 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

optimization settings should be taken from test_xxx.props

I copy pasted your changes, it's like that in every project file I checked.


src/test/obj_ringbuf/obj_ringbuf.vcxproj, line 93 at r4 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

Does it still depend on libpmemobj, if most (if not all) of the obj files are compiled/linked directly?

Again, does it matter? It's like that everywhere.


src/test/obj_ringbuf/TEST0, line 44 at r4 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

require_fs_type any

Done.


src/test/obj_async_postcommit/obj_async_postcommit.filters, line 1 at r4 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

*.vcxproj.filters"

Done.


src/test/obj_ringbuf/obj_ringbuf.filters, line 1 at r4 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

"obj_ringbuf.vcxproj.filters"

Done.


Comments from Reviewable

@krzycz

This comment has been minimized.

Show comment
Hide comment
@krzycz

krzycz May 11, 2017

Contributor

Reviewed 12 of 15 files at r5.
Review status: 50 of 53 files reviewed at latest revision, 19 unresolved discussions, some commit checks failed.


src/libpmemobj/ringbuf.c, line 71 at r4 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

Yes. Does it hurt us to add this function?
I'd argue my way is more readable and faster.

No, it doesn't. But instead of just one line, we have a


Comments from Reviewable

Contributor

krzycz commented May 11, 2017

Reviewed 12 of 15 files at r5.
Review status: 50 of 53 files reviewed at latest revision, 19 unresolved discussions, some commit checks failed.


src/libpmemobj/ringbuf.c, line 71 at r4 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

Yes. Does it hurt us to add this function?
I'd argue my way is more readable and faster.

No, it doesn't. But instead of just one line, we have a


Comments from Reviewable

@krzycz

This comment has been minimized.

Show comment
Hide comment
@krzycz

krzycz May 11, 2017

Contributor

Review status: 50 of 53 files reviewed at latest revision, 19 unresolved discussions, some commit checks failed.


src/libpmemobj/ringbuf.c, line 71 at r4 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

No, it doesn't. But instead of just one line, we have a

...a single line plus a few more in another file. Just a detail...
It's like - I can do x = a * a, but why not to have a new functionsqr(a) that does exactly the same using the CPU instruction SQR (if there is one), and then do x = sqr(a)?
Is it more readable? I'd argue. IMHO the most important is the comment that says everything in both cases.
Is it faster? Maybe... For debug we don't really care. But, now it's used in non-debug as well, so it (theoretically) can make a difference. However..., how often do you expect ringbuf_new()will be called - 1000x per second or... just once?


Comments from Reviewable

Contributor

krzycz commented May 11, 2017

Review status: 50 of 53 files reviewed at latest revision, 19 unresolved discussions, some commit checks failed.


src/libpmemobj/ringbuf.c, line 71 at r4 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

No, it doesn't. But instead of just one line, we have a

...a single line plus a few more in another file. Just a detail...
It's like - I can do x = a * a, but why not to have a new functionsqr(a) that does exactly the same using the CPU instruction SQR (if there is one), and then do x = sqr(a)?
Is it more readable? I'd argue. IMHO the most important is the comment that says everything in both cases.
Is it faster? Maybe... For debug we don't really care. But, now it's used in non-debug as well, so it (theoretically) can make a difference. However..., how often do you expect ringbuf_new()will be called - 1000x per second or... just once?


Comments from Reviewable

@krzycz

This comment has been minimized.

Show comment
Hide comment
@krzycz

krzycz May 15, 2017

Contributor

Reviewed 3 of 15 files at r5.
Review status: all files reviewed at latest revision, 11 unresolved discussions, some commit checks failed.


doc/libpmemobj.3.md, line 2262 at r4 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

Why would it? It does what you would expect: there's no special behavior associated. And how would you detect it anyway?

Maybe I'm not understanding your question?

If you don't launch the worker entry point, you will never cleanup the lanes data and thus will end up several lanes locked up until you do launch the threads. I think that is the least surprising thing to do.

Each PC worker thread must be explicitly spawned, so you know if there are any of them running. But as I understand, the problem could be with the order of operations - whether you change the queue depth first, then spawn the PC threads (so for a moment, the queue depth is non-zero, but no PC threads are running), or if you start PC threads first, then change the queue depth (may lead to race conditions).

OK, let's leave it as is.


src/libpmemobj/ringbuf.c, line 71 at r4 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

...a single line plus a few more in another file. Just a detail...
It's like - I can do x = a * a, but why not to have a new functionsqr(a) that does exactly the same using the CPU instruction SQR (if there is one), and then do x = sqr(a)?
Is it more readable? I'd argue. IMHO the most important is the comment that says everything in both cases.
Is it faster? Maybe... For debug we don't really care. But, now it's used in non-debug as well, so it (theoretically) can make a difference. However..., how often do you expect ringbuf_new()will be called - 1000x per second or... just once?

Never mind - Maybe I'm picky on that just because when I grew up, there was no CPU having popcnt instruction. So, every programmer had to know how to do such a check using only basic arith operations, and I could never think about using popcnt for that purpose...


src/test/obj_async_postcommit/obj_async_postcommit.vcxproj, line 69 at r4 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

Does it matter? I copy/pasted the test project.

Not a big deal - just asking.


src/test/obj_ringbuf/obj_ringbuf.vcxproj, line 50 at r4 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

I copy pasted your changes, it's like that in every project file I checked.

So, it looks like I need to fix vcxproj files... ;-)


src/test/obj_ringbuf/obj_ringbuf.vcxproj, line 93 at r4 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

Again, does it matter? It's like that everywhere.

As most of the obj tests do link to libpmemobj...


Comments from Reviewable

Contributor

krzycz commented May 15, 2017

Reviewed 3 of 15 files at r5.
Review status: all files reviewed at latest revision, 11 unresolved discussions, some commit checks failed.


doc/libpmemobj.3.md, line 2262 at r4 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

Why would it? It does what you would expect: there's no special behavior associated. And how would you detect it anyway?

Maybe I'm not understanding your question?

If you don't launch the worker entry point, you will never cleanup the lanes data and thus will end up several lanes locked up until you do launch the threads. I think that is the least surprising thing to do.

Each PC worker thread must be explicitly spawned, so you know if there are any of them running. But as I understand, the problem could be with the order of operations - whether you change the queue depth first, then spawn the PC threads (so for a moment, the queue depth is non-zero, but no PC threads are running), or if you start PC threads first, then change the queue depth (may lead to race conditions).

OK, let's leave it as is.


src/libpmemobj/ringbuf.c, line 71 at r4 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

...a single line plus a few more in another file. Just a detail...
It's like - I can do x = a * a, but why not to have a new functionsqr(a) that does exactly the same using the CPU instruction SQR (if there is one), and then do x = sqr(a)?
Is it more readable? I'd argue. IMHO the most important is the comment that says everything in both cases.
Is it faster? Maybe... For debug we don't really care. But, now it's used in non-debug as well, so it (theoretically) can make a difference. However..., how often do you expect ringbuf_new()will be called - 1000x per second or... just once?

Never mind - Maybe I'm picky on that just because when I grew up, there was no CPU having popcnt instruction. So, every programmer had to know how to do such a check using only basic arith operations, and I could never think about using popcnt for that purpose...


src/test/obj_async_postcommit/obj_async_postcommit.vcxproj, line 69 at r4 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

Does it matter? I copy/pasted the test project.

Not a big deal - just asking.


src/test/obj_ringbuf/obj_ringbuf.vcxproj, line 50 at r4 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

I copy pasted your changes, it's like that in every project file I checked.

So, it looks like I need to fix vcxproj files... ;-)


src/test/obj_ringbuf/obj_ringbuf.vcxproj, line 93 at r4 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

Again, does it matter? It's like that everywhere.

As most of the obj tests do link to libpmemobj...


Comments from Reviewable

@krzycz

This comment has been minimized.

Show comment
Hide comment
@krzycz

krzycz May 15, 2017

Contributor
  1. Keep in mind that these macros are not (only) for us, but also for the users of NVML or the programs/applications based on NVML. Perhaps they don't even know how to use debugger. ;-) Sometimes, the only we can expect from them in case of a failure is to run their app on debug build with debug traces enabled and send the logs back to us.
    Also, in case of tests run on Travis/AppVeyor (where you can't log in and debug the problem) debug traces could be the only way to identify the root cause of the problem.
    And finally, sometimes the issue is not the program crash - which is easy to debug. It could be an error that results in graceful exit. Then the log could help you to see where to put the break point.

In general, I think it's very useful to have tracing points - as much precise as they could be. They don't affect non-debug builds, and for debug builds they can be disabled, so again, not a huge performance impact.


Review status: all files reviewed at latest revision, 11 unresolved discussions, some commit checks failed.


Comments from Reviewable

Contributor

krzycz commented May 15, 2017

  1. Keep in mind that these macros are not (only) for us, but also for the users of NVML or the programs/applications based on NVML. Perhaps they don't even know how to use debugger. ;-) Sometimes, the only we can expect from them in case of a failure is to run their app on debug build with debug traces enabled and send the logs back to us.
    Also, in case of tests run on Travis/AppVeyor (where you can't log in and debug the problem) debug traces could be the only way to identify the root cause of the problem.
    And finally, sometimes the issue is not the program crash - which is easy to debug. It could be an error that results in graceful exit. Then the log could help you to see where to put the break point.

In general, I think it's very useful to have tracing points - as much precise as they could be. They don't affect non-debug builds, and for debug builds they can be disabled, so again, not a huge performance impact.


Review status: all files reviewed at latest revision, 11 unresolved discussions, some commit checks failed.


Comments from Reviewable

@krzycz

This comment has been minimized.

Show comment
Hide comment
@krzycz

krzycz May 15, 2017

Contributor

:lgtm:


Review status: all files reviewed at latest revision, 11 unresolved discussions, some commit checks failed.


Comments from Reviewable

Contributor

krzycz commented May 15, 2017

:lgtm:


Review status: all files reviewed at latest revision, 11 unresolved discussions, some commit checks failed.


Comments from Reviewable

@krzycz

This comment has been minimized.

Show comment
Hide comment
@krzycz

krzycz May 16, 2017

Contributor

Reviewed 6 of 10 files at r6.
Review status: 51 of 55 files reviewed at latest revision, 11 unresolved discussions.


Comments from Reviewable

Contributor

krzycz commented May 16, 2017

Reviewed 6 of 10 files at r6.
Review status: 51 of 55 files reviewed at latest revision, 11 unresolved discussions.


Comments from Reviewable

@krzycz

This comment has been minimized.

Show comment
Hide comment
@krzycz

krzycz May 16, 2017

Contributor

Reviewed 4 of 10 files at r6.
Review status: all files reviewed at latest revision, 11 unresolved discussions.


Comments from Reviewable

Contributor

krzycz commented May 16, 2017

Reviewed 4 of 10 files at r6.
Review status: all files reviewed at latest revision, 11 unresolved discussions.


Comments from Reviewable

@krzycz krzycz merged commit f98b4e5 into pmem:master May 16, 2017

2 of 3 checks passed

code-review/reviewable 11 discussions left (krzycz, pbalcer, plebioda)
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment