obj: two-phase transactional aggregated heap operations #1711

Merged
merged 1 commit into from Dec 2, 2017

Conversation

Projects
None yet
6 participants
@pbalcer
Contributor

pbalcer commented Mar 1, 2017

This patch first has no user-visible changes, it just creates an internal API that allows for aggregated allocations. The only slightly different behavior is with locking, but it eventually boils down to almost exact same allocation process.

The second patch uses the new capabilities to improve throughput of allocating objects inside of a transaction.

This is 99.5% complete. There are two minor issues left, but feel free to review.


This change is Reviewable

@pbalcer pbalcer changed the title from obj: fully split palloc into volat and pmem parts to obj: two-phase transactional aggregated heap operations Mar 2, 2017

@pbalcer

This comment has been minimized.

Show comment
Hide comment
@pbalcer

pbalcer Mar 2, 2017

Contributor

Initial measurements show ~50% performance improvement, I'll prepare some charts later.

Contributor

pbalcer commented Mar 2, 2017

Initial measurements show ~50% performance improvement, I'll prepare some charts later.

@pbalcer

This comment has been minimized.

Show comment
Hide comment
@pbalcer

pbalcer Mar 3, 2017

Contributor

Here are the promised charts:

image

image

The benchmark was ran on my PC (i7-6700k, 4 cores, 8 threads) on tmpfs (/w PMEM_IS_PMEM_FORCE=1).

Contributor

pbalcer commented Mar 3, 2017

Here are the promised charts:

image

image

The benchmark was ran on my PC (i7-6700k, 4 cores, 8 threads) on tmpfs (/w PMEM_IS_PMEM_FORCE=1).

@pbalcer

This comment has been minimized.

Show comment
Hide comment
@pbalcer

pbalcer Mar 6, 2017

Contributor

I've added a third patch related to pmem/issues#415. It's complete, but lacks tests and documentation.

Contributor

pbalcer commented Mar 6, 2017

I've added a third patch related to pmem/issues#415. It's complete, but lacks tests and documentation.

@pbalcer

This comment has been minimized.

Show comment
Hide comment
@pbalcer

pbalcer Mar 10, 2017

Contributor

reservation API performance:
image

image

Contributor

pbalcer commented Mar 10, 2017

reservation API performance:
image

image

@plebioda

This comment has been minimized.

Show comment
Hide comment
@plebioda

plebioda Mar 17, 2017

Contributor

Reviewed 1 of 9 files at r1, 3 of 22 files at r2, 7 of 19 files at r3, 5 of 15 files at r4.
Review status: 16 of 47 files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.


src/include/libpmemobj/action.h, line 34 at r4 (raw file):

/*
 * libpmemobj/action.h -- definitions of libpmemobj action interface

don't like the name of this header, but don't have better name right now...


src/include/libpmemobj/action_base.h, line 47 at r4 (raw file):

enum pobj_action_type {
	POBJ_ACTION_TYPE_HEAP,

please add comments


src/include/libpmemobj/action_base.h, line 55 at r4 (raw file):

struct pobj_action {
	enum pobj_action_type type;
	uint32_t data[3];

use '_' or '__' for fields in a struct which should not be used by the user ?


src/include/libpmemobj/action_base.h, line 64 at r4 (raw file):

};

#define POBJ_MAX_PUBLISHABLE_ACTIONS 60

Where does this number come from ?

POBJ_MAX_ACTIONS the PUBLISHABLE is too long and looks weird


src/libpmemobj/palloc.c, line 49 at r4 (raw file):

#include "palloc.h"

struct pobj_action_internal {

I don't like it. At least you should add compile time asserts which check if this definition is fully compatible with the public one. Or a test ?


Comments from Reviewable

Contributor

plebioda commented Mar 17, 2017

Reviewed 1 of 9 files at r1, 3 of 22 files at r2, 7 of 19 files at r3, 5 of 15 files at r4.
Review status: 16 of 47 files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.


src/include/libpmemobj/action.h, line 34 at r4 (raw file):

/*
 * libpmemobj/action.h -- definitions of libpmemobj action interface

don't like the name of this header, but don't have better name right now...


src/include/libpmemobj/action_base.h, line 47 at r4 (raw file):

enum pobj_action_type {
	POBJ_ACTION_TYPE_HEAP,

please add comments


src/include/libpmemobj/action_base.h, line 55 at r4 (raw file):

struct pobj_action {
	enum pobj_action_type type;
	uint32_t data[3];

use '_' or '__' for fields in a struct which should not be used by the user ?


src/include/libpmemobj/action_base.h, line 64 at r4 (raw file):

};

#define POBJ_MAX_PUBLISHABLE_ACTIONS 60

Where does this number come from ?

POBJ_MAX_ACTIONS the PUBLISHABLE is too long and looks weird


src/libpmemobj/palloc.c, line 49 at r4 (raw file):

#include "palloc.h"

struct pobj_action_internal {

I don't like it. At least you should add compile time asserts which check if this definition is fully compatible with the public one. Or a test ?


Comments from Reviewable

@pbalcer

This comment has been minimized.

Show comment
Hide comment
@pbalcer

pbalcer Mar 20, 2017

Contributor

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


src/include/libpmemobj/action.h, line 34 at r4 (raw file):

Previously, plebioda (Pawel Lebioda) wrote…

don't like the name of this header, but don't have better name right now...

I don't like it either, but that's the best I've came up with ;)


src/include/libpmemobj/action_base.h, line 55 at r4 (raw file):

Previously, plebioda (Pawel Lebioda) wrote…

use '_' or '__' for fields in a struct which should not be used by the user ?

We don't do that anywhere else, besides, the _* namespace is reserved:
All identifiers that begin with an underscore and either an uppercase letter or another underscore are always reserved for any use.

All identifiers that begin with an underscore are always reserved for use as identifiers with file scope in both the ordinary and tag name spaces.

src/include/libpmemobj/action_base.h, line 64 at r4 (raw file):

Previously, plebioda (Pawel Lebioda) wrote…

Where does this number come from ?

POBJ_MAX_ACTIONS the PUBLISHABLE is too long and looks weird

1024 / 16 = 64
lane size / redo size = max actions

I agree on the name.


src/libpmemobj/palloc.c, line 49 at r4 (raw file):

Previously, plebioda (Pawel Lebioda) wrote…

I don't like it. At least you should add compile time asserts which check if this definition is fully compatible with the public one. Or a test ?

I've added macros below.


Comments from Reviewable

Contributor

pbalcer commented Mar 20, 2017

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


src/include/libpmemobj/action.h, line 34 at r4 (raw file):

Previously, plebioda (Pawel Lebioda) wrote…

don't like the name of this header, but don't have better name right now...

I don't like it either, but that's the best I've came up with ;)


src/include/libpmemobj/action_base.h, line 55 at r4 (raw file):

Previously, plebioda (Pawel Lebioda) wrote…

use '_' or '__' for fields in a struct which should not be used by the user ?

We don't do that anywhere else, besides, the _* namespace is reserved:
All identifiers that begin with an underscore and either an uppercase letter or another underscore are always reserved for any use.

All identifiers that begin with an underscore are always reserved for use as identifiers with file scope in both the ordinary and tag name spaces.

src/include/libpmemobj/action_base.h, line 64 at r4 (raw file):

Previously, plebioda (Pawel Lebioda) wrote…

Where does this number come from ?

POBJ_MAX_ACTIONS the PUBLISHABLE is too long and looks weird

1024 / 16 = 64
lane size / redo size = max actions

I agree on the name.


src/libpmemobj/palloc.c, line 49 at r4 (raw file):

Previously, plebioda (Pawel Lebioda) wrote…

I don't like it. At least you should add compile time asserts which check if this definition is fully compatible with the public one. Or a test ?

I've added macros below.


Comments from Reviewable

@janekmi

This comment has been minimized.

Show comment
Hide comment
@janekmi

janekmi Sep 12, 2017

Contributor

Reviewed 3 of 22 files at r2, 1 of 19 files at r3, 6 of 22 files at r5, 9 of 15 files at r6.
Review status: 22 of 43 files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.


src/include/libpmemobj/action_base.h, line 64 at r4 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

1024 / 16 = 64
lane size / redo size = max actions

I agree on the name.

Please add a comment where this number come from. Just to make this knowledge "persistent". ;-)


src/libpmemobj/memblock.c, line 665 at r6 (raw file):

		hdr->flags |= f;
		pmemops_persist(&m->heap->p_ops, hdr, sizeof(*hdr));
		VALGRIND_ADD_TO_TX(hdr, sizeof(*hdr));

Why hdr is added twice to Valgrind TX?


src/libpmemobj/palloc.c, line 71 at r6 (raw file):

/*
 * palloc_set_value -- creates a new set memory action

Please use first or third person verbs consistently.


src/libpmemobj/palloc.c, line 446 at r6 (raw file):

 *
 * Reallocation is a combination of the above, which one additional step
 * of copying the old content in the meantime.

In the meantime means between allocation of the new block and deallocation of the old one? I think it will be better not to use so imprecise expression.


Comments from Reviewable

Contributor

janekmi commented Sep 12, 2017

Reviewed 3 of 22 files at r2, 1 of 19 files at r3, 6 of 22 files at r5, 9 of 15 files at r6.
Review status: 22 of 43 files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.


src/include/libpmemobj/action_base.h, line 64 at r4 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

1024 / 16 = 64
lane size / redo size = max actions

I agree on the name.

Please add a comment where this number come from. Just to make this knowledge "persistent". ;-)


src/libpmemobj/memblock.c, line 665 at r6 (raw file):

		hdr->flags |= f;
		pmemops_persist(&m->heap->p_ops, hdr, sizeof(*hdr));
		VALGRIND_ADD_TO_TX(hdr, sizeof(*hdr));

Why hdr is added twice to Valgrind TX?


src/libpmemobj/palloc.c, line 71 at r6 (raw file):

/*
 * palloc_set_value -- creates a new set memory action

Please use first or third person verbs consistently.


src/libpmemobj/palloc.c, line 446 at r6 (raw file):

 *
 * Reallocation is a combination of the above, which one additional step
 * of copying the old content in the meantime.

In the meantime means between allocation of the new block and deallocation of the old one? I think it will be better not to use so imprecise expression.


Comments from Reviewable

@janekmi

This comment has been minimized.

Show comment
Hide comment
@janekmi

janekmi Sep 22, 2017

Contributor

Reviewed 2 of 22 files at r2, 1 of 19 files at r3, 7 of 15 files at r4, 5 of 22 files at r5, 6 of 15 files at r6.
Review status: all files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.


src/test/obj_pmalloc_basic/obj_pmalloc_basic.c, line 145 at r6 (raw file):

	}

	for (int i = 0; i < count; ++i) {

Why do you check this allocs twice?


Comments from Reviewable

Contributor

janekmi commented Sep 22, 2017

Reviewed 2 of 22 files at r2, 1 of 19 files at r3, 7 of 15 files at r4, 5 of 22 files at r5, 6 of 15 files at r6.
Review status: all files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.


src/test/obj_pmalloc_basic/obj_pmalloc_basic.c, line 145 at r6 (raw file):

	}

	for (int i = 0; i < count; ++i) {

Why do you check this allocs twice?


Comments from Reviewable

@pbalcer

This comment has been minimized.

Show comment
Hide comment
@pbalcer

pbalcer Sep 29, 2017

Contributor

Review status: 14 of 45 files reviewed at latest revision, 9 unresolved discussions.


src/libpmemobj/memblock.c, line 665 at r6 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Why hdr is added twice to Valgrind TX?

Done.


src/test/obj_pmalloc_basic/obj_pmalloc_basic.c, line 145 at r6 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Why do you check this allocs twice?

To make sure other frees don't affect the return value of this function.


Comments from Reviewable

Contributor

pbalcer commented Sep 29, 2017

Review status: 14 of 45 files reviewed at latest revision, 9 unresolved discussions.


src/libpmemobj/memblock.c, line 665 at r6 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Why hdr is added twice to Valgrind TX?

Done.


src/test/obj_pmalloc_basic/obj_pmalloc_basic.c, line 145 at r6 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Why do you check this allocs twice?

To make sure other frees don't affect the return value of this function.


Comments from Reviewable

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Oct 2, 2017

Codecov Report

Merging #1711 into master will increase coverage by 0.06%.
The diff coverage is 92.3%.

@@            Coverage Diff             @@
##           master    #1711      +/-   ##
==========================================
+ Coverage   80.57%   80.63%   +0.06%     
==========================================
  Files         134      134              
  Lines       20445    20607     +162     
  Branches     3554     3576      +22     
==========================================
+ Hits        16474    16617     +143     
- Misses       3971     3990      +19

codecov-io commented Oct 2, 2017

Codecov Report

Merging #1711 into master will increase coverage by 0.06%.
The diff coverage is 92.3%.

@@            Coverage Diff             @@
##           master    #1711      +/-   ##
==========================================
+ Coverage   80.57%   80.63%   +0.06%     
==========================================
  Files         134      134              
  Lines       20445    20607     +162     
  Branches     3554     3576      +22     
==========================================
+ Hits        16474    16617     +143     
- Misses       3971     3990      +19
@krzycz

This comment has been minimized.

Show comment
Hide comment
@krzycz

krzycz Oct 2, 2017

Contributor
  1. man page for new APIs is missing
  2. please, use LOG/ERR macros

Reviewed 2 of 9 files at r1, 11 of 22 files at r2, 10 of 19 files at r3, 13 of 15 files at r4, 16 of 22 files at r5, 9 of 15 files at r6, 27 of 31 files at r7.
Review status: 41 of 45 files reviewed at latest revision, 15 unresolved discussions.


src/common/util.h, line 148 at r5 (raw file):

#define util_bool_compare_and_swap64 __sync_bool_compare_and_swap
#define util_inc_and_fetch(ptr) __sync_add_and_fetch(ptr, 1)
#define util_sub_and_fetch(ptr) __sync_sub_and_fetch(ptr, 1)

util_dec_and_fetch() ?


src/include/libpmemobj/action.h, line 34 at r4 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

I don't like it either, but that's the best I've came up with ;)

reserve.h ?


src/include/libpmemobj/action_base.h, line 55 at r4 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

We don't do that anywhere else, besides, the _* namespace is reserved:
All identifiers that begin with an underscore and either an uppercase letter or another underscore are always reserved for any use.

All identifiers that begin with an underscore are always reserved for use as identifiers with file scope in both the ordinary and tag name spaces.

data => unused?


src/include/libpmemobj/action_base.h, line 64 at r4 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

1024 / 16 = 64
lane size / redo size = max actions

I agree on the name.

  1. 60 != 64
  2. Put this formula into the comment in the code.

src/test/obj_action/obj_action.vcxproj, line 84 at r7 (raw file):

      <DisableSpecificWarnings>
      </DisableSpecificWarnings>
      <PrecompiledHeaderFile>stdafx.h</PrecompiledHeaderFile>

stdafx.h ?


src/test/obj_action/TEST0.PS1, line 44 at r4 (raw file):

. ..\unittest\unittest.ps1

require_test_type medium

on Linux it's "short"


src/test/obj_persist_count/out0.log.match, line 12 at r4 (raw file):

0	;9	;0	;0	;tx_free
0	;8	;0	;0	;tx_free_next
0	;17	;0	;0	;tx_add

+1 ?


src/test/obj_persist_count/out1.log.match, line 12 at r4 (raw file):

8	;0	;1	;2	;tx_free
7	;0	;1	;2	;tx_free_next
11	;0	;3	;2	;tx_add

ditto


Comments from Reviewable

Contributor

krzycz commented Oct 2, 2017

  1. man page for new APIs is missing
  2. please, use LOG/ERR macros

Reviewed 2 of 9 files at r1, 11 of 22 files at r2, 10 of 19 files at r3, 13 of 15 files at r4, 16 of 22 files at r5, 9 of 15 files at r6, 27 of 31 files at r7.
Review status: 41 of 45 files reviewed at latest revision, 15 unresolved discussions.


src/common/util.h, line 148 at r5 (raw file):

#define util_bool_compare_and_swap64 __sync_bool_compare_and_swap
#define util_inc_and_fetch(ptr) __sync_add_and_fetch(ptr, 1)
#define util_sub_and_fetch(ptr) __sync_sub_and_fetch(ptr, 1)

util_dec_and_fetch() ?


src/include/libpmemobj/action.h, line 34 at r4 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

I don't like it either, but that's the best I've came up with ;)

reserve.h ?


src/include/libpmemobj/action_base.h, line 55 at r4 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

We don't do that anywhere else, besides, the _* namespace is reserved:
All identifiers that begin with an underscore and either an uppercase letter or another underscore are always reserved for any use.

All identifiers that begin with an underscore are always reserved for use as identifiers with file scope in both the ordinary and tag name spaces.

data => unused?


src/include/libpmemobj/action_base.h, line 64 at r4 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

1024 / 16 = 64
lane size / redo size = max actions

I agree on the name.

  1. 60 != 64
  2. Put this formula into the comment in the code.

src/test/obj_action/obj_action.vcxproj, line 84 at r7 (raw file):

      <DisableSpecificWarnings>
      </DisableSpecificWarnings>
      <PrecompiledHeaderFile>stdafx.h</PrecompiledHeaderFile>

stdafx.h ?


src/test/obj_action/TEST0.PS1, line 44 at r4 (raw file):

. ..\unittest\unittest.ps1

require_test_type medium

on Linux it's "short"


src/test/obj_persist_count/out0.log.match, line 12 at r4 (raw file):

0	;9	;0	;0	;tx_free
0	;8	;0	;0	;tx_free_next
0	;17	;0	;0	;tx_add

+1 ?


src/test/obj_persist_count/out1.log.match, line 12 at r4 (raw file):

8	;0	;1	;2	;tx_free
7	;0	;1	;2	;tx_free_next
11	;0	;3	;2	;tx_add

ditto


Comments from Reviewable

@krzycz

This comment has been minimized.

Show comment
Hide comment
@krzycz

krzycz Oct 2, 2017

Contributor

Reviewed 4 of 31 files at r7.
Review status: all files reviewed at latest revision, 14 unresolved discussions.


Comments from Reviewable

Contributor

krzycz commented Oct 2, 2017

Reviewed 4 of 31 files at r7.
Review status: all files reviewed at latest revision, 14 unresolved discussions.


Comments from Reviewable

@janekmi

This comment has been minimized.

Show comment
Hide comment
@janekmi

janekmi Oct 12, 2017

Contributor

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


src/common/util.h, line 148 at r5 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

util_dec_and_fetch() ?

+1


src/include/libpmemobj/action_base.h, line 55 at r4 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

data => unused?

unused +1


src/include/libpmemobj/action_base.h, line 64 at r4 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…
  1. 60 != 64
  2. Put this formula into the comment in the code.

+2


Comments from Reviewable

Contributor

janekmi commented Oct 12, 2017

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


src/common/util.h, line 148 at r5 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

util_dec_and_fetch() ?

+1


src/include/libpmemobj/action_base.h, line 55 at r4 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

data => unused?

unused +1


src/include/libpmemobj/action_base.h, line 64 at r4 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…
  1. 60 != 64
  2. Put this formula into the comment in the code.

+2


Comments from Reviewable

@krzycz krzycz added the WIP label Oct 30, 2017

@pbalcer

This comment has been minimized.

Show comment
Hide comment
@pbalcer

pbalcer Nov 2, 2017

Contributor

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


src/include/libpmemobj/action.h, line 34 at r4 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

reserve.h ?

It's not just a reservation API. It's more like a redo log thing.


src/include/libpmemobj/action_base.h, line 55 at r4 (raw file):

Previously, janekmi (Jan Michalski) wrote…

unused +1

It's not unused. It might be needed by the internal representation.


src/include/libpmemobj/action_base.h, line 64 at r4 (raw file):

Previously, janekmi (Jan Michalski) wrote…

+2

This is a public header, the formula is an implementation detail. And it's pretty apparent when you look at the equivalent of this #define in tx.c (MAX_TX_ALLOC_RESERVATIONS).

I rounded down the number because of the REDO_NUM_ENTRIES which reserves 16 bytes.


src/libpmemobj/palloc.c, line 71 at r6 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Please use first or third person verbs consistently.

I'm not sure what you mean. All briefs use third person. The larger comments might use a different styles, because they are usually composed so that it's easier to understand, and because they are often written by different people, it might be impossible to enforce a strict rules.


src/libpmemobj/palloc.c, line 446 at r6 (raw file):

Previously, janekmi (Jan Michalski) wrote…

In the meantime means between allocation of the new block and deallocation of the old one? I think it will be better not to use so imprecise expression.

I disagree that it's imprecise.


src/test/obj_action/obj_action.vcxproj, line 84 at r7 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

stdafx.h ?

It's a copy/paste. I don't write those projects by hand.

If you have any issues with the project file, please be explicit. I'm not an expert on VS or our windows build system.


src/test/obj_action/TEST0.PS1, line 44 at r4 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

on Linux it's "short"

Done.


Comments from Reviewable

Contributor

pbalcer commented Nov 2, 2017

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


src/include/libpmemobj/action.h, line 34 at r4 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

reserve.h ?

It's not just a reservation API. It's more like a redo log thing.


src/include/libpmemobj/action_base.h, line 55 at r4 (raw file):

Previously, janekmi (Jan Michalski) wrote…

unused +1

It's not unused. It might be needed by the internal representation.


src/include/libpmemobj/action_base.h, line 64 at r4 (raw file):

Previously, janekmi (Jan Michalski) wrote…

+2

This is a public header, the formula is an implementation detail. And it's pretty apparent when you look at the equivalent of this #define in tx.c (MAX_TX_ALLOC_RESERVATIONS).

I rounded down the number because of the REDO_NUM_ENTRIES which reserves 16 bytes.


src/libpmemobj/palloc.c, line 71 at r6 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Please use first or third person verbs consistently.

I'm not sure what you mean. All briefs use third person. The larger comments might use a different styles, because they are usually composed so that it's easier to understand, and because they are often written by different people, it might be impossible to enforce a strict rules.


src/libpmemobj/palloc.c, line 446 at r6 (raw file):

Previously, janekmi (Jan Michalski) wrote…

In the meantime means between allocation of the new block and deallocation of the old one? I think it will be better not to use so imprecise expression.

I disagree that it's imprecise.


src/test/obj_action/obj_action.vcxproj, line 84 at r7 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

stdafx.h ?

It's a copy/paste. I don't write those projects by hand.

If you have any issues with the project file, please be explicit. I'm not an expert on VS or our windows build system.


src/test/obj_action/TEST0.PS1, line 44 at r4 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

on Linux it's "short"

Done.


Comments from Reviewable

@krzycz

This comment has been minimized.

Show comment
Hide comment
@krzycz

krzycz Nov 2, 2017

Contributor

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


src/include/libpmemobj/action_base.h, line 55 at r4 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

It's not unused. It might be needed by the internal representation.

Another question,
Does the user really know the definition of this structure, including the action type enum?
As I can see, the structure is initialized by the library, so the user only needs to know its size. Am I right?
IOW, it could be defined in a similar fashion as our pmem locks (uint64_t data[16]) and inside the library, we can cast it to the internal definition.


Comments from Reviewable

Contributor

krzycz commented Nov 2, 2017

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


src/include/libpmemobj/action_base.h, line 55 at r4 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

It's not unused. It might be needed by the internal representation.

Another question,
Does the user really know the definition of this structure, including the action type enum?
As I can see, the structure is initialized by the library, so the user only needs to know its size. Am I right?
IOW, it could be defined in a similar fashion as our pmem locks (uint64_t data[16]) and inside the library, we can cast it to the internal definition.


Comments from Reviewable

@pbalcer

This comment has been minimized.

Show comment
Hide comment
@pbalcer

pbalcer Nov 3, 2017

Contributor

Review status: 24 of 52 files reviewed at latest revision, 11 unresolved discussions.


src/include/libpmemobj/action_base.h, line 55 at r4 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

Another question,
Does the user really know the definition of this structure, including the action type enum?
As I can see, the structure is initialized by the library, so the user only needs to know its size. Am I right?
IOW, it could be defined in a similar fashion as our pmem locks (uint64_t data[16]) and inside the library, we can cast it to the internal definition.

I want the user to be able to see those fields - this might be useful in code that moves the actions around between threads.


Comments from Reviewable

Contributor

pbalcer commented Nov 3, 2017

Review status: 24 of 52 files reviewed at latest revision, 11 unresolved discussions.


src/include/libpmemobj/action_base.h, line 55 at r4 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

Another question,
Does the user really know the definition of this structure, including the action type enum?
As I can see, the structure is initialized by the library, so the user only needs to know its size. Am I right?
IOW, it could be defined in a similar fashion as our pmem locks (uint64_t data[16]) and inside the library, we can cast it to the internal definition.

I want the user to be able to see those fields - this might be useful in code that moves the actions around between threads.


Comments from Reviewable

@pbalcer

This comment has been minimized.

Show comment
Hide comment
@pbalcer

pbalcer Nov 3, 2017

Contributor

I've fixed all issues that appeared after the rebase and added the docs.

Contributor

pbalcer commented Nov 3, 2017

I've fixed all issues that appeared after the rebase and added the docs.

@krzycz

This comment has been minimized.

Show comment
Hide comment
@krzycz

krzycz Nov 7, 2017

Contributor

Reviewed 15 of 29 files at r8.
Review status: 38 of 52 files reviewed at latest revision, 11 unresolved discussions.


Comments from Reviewable

Contributor

krzycz commented Nov 7, 2017

Reviewed 15 of 29 files at r8.
Review status: 38 of 52 files reviewed at latest revision, 11 unresolved discussions.


Comments from Reviewable

@krzycz

This comment has been minimized.

Show comment
Hide comment
@krzycz

krzycz Nov 15, 2017

Contributor

To be honest, it does not look obvious for me on how and when to use these new APIs. There is a man page and some tests, but I would also expect an example or two that illustrate the actual use case for this new feature.

IOW, the potential benefits of using reserve/publish approach are described somewhere in the related PR or GitHub issue, but not in the documentation/man page, so it might be not clear for the users what's the difference between using the regular transactions vs. the new APIs.


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


doc/libpmemobj.3.md, line 391 at r8 (raw file):

void pmemobj_cancel(PMEMobjpool *pop, struct pobj_action *actv, int actvcnt);

#define POBJ_RESERVE(pop, t, act)\

What about POBJ_RESERVE_NEW? It would be consistent with POBJ_ALLOC/POBJ_NEW.


src/libpmemobj/heap.c, line 630 at r8 (raw file):

	if (recycler_put(heap->rt->recyclers[c->id], m, score) < 0)
		ERR("lost runtime tracking info of %u run due to OOM", c->id);

And what happens to this memory block in such case?


Comments from Reviewable

Contributor

krzycz commented Nov 15, 2017

To be honest, it does not look obvious for me on how and when to use these new APIs. There is a man page and some tests, but I would also expect an example or two that illustrate the actual use case for this new feature.

IOW, the potential benefits of using reserve/publish approach are described somewhere in the related PR or GitHub issue, but not in the documentation/man page, so it might be not clear for the users what's the difference between using the regular transactions vs. the new APIs.


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


doc/libpmemobj.3.md, line 391 at r8 (raw file):

void pmemobj_cancel(PMEMobjpool *pop, struct pobj_action *actv, int actvcnt);

#define POBJ_RESERVE(pop, t, act)\

What about POBJ_RESERVE_NEW? It would be consistent with POBJ_ALLOC/POBJ_NEW.


src/libpmemobj/heap.c, line 630 at r8 (raw file):

	if (recycler_put(heap->rt->recyclers[c->id], m, score) < 0)
		ERR("lost runtime tracking info of %u run due to OOM", c->id);

And what happens to this memory block in such case?


Comments from Reviewable

@pbalcer

This comment has been minimized.

Show comment
Hide comment
@pbalcer

pbalcer Nov 16, 2017

Contributor

I have an example container that uses this API to improve performance. I'll fix it up and publish it once this patch gets merged.

In general, this API should be used to implement asynchronous semantics. One obvious scenarios involves a database INSERT INTO abc VALUES (...); where you might want to offload the cost of persistence to a different thread.


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


doc/libpmemobj.3.md, line 391 at r8 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

What about POBJ_RESERVE_NEW? It would be consistent with POBJ_ALLOC/POBJ_NEW.

+1. I'll change it.


src/libpmemobj/heap.c, line 630 at r8 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

And what happens to this memory block in such case?

It will be unavailable until the next time the program reaches complete persistent OOM or a restart. The alternative is FATAL.


Comments from Reviewable

Contributor

pbalcer commented Nov 16, 2017

I have an example container that uses this API to improve performance. I'll fix it up and publish it once this patch gets merged.

In general, this API should be used to implement asynchronous semantics. One obvious scenarios involves a database INSERT INTO abc VALUES (...); where you might want to offload the cost of persistence to a different thread.


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


doc/libpmemobj.3.md, line 391 at r8 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

What about POBJ_RESERVE_NEW? It would be consistent with POBJ_ALLOC/POBJ_NEW.

+1. I'll change it.


src/libpmemobj/heap.c, line 630 at r8 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

And what happens to this memory block in such case?

It will be unavailable until the next time the program reaches complete persistent OOM or a restart. The alternative is FATAL.


Comments from Reviewable

@krzycz

This comment has been minimized.

Show comment
Hide comment
@krzycz

krzycz Nov 30, 2017

Contributor

Reviewed 9 of 11 files at r9.
Review status: 51 of 52 files reviewed at latest revision, 17 unresolved discussions.


doc/libpmemobj/pmemobj_action.3.md, line 85 at r9 (raw file):

The publication is fail-safe atomic in the scope of the entire collection of
actions, but the number of said actions is limited by **POBJ_MAX_ACTIONS**
variable. If a program exists without publishing the actions, or the actions are

variable => macro/constant ?


doc/libpmemobj/pmemobj_action.3.md, line 94 at r9 (raw file):

created, and published, from different threads.

```c

Function signatures are not needed in description.


doc/libpmemobj/pmemobj_action.3.md, line 98 at r9 (raw file):

The pmemobj_reserve() functions performs a transient reservation of an object.

It is not explained what's the purpose of act argument.
It is mentioned somewhere above, but IMHO it should be pointed here, that the caller must allocate memory for this struct somewhere and pass a pointer to it. The struct will be populated by the pmemobj_reserve and should not be modified (could it be?).


doc/libpmemobj/pmemobj_action.3.md, line 99 at r9 (raw file):

The **pmemobj_reserve**() functions performs a transient reservation of an object.
Behaves similarly to **pmemobj_alloc**(), but performs no modification to the

pmemobj_alloc(3)


doc/libpmemobj/pmemobj_action.3.md, line 111 at r9 (raw file):

The **pmemobj_set_value** function prepares an action that, once published, will
modify the memory location pointed to by **ptr** to **value**.
  1. arg names in italic, not bold
  2. as above - no mention about act

doc/libpmemobj/pmemobj_action.3.md, line 117 at r9 (raw file):

The pmemobj_publish function publishes the provided set of actions. The

I think it should return a status value. Could it fail for some reason?


doc/libpmemobj/pmemobj_action.3.md, line 161 at r9 (raw file):

On success, **pmemobj_tx_publish**() returns 0, otherwise,
stage changes to **TX_STAGE_ONABORT** and *errno* is set appropriately

What about a small code snippet (EXAMPLE) illustrating how to use it:

...
struct foo {
  int x;
  PMEMoid next;
};
struct pobj_action actv[4];
...
PMEMoid o1 = pmemobj_reserve(pop, &actv[0], sizeof(struct foo), 0);
D_RW(o1)->x = 5;
D_RW(o1)->next = OID_NULL;
PMEMoid o2 = pmemobj_reserve(pop, &actv[1], sizeof(struct foo), 0);
D_RW(o2)->x = 55;
D_RW(o2)->next = o1;
pmemobj_set_value(pop, &actv[2], &D_RO(root)->head.pool_uuid_lo, o2.pool_uuid_lo);
pmemobj_set_value(pop, &actv[3], &D_RO(root)->head.off, o2.off);
pmemobj_publish(pop, actv, 4);

BTW, is this code correct? Does it make sense?


Comments from Reviewable

Contributor

krzycz commented Nov 30, 2017

Reviewed 9 of 11 files at r9.
Review status: 51 of 52 files reviewed at latest revision, 17 unresolved discussions.


doc/libpmemobj/pmemobj_action.3.md, line 85 at r9 (raw file):

The publication is fail-safe atomic in the scope of the entire collection of
actions, but the number of said actions is limited by **POBJ_MAX_ACTIONS**
variable. If a program exists without publishing the actions, or the actions are

variable => macro/constant ?


doc/libpmemobj/pmemobj_action.3.md, line 94 at r9 (raw file):

created, and published, from different threads.

```c

Function signatures are not needed in description.


doc/libpmemobj/pmemobj_action.3.md, line 98 at r9 (raw file):

The pmemobj_reserve() functions performs a transient reservation of an object.

It is not explained what's the purpose of act argument.
It is mentioned somewhere above, but IMHO it should be pointed here, that the caller must allocate memory for this struct somewhere and pass a pointer to it. The struct will be populated by the pmemobj_reserve and should not be modified (could it be?).


doc/libpmemobj/pmemobj_action.3.md, line 99 at r9 (raw file):

The **pmemobj_reserve**() functions performs a transient reservation of an object.
Behaves similarly to **pmemobj_alloc**(), but performs no modification to the

pmemobj_alloc(3)


doc/libpmemobj/pmemobj_action.3.md, line 111 at r9 (raw file):

The **pmemobj_set_value** function prepares an action that, once published, will
modify the memory location pointed to by **ptr** to **value**.
  1. arg names in italic, not bold
  2. as above - no mention about act

doc/libpmemobj/pmemobj_action.3.md, line 117 at r9 (raw file):

The pmemobj_publish function publishes the provided set of actions. The

I think it should return a status value. Could it fail for some reason?


doc/libpmemobj/pmemobj_action.3.md, line 161 at r9 (raw file):

On success, **pmemobj_tx_publish**() returns 0, otherwise,
stage changes to **TX_STAGE_ONABORT** and *errno* is set appropriately

What about a small code snippet (EXAMPLE) illustrating how to use it:

...
struct foo {
  int x;
  PMEMoid next;
};
struct pobj_action actv[4];
...
PMEMoid o1 = pmemobj_reserve(pop, &actv[0], sizeof(struct foo), 0);
D_RW(o1)->x = 5;
D_RW(o1)->next = OID_NULL;
PMEMoid o2 = pmemobj_reserve(pop, &actv[1], sizeof(struct foo), 0);
D_RW(o2)->x = 55;
D_RW(o2)->next = o1;
pmemobj_set_value(pop, &actv[2], &D_RO(root)->head.pool_uuid_lo, o2.pool_uuid_lo);
pmemobj_set_value(pop, &actv[3], &D_RO(root)->head.off, o2.off);
pmemobj_publish(pop, actv, 4);

BTW, is this code correct? Does it make sense?


Comments from Reviewable

@krzycz

This comment has been minimized.

Show comment
Hide comment
@krzycz

krzycz Nov 30, 2017

Contributor

Reviewed 1 of 11 files at r9.
Review status: all files reviewed at latest revision, 17 unresolved discussions.


Comments from Reviewable

Contributor

krzycz commented Nov 30, 2017

Reviewed 1 of 11 files at r9.
Review status: all files reviewed at latest revision, 17 unresolved discussions.


Comments from Reviewable

@pbalcer

This comment has been minimized.

Show comment
Hide comment
@pbalcer

pbalcer Dec 1, 2017

Contributor

Review status: all files reviewed at latest revision, 17 unresolved discussions.


doc/libpmemobj/pmemobj_action.3.md, line 85 at r9 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

variable => macro/constant ?

Done.


doc/libpmemobj/pmemobj_action.3.md, line 94 at r9 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

Function signatures are not needed in description.

Done.


doc/libpmemobj/pmemobj_action.3.md, line 98 at r9 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

It is not explained what's the purpose of act argument.
It is mentioned somewhere above, but IMHO it should be pointed here, that the caller must allocate memory for this struct somewhere and pass a pointer to it. The struct will be populated by the pmemobj_reserve and should not be modified (could it be?).

I intentionally moved the definition of the pobj_action structure above the functions because I wanted to avoid repeating the same thing several times.

I've expanded the description.


doc/libpmemobj/pmemobj_action.3.md, line 99 at r9 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

pmemobj_alloc(3)

Done.


doc/libpmemobj/pmemobj_action.3.md, line 111 at r9 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…
  1. arg names in italic, not bold
  2. as above - no mention about act

Done.


doc/libpmemobj/pmemobj_action.3.md, line 117 at r9 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

I think it should return a status value. Could it fail for some reason?

Nope. All the resources have been allocated by this point.


doc/libpmemobj/pmemobj_action.3.md, line 161 at r9 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

What about a small code snippet (EXAMPLE) illustrating how to use it:

...
struct foo {
  int x;
  PMEMoid next;
};
struct pobj_action actv[4];
...
PMEMoid o1 = pmemobj_reserve(pop, &actv[0], sizeof(struct foo), 0);
D_RW(o1)->x = 5;
D_RW(o1)->next = OID_NULL;
PMEMoid o2 = pmemobj_reserve(pop, &actv[1], sizeof(struct foo), 0);
D_RW(o2)->x = 55;
D_RW(o2)->next = o1;
pmemobj_set_value(pop, &actv[2], &D_RO(root)->head.pool_uuid_lo, o2.pool_uuid_lo);
pmemobj_set_value(pop, &actv[3], &D_RO(root)->head.off, o2.off);
pmemobj_publish(pop, actv, 4);

BTW, is this code correct? Does it make sense?

I'll add the example.

And yes, it does make sense, that's exactly the intention behind set_value.
But you forgot to flush :)


Comments from Reviewable

Contributor

pbalcer commented Dec 1, 2017

Review status: all files reviewed at latest revision, 17 unresolved discussions.


doc/libpmemobj/pmemobj_action.3.md, line 85 at r9 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

variable => macro/constant ?

Done.


doc/libpmemobj/pmemobj_action.3.md, line 94 at r9 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

Function signatures are not needed in description.

Done.


doc/libpmemobj/pmemobj_action.3.md, line 98 at r9 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

It is not explained what's the purpose of act argument.
It is mentioned somewhere above, but IMHO it should be pointed here, that the caller must allocate memory for this struct somewhere and pass a pointer to it. The struct will be populated by the pmemobj_reserve and should not be modified (could it be?).

I intentionally moved the definition of the pobj_action structure above the functions because I wanted to avoid repeating the same thing several times.

I've expanded the description.


doc/libpmemobj/pmemobj_action.3.md, line 99 at r9 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

pmemobj_alloc(3)

Done.


doc/libpmemobj/pmemobj_action.3.md, line 111 at r9 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…
  1. arg names in italic, not bold
  2. as above - no mention about act

Done.


doc/libpmemobj/pmemobj_action.3.md, line 117 at r9 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

I think it should return a status value. Could it fail for some reason?

Nope. All the resources have been allocated by this point.


doc/libpmemobj/pmemobj_action.3.md, line 161 at r9 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

What about a small code snippet (EXAMPLE) illustrating how to use it:

...
struct foo {
  int x;
  PMEMoid next;
};
struct pobj_action actv[4];
...
PMEMoid o1 = pmemobj_reserve(pop, &actv[0], sizeof(struct foo), 0);
D_RW(o1)->x = 5;
D_RW(o1)->next = OID_NULL;
PMEMoid o2 = pmemobj_reserve(pop, &actv[1], sizeof(struct foo), 0);
D_RW(o2)->x = 55;
D_RW(o2)->next = o1;
pmemobj_set_value(pop, &actv[2], &D_RO(root)->head.pool_uuid_lo, o2.pool_uuid_lo);
pmemobj_set_value(pop, &actv[3], &D_RO(root)->head.off, o2.off);
pmemobj_publish(pop, actv, 4);

BTW, is this code correct? Does it make sense?

I'll add the example.

And yes, it does make sense, that's exactly the intention behind set_value.
But you forgot to flush :)


Comments from Reviewable

@krzycz

This comment has been minimized.

Show comment
Hide comment
@krzycz

krzycz Dec 1, 2017

Contributor

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


doc/libpmemobj/pmemobj_action.3.md, line 161 at r9 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

I'll add the example.

And yes, it does make sense, that's exactly the intention behind set_value.
But you forgot to flush :)

Ha! So, that's why the example is useful.
OTH, the question is - what about (optional) flushing in pmemobj_publish?
Also, I would emphasize it in the description of *publish functions that those functions DO NOT flush the object content. It may be confusing especially in the case of pmemobj_tx_publish.


Comments from Reviewable

Contributor

krzycz commented Dec 1, 2017

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


doc/libpmemobj/pmemobj_action.3.md, line 161 at r9 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

I'll add the example.

And yes, it does make sense, that's exactly the intention behind set_value.
But you forgot to flush :)

Ha! So, that's why the example is useful.
OTH, the question is - what about (optional) flushing in pmemobj_publish?
Also, I would emphasize it in the description of *publish functions that those functions DO NOT flush the object content. It may be confusing especially in the case of pmemobj_tx_publish.


Comments from Reviewable

@krzycz krzycz removed the WIP label Dec 1, 2017

@krzycz

This comment has been minimized.

Show comment
Hide comment
@krzycz

krzycz Dec 1, 2017

Contributor

Reviewed 2 of 3 files at r11.
Review status: 53 of 54 files reviewed at latest revision, 12 unresolved discussions, some commit checks broke.


src/libpmemobj/tx.c, line 2181 at r11 (raw file):

			ERR("only heap actions can be "
			"published with a transaction");
			ASSERT(0);

return obj_tx_abort_err(EINVAL); or break; ?


Comments from Reviewable

Contributor

krzycz commented Dec 1, 2017

Reviewed 2 of 3 files at r11.
Review status: 53 of 54 files reviewed at latest revision, 12 unresolved discussions, some commit checks broke.


src/libpmemobj/tx.c, line 2181 at r11 (raw file):

			ERR("only heap actions can be "
			"published with a transaction");
			ASSERT(0);

return obj_tx_abort_err(EINVAL); or break; ?


Comments from Reviewable

@krzycz

This comment has been minimized.

Show comment
Hide comment
@krzycz

krzycz Dec 1, 2017

Contributor

Reviewed 1 of 3 files at r11.
Review status: all files reviewed at latest revision, 12 unresolved discussions, some commit checks broke.


Comments from Reviewable

Contributor

krzycz commented Dec 1, 2017

Reviewed 1 of 3 files at r11.
Review status: all files reviewed at latest revision, 12 unresolved discussions, some commit checks broke.


Comments from Reviewable

@pbalcer

This comment has been minimized.

Show comment
Hide comment
@pbalcer

pbalcer Dec 1, 2017

Contributor

Review status: 49 of 54 files reviewed at latest revision, 12 unresolved discussions.


src/libpmemobj/tx.c, line 2181 at r11 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

return obj_tx_abort_err(EINVAL); or break; ?

Done.


Comments from Reviewable

Contributor

pbalcer commented Dec 1, 2017

Review status: 49 of 54 files reviewed at latest revision, 12 unresolved discussions.


src/libpmemobj/tx.c, line 2181 at r11 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

return obj_tx_abort_err(EINVAL); or break; ?

Done.


Comments from Reviewable

@krzycz

This comment has been minimized.

Show comment
Hide comment
@krzycz

krzycz Dec 1, 2017

Contributor

Reviewed 5 of 5 files at r12.
Review status: all files reviewed at latest revision, 10 unresolved discussions.


Comments from Reviewable

Contributor

krzycz commented Dec 1, 2017

Reviewed 5 of 5 files at r12.
Review status: all files reviewed at latest revision, 10 unresolved discussions.


Comments from Reviewable

@krzycz krzycz merged commit 8ee34d9 into pmem:master Dec 2, 2017

1 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
code-review/reviewable 10 discussions left (janekmi, krzycz, pbalcer, plebioda)
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@mslusarz

This comment has been minimized.

Show comment
Hide comment
@mslusarz

mslusarz Dec 2, 2017

Contributor

🆒

Contributor

mslusarz commented Dec 2, 2017

🆒

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment