New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

allocation class interface + xalloc #1985

Merged
merged 2 commits into from Jul 25, 2017

Conversation

Projects
None yet
4 participants
@pbalcer
Contributor

pbalcer commented Jun 7, 2017

First part of the allocation classes interface.


This change is Reviewable

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Jun 8, 2017

Codecov Report

Merging #1985 into master will increase coverage by 0.02%.
The diff coverage is 82.62%.

@@            Coverage Diff             @@
##           master    #1985      +/-   ##
==========================================
+ Coverage   83.05%   83.08%   +0.02%     
==========================================
  Files         330      332       +2     
  Lines       36037    36247     +210     
  Branches     5376     5397      +21     
==========================================
+ Hits        29932    30116     +184     
- Misses       6105     6131      +26

codecov-io commented Jun 8, 2017

Codecov Report

Merging #1985 into master will increase coverage by 0.02%.
The diff coverage is 82.62%.

@@            Coverage Diff             @@
##           master    #1985      +/-   ##
==========================================
+ Coverage   83.05%   83.08%   +0.02%     
==========================================
  Files         330      332       +2     
  Lines       36037    36247     +210     
  Branches     5376     5397      +21     
==========================================
+ Hits        29932    30116     +184     
- Misses       6105     6131      +26
#define POBJ_FLAG_ZERO (((uint64_t)1) << 0)
#define POBJ_FLAG_NO_FLUSH (((uint64_t)1) << 1)
#define POBJ_CLASS_ID(id) (((uint64_t)(id)) << 48)

This comment has been minimized.

@marcinslusarz

marcinslusarz Jun 8, 2017

Member

56? Class ids are 8 bit only.

@marcinslusarz

marcinslusarz Jun 8, 2017

Member

56? Class ids are 8 bit only.

This comment has been minimized.

@pbalcer

pbalcer Jun 9, 2017

Contributor

Please read previous reviews...
I've reserved 16 bits for future use.

@pbalcer

pbalcer Jun 9, 2017

Contributor

Please read previous reviews...
I've reserved 16 bits for future use.

Show outdated Hide outdated src/include/libpmemobj/ctl.h
Show outdated Hide outdated src/include/libpmemobj/ctl.h
* This can be used to create very small allocation classes, but it
* does not support type numbers.
* Additionally, allocations with this header can only span a single
* unit.

This comment has been minimized.

@marcinslusarz

marcinslusarz Jun 8, 2017

Member

I understand why it's here, but maybe we could do something about this limitation (not necessarily in this PR)? Like let user allocate more than 1 unit (with a flag), document that alloc_usable_size returns unit_size for such allocations and user is obliged to free all units one by one.

@marcinslusarz

marcinslusarz Jun 8, 2017

Member

I understand why it's here, but maybe we could do something about this limitation (not necessarily in this PR)? Like let user allocate more than 1 unit (with a flag), document that alloc_usable_size returns unit_size for such allocations and user is obliged to free all units one by one.

This comment has been minimized.

@pbalcer

pbalcer Jun 9, 2017

Contributor

It's already technically speaking possible. But it goes way beyond normal use and it breaks the API... You are better off just allocating N single-unit objects in a transaction (after we merge the aggregated tx ops patch, it will perform similarly).

@pbalcer

pbalcer Jun 9, 2017

Contributor

It's already technically speaking possible. But it goes way beyond normal use and it breaks the API... You are better off just allocating N single-unit objects in a transaction (after we merge the aggregated tx ops patch, it will perform similarly).

Show outdated Hide outdated src/include/libpmemobj/ctl.h
@marcinslusarz

This comment has been minimized.

Show comment
Hide comment
@marcinslusarz

marcinslusarz Jun 9, 2017

Member

Reviewed 3 of 42 files at r1.
Review status: 3 of 42 files reviewed at latest revision, 6 unresolved discussions.


doc/libpmemobj.3.md, line 2417 at r2 (raw file):

heap.
Keep in mind that the information whether an object is allocated or not is
stored in a bitmap with **2432** entries, this limitation makes it inefficient

Do we really want to tell users about this specific value?


src/include/libpmemobj/ctl.h, line 57 at r2 (raw file):

 * class is selected in a fashion that minimizes contention between threads.
 * Depending on the requested size and the configuration of the size<>class
 * mapping, it might happen that the object size (including required metadata)

no mapping in this PR


src/include/libpmemobj/ctl.h, line 75 at r2 (raw file):

 * property of the allocator - they are NOT stored persistently in the pool.
 * It's recommended to always create custom allocation classes and the
 * corresponding mapping immediately after creating and opening the pool,

no mapping in this PR


src/include/libpmemobj/ctl.h, line 77 at r2 (raw file):

 * corresponding mapping immediately after creating and opening the pool,
 * before any use.
 * If there are existing objects created using an class that is no longer stored

a class


Comments from Reviewable

Member

marcinslusarz commented Jun 9, 2017

Reviewed 3 of 42 files at r1.
Review status: 3 of 42 files reviewed at latest revision, 6 unresolved discussions.


doc/libpmemobj.3.md, line 2417 at r2 (raw file):

heap.
Keep in mind that the information whether an object is allocated or not is
stored in a bitmap with **2432** entries, this limitation makes it inefficient

Do we really want to tell users about this specific value?


src/include/libpmemobj/ctl.h, line 57 at r2 (raw file):

 * class is selected in a fashion that minimizes contention between threads.
 * Depending on the requested size and the configuration of the size<>class
 * mapping, it might happen that the object size (including required metadata)

no mapping in this PR


src/include/libpmemobj/ctl.h, line 75 at r2 (raw file):

 * property of the allocator - they are NOT stored persistently in the pool.
 * It's recommended to always create custom allocation classes and the
 * corresponding mapping immediately after creating and opening the pool,

no mapping in this PR


src/include/libpmemobj/ctl.h, line 77 at r2 (raw file):

 * corresponding mapping immediately after creating and opening the pool,
 * before any use.
 * If there are existing objects created using an class that is no longer stored

a class


Comments from Reviewable

@pbalcer

This comment has been minimized.

Show comment
Hide comment
@pbalcer

pbalcer Jul 21, 2017

Contributor

Review status: 2 of 42 files reviewed at latest revision, 6 unresolved discussions, some commit checks broke.


src/include/libpmemobj/ctl.h, line 53 at r1 (raw file):

Previously, marcinslusarz (Marcin Ślusarz) wrote…

This whole comment assumes map interface is merged while it's in a separate PR.

Done.


src/include/libpmemobj/ctl.h, line 79 at r1 (raw file):

Previously, marcinslusarz (Marcin Ślusarz) wrote…

Not in this PR

Done.


src/include/libpmemobj/ctl.h, line 57 at r2 (raw file):

Previously, marcinslusarz (Marcin Ślusarz) wrote…

no mapping in this PR

Done.


src/include/libpmemobj/ctl.h, line 75 at r2 (raw file):

Previously, marcinslusarz (Marcin Ślusarz) wrote…

no mapping in this PR

Done.


src/include/libpmemobj/ctl.h, line 77 at r2 (raw file):

Previously, marcinslusarz (Marcin Ślusarz) wrote…

a class

Done.


Comments from Reviewable

Contributor

pbalcer commented Jul 21, 2017

Review status: 2 of 42 files reviewed at latest revision, 6 unresolved discussions, some commit checks broke.


src/include/libpmemobj/ctl.h, line 53 at r1 (raw file):

Previously, marcinslusarz (Marcin Ślusarz) wrote…

This whole comment assumes map interface is merged while it's in a separate PR.

Done.


src/include/libpmemobj/ctl.h, line 79 at r1 (raw file):

Previously, marcinslusarz (Marcin Ślusarz) wrote…

Not in this PR

Done.


src/include/libpmemobj/ctl.h, line 57 at r2 (raw file):

Previously, marcinslusarz (Marcin Ślusarz) wrote…

no mapping in this PR

Done.


src/include/libpmemobj/ctl.h, line 75 at r2 (raw file):

Previously, marcinslusarz (Marcin Ślusarz) wrote…

no mapping in this PR

Done.


src/include/libpmemobj/ctl.h, line 77 at r2 (raw file):

Previously, marcinslusarz (Marcin Ślusarz) wrote…

a class

Done.


Comments from Reviewable

@krzycz

This comment has been minimized.

Show comment
Hide comment
@krzycz

krzycz Jul 24, 2017

Contributor

Reviewed 35 of 42 files at r1, 11 of 11 files at r3.
Review status: all files reviewed at latest revision, 18 unresolved discussions, some commit checks failed.


doc/libpmemobj.3.md, line 2417 at r2 (raw file):

Previously, marcinslusarz (Marcin Ślusarz) wrote…

Do we really want to tell users about this specific value?

Agree. IMHO, there is no need to provide such details here.


doc/libpmemobj.3.md, line 2512 at r3 (raw file):

struct pobj_alloc_class_desc {

Wouldn't it be better to use sized types? uint64_t, etc.


src/examples/libpmemobj/slab_allocator/main.c, line 48 at r3 (raw file):

#include "slab_allocator.h"

POBJ_LAYOUT_BEGIN(slab_allocator);

A crazy idea - if user defines their object types using POBJ_LAYOUT macros, we could probably define alloc classes for those types under the hood (optionally).


src/include/libpmemobj/ctl.h, line 118 at r3 (raw file):

/*
 * Archetype of allocation classes

archetype?


src/libpmemobj/alloc_class.h, line 118 at r3 (raw file):

struct alloc_class *
alloc_class_register(struct alloc_class_collection *ac,
	struct alloc_class *aclass);

aclass => c (as for other functions)


src/libpmemobj/heap_layout.h, line 69 at r3 (raw file):

#define CHUNKSIZE_MASK ((CHUNKSIZE) - 1)
#define CHUNKSIZE_ALIGN(value) ((((value) + CHUNKSIZE_MASK) & ~CHUNKSIZE_MASK))

what about CHUNKSIZE_ALIGN_UP()? / ROUND_UP


src/libpmemobj/palloc.c, line 176 at r2 (raw file):

	if (size != 0) {
		/*
		 * class_id is 16 byte, but for now we only support up to 255

16-bit


src/libpmemobj/palloc.c, line 179 at r2 (raw file):

		 * classes - might change in the future.
		 */
		ASSERT(class_id < UINT8_MAX);

not "<=" ?
I mean, is 255 a valid class id?


src/libpmemobj/pmalloc.c, line 354 at r3 (raw file):

			break;
		default:
			ASSERT(0); /* unreachable */

It must be handled - assert is not enough.
And no, C compiler would NOT warn you about invalid header type assignment. I.e. p->header_type = 20 is perfectly fine in C. (could be an error in C++)
BTW, MAX_HEADER_TYPES is a member of the enumeration, but it's not a valid header type value.


src/test/obj_ctl_alloc_class/obj_ctl_alloc_class.c, line 48 at r3 (raw file):

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

no need to break the line


src/test/obj_ctl_alloc_class/obj_ctl_alloc_class.c, line 159 at r3 (raw file):

	ret = pmemobj_ctl_set(pop, "heap.alloc_class.new.desc",
		&alloc_class_new);
	UT_ASSERTeq(ret, 0);

UT_ASSERTeq(alloc_class_new.class_id, 130); ?


src/test/obj_ctl_alloc_class/TEST0, line 1 at r3 (raw file):

#!/bin/bash -e

shebang


src/test/obj_ctl_alloc_class_config/TEST0, line 1 at r3 (raw file):

#!/bin/bash -e

ditto


Comments from Reviewable

Contributor

krzycz commented Jul 24, 2017

Reviewed 35 of 42 files at r1, 11 of 11 files at r3.
Review status: all files reviewed at latest revision, 18 unresolved discussions, some commit checks failed.


doc/libpmemobj.3.md, line 2417 at r2 (raw file):

Previously, marcinslusarz (Marcin Ślusarz) wrote…

Do we really want to tell users about this specific value?

Agree. IMHO, there is no need to provide such details here.


doc/libpmemobj.3.md, line 2512 at r3 (raw file):

struct pobj_alloc_class_desc {

Wouldn't it be better to use sized types? uint64_t, etc.


src/examples/libpmemobj/slab_allocator/main.c, line 48 at r3 (raw file):

#include "slab_allocator.h"

POBJ_LAYOUT_BEGIN(slab_allocator);

A crazy idea - if user defines their object types using POBJ_LAYOUT macros, we could probably define alloc classes for those types under the hood (optionally).


src/include/libpmemobj/ctl.h, line 118 at r3 (raw file):

/*
 * Archetype of allocation classes

archetype?


src/libpmemobj/alloc_class.h, line 118 at r3 (raw file):

struct alloc_class *
alloc_class_register(struct alloc_class_collection *ac,
	struct alloc_class *aclass);

aclass => c (as for other functions)


src/libpmemobj/heap_layout.h, line 69 at r3 (raw file):

#define CHUNKSIZE_MASK ((CHUNKSIZE) - 1)
#define CHUNKSIZE_ALIGN(value) ((((value) + CHUNKSIZE_MASK) & ~CHUNKSIZE_MASK))

what about CHUNKSIZE_ALIGN_UP()? / ROUND_UP


src/libpmemobj/palloc.c, line 176 at r2 (raw file):

	if (size != 0) {
		/*
		 * class_id is 16 byte, but for now we only support up to 255

16-bit


src/libpmemobj/palloc.c, line 179 at r2 (raw file):

		 * classes - might change in the future.
		 */
		ASSERT(class_id < UINT8_MAX);

not "<=" ?
I mean, is 255 a valid class id?


src/libpmemobj/pmalloc.c, line 354 at r3 (raw file):

			break;
		default:
			ASSERT(0); /* unreachable */

It must be handled - assert is not enough.
And no, C compiler would NOT warn you about invalid header type assignment. I.e. p->header_type = 20 is perfectly fine in C. (could be an error in C++)
BTW, MAX_HEADER_TYPES is a member of the enumeration, but it's not a valid header type value.


src/test/obj_ctl_alloc_class/obj_ctl_alloc_class.c, line 48 at r3 (raw file):

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

no need to break the line


src/test/obj_ctl_alloc_class/obj_ctl_alloc_class.c, line 159 at r3 (raw file):

	ret = pmemobj_ctl_set(pop, "heap.alloc_class.new.desc",
		&alloc_class_new);
	UT_ASSERTeq(ret, 0);

UT_ASSERTeq(alloc_class_new.class_id, 130); ?


src/test/obj_ctl_alloc_class/TEST0, line 1 at r3 (raw file):

#!/bin/bash -e

shebang


src/test/obj_ctl_alloc_class_config/TEST0, line 1 at r3 (raw file):

#!/bin/bash -e

ditto


Comments from Reviewable

@pbalcer

This comment has been minimized.

Show comment
Hide comment
@pbalcer

pbalcer Jul 25, 2017

Contributor

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


doc/libpmemobj.3.md, line 2417 at r2 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

Agree. IMHO, there is no need to provide such details here.

Ok, but then how do I make this limitation clear?


doc/libpmemobj.3.md, line 2512 at r3 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

Wouldn't it be better to use sized types? uint64_t, etc.

This is not a persistent structure, think of it as API function specification - we don't exclusively use fixed width types there either.


src/examples/libpmemobj/slab_allocator/main.c, line 48 at r3 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

A crazy idea - if user defines their object types using POBJ_LAYOUT macros, we could probably define alloc classes for those types under the hood (optionally).

How? It would still require you to repeat all the types in a function body. We can provide a helper macro, but I think an example like this one is enough.


src/include/libpmemobj/ctl.h, line 118 at r3 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

archetype?

Or "prototype". This struct was originally called pobj_alloc_class_proto.
"original pattern from which copies are made"


src/libpmemobj/alloc_class.h, line 118 at r3 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

aclass => c (as for other functions)

Done.


src/libpmemobj/heap_layout.h, line 69 at r3 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

what about CHUNKSIZE_ALIGN_UP()? / ROUND_UP

Done.


src/libpmemobj/palloc.c, line 176 at r2 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

16-bit

Done.


src/libpmemobj/palloc.c, line 179 at r2 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

not "<=" ?
I mean, is 255 a valid class id?

It's not. I fixed the manpage.


src/libpmemobj/pmalloc.c, line 354 at r3 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

It must be handled - assert is not enough.
And no, C compiler would NOT warn you about invalid header type assignment. I.e. p->header_type = 20 is perfectly fine in C. (could be an error in C++)
BTW, MAX_HEADER_TYPES is a member of the enumeration, but it's not a valid header type value.

enum foo {BAR};
int main() {
enum foo f = 5;
return 0;
}

clang main.c -Wassign-enum

main.c:3:22: warning: integer constant not in range of enumerated type 'enum foo' [-Wassign-enum]
enum foo f = 5;
^
1 warning generated.

But done.


src/test/obj_ctl_alloc_class/obj_ctl_alloc_class.c, line 48 at r3 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

no need to break the line

Done.


src/test/obj_ctl_alloc_class/obj_ctl_alloc_class.c, line 159 at r3 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

UT_ASSERTeq(alloc_class_new.class_id, 130); ?

That's not specified in the manpage.


src/test/obj_ctl_alloc_class/TEST0, line 1 at r3 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

shebang

Done.


src/test/obj_ctl_alloc_class_config/TEST0, line 1 at r3 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

ditto

Done.


Comments from Reviewable

Contributor

pbalcer commented Jul 25, 2017

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


doc/libpmemobj.3.md, line 2417 at r2 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

Agree. IMHO, there is no need to provide such details here.

Ok, but then how do I make this limitation clear?


doc/libpmemobj.3.md, line 2512 at r3 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

Wouldn't it be better to use sized types? uint64_t, etc.

This is not a persistent structure, think of it as API function specification - we don't exclusively use fixed width types there either.


src/examples/libpmemobj/slab_allocator/main.c, line 48 at r3 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

A crazy idea - if user defines their object types using POBJ_LAYOUT macros, we could probably define alloc classes for those types under the hood (optionally).

How? It would still require you to repeat all the types in a function body. We can provide a helper macro, but I think an example like this one is enough.


src/include/libpmemobj/ctl.h, line 118 at r3 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

archetype?

Or "prototype". This struct was originally called pobj_alloc_class_proto.
"original pattern from which copies are made"


src/libpmemobj/alloc_class.h, line 118 at r3 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

aclass => c (as for other functions)

Done.


src/libpmemobj/heap_layout.h, line 69 at r3 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

what about CHUNKSIZE_ALIGN_UP()? / ROUND_UP

Done.


src/libpmemobj/palloc.c, line 176 at r2 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

16-bit

Done.


src/libpmemobj/palloc.c, line 179 at r2 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

not "<=" ?
I mean, is 255 a valid class id?

It's not. I fixed the manpage.


src/libpmemobj/pmalloc.c, line 354 at r3 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

It must be handled - assert is not enough.
And no, C compiler would NOT warn you about invalid header type assignment. I.e. p->header_type = 20 is perfectly fine in C. (could be an error in C++)
BTW, MAX_HEADER_TYPES is a member of the enumeration, but it's not a valid header type value.

enum foo {BAR};
int main() {
enum foo f = 5;
return 0;
}

clang main.c -Wassign-enum

main.c:3:22: warning: integer constant not in range of enumerated type 'enum foo' [-Wassign-enum]
enum foo f = 5;
^
1 warning generated.

But done.


src/test/obj_ctl_alloc_class/obj_ctl_alloc_class.c, line 48 at r3 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

no need to break the line

Done.


src/test/obj_ctl_alloc_class/obj_ctl_alloc_class.c, line 159 at r3 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

UT_ASSERTeq(alloc_class_new.class_id, 130); ?

That's not specified in the manpage.


src/test/obj_ctl_alloc_class/TEST0, line 1 at r3 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

shebang

Done.


src/test/obj_ctl_alloc_class_config/TEST0, line 1 at r3 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

ditto

Done.


Comments from Reviewable

@krzycz

This comment has been minimized.

Show comment
Hide comment
@krzycz

krzycz Jul 25, 2017

Contributor

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


src/examples/libpmemobj/slab_allocator/main.c, line 48 at r3 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

How? It would still require you to repeat all the types in a function body. We can provide a helper macro, but I think an example like this one is enough.

OK, so think about it like each type number is mapped to its own (implicitly created - i.e. on the first alloc) allocation class. The unit size is just the size of given type/object (+header), the header type would need to be compact and the only missing information is "units_per_block" value, which can be determined based on some heuristic. Then, when you allocate an object with type number other than 0, pmemobj would pick the right allocation class automatically (no need to use xalloc).

Another option - having the APIs as they are now, we can add additional CTL entry points to map type numbers to alloc classes, so user do not need to use xalloc variants to benefit from allocation classes. They still need to create alloc classes somehow and map them to type numbers (i.e via config file), but there's no need to change a single line of code.

That's just a suggestion for the improvement we can do in the future. Something to discuss.


src/include/libpmemobj/ctl.h, line 118 at r3 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

Or "prototype". This struct was originally called pobj_alloc_class_proto.
"original pattern from which copies are made"

I mean, for me "archetype" means something slightly different. IMHO "prototype" sounds better, but it's a detail.


src/libpmemobj/pmalloc.c, line 354 at r3 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

enum foo {BAR};
int main() {
enum foo f = 5;
return 0;
}

clang main.c -Wassign-enum

main.c:3:22: warning: integer constant not in range of enumerated type 'enum foo' [-Wassign-enum]
enum foo f = 5;
^
1 warning generated.

But done.

Ha! I didn't know that switch.
However,-Wassign-enum seems to be only supported by clang, not gcc (at least not by gcc 6.3). Also, it's not enabled with -Wall (that's why I overlooked that).


Comments from Reviewable

Contributor

krzycz commented Jul 25, 2017

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


src/examples/libpmemobj/slab_allocator/main.c, line 48 at r3 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

How? It would still require you to repeat all the types in a function body. We can provide a helper macro, but I think an example like this one is enough.

OK, so think about it like each type number is mapped to its own (implicitly created - i.e. on the first alloc) allocation class. The unit size is just the size of given type/object (+header), the header type would need to be compact and the only missing information is "units_per_block" value, which can be determined based on some heuristic. Then, when you allocate an object with type number other than 0, pmemobj would pick the right allocation class automatically (no need to use xalloc).

Another option - having the APIs as they are now, we can add additional CTL entry points to map type numbers to alloc classes, so user do not need to use xalloc variants to benefit from allocation classes. They still need to create alloc classes somehow and map them to type numbers (i.e via config file), but there's no need to change a single line of code.

That's just a suggestion for the improvement we can do in the future. Something to discuss.


src/include/libpmemobj/ctl.h, line 118 at r3 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

Or "prototype". This struct was originally called pobj_alloc_class_proto.
"original pattern from which copies are made"

I mean, for me "archetype" means something slightly different. IMHO "prototype" sounds better, but it's a detail.


src/libpmemobj/pmalloc.c, line 354 at r3 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

enum foo {BAR};
int main() {
enum foo f = 5;
return 0;
}

clang main.c -Wassign-enum

main.c:3:22: warning: integer constant not in range of enumerated type 'enum foo' [-Wassign-enum]
enum foo f = 5;
^
1 warning generated.

But done.

Ha! I didn't know that switch.
However,-Wassign-enum seems to be only supported by clang, not gcc (at least not by gcc 6.3). Also, it's not enabled with -Wall (that's why I overlooked that).


Comments from Reviewable

@krzycz

This comment has been minimized.

Show comment
Hide comment
@krzycz

krzycz Jul 25, 2017

Contributor

:lgtm:


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


Comments from Reviewable

Contributor

krzycz commented Jul 25, 2017

:lgtm:


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


Comments from Reviewable

@marcinslusarz

This comment has been minimized.

Show comment
Hide comment
@marcinslusarz

marcinslusarz Jul 25, 2017

Member

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


doc/libpmemobj.3.md, line 2417 at r2 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

Ok, but then how do I make this limitation clear?

You just state there's such limitation. You don't have to tell the exact size of the bitmap.


Comments from Reviewable

Member

marcinslusarz commented Jul 25, 2017

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


doc/libpmemobj.3.md, line 2417 at r2 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

Ok, but then how do I make this limitation clear?

You just state there's such limitation. You don't have to tell the exact size of the bitmap.


Comments from Reviewable

@marcinslusarz

This comment has been minimized.

Show comment
Hide comment
@marcinslusarz

marcinslusarz Jul 25, 2017

Member

Reviewed 6 of 42 files at r1, 2 of 11 files at r3.
Review status: all files reviewed at latest revision, 7 unresolved discussions.


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

If one wants to retrieve information about all allocation classes, the
recommended method is to simply call this entry point for all class ids between
0 and 255 and discard those results for which the function returned an error.

s/255/254/


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

 - **POBJ_HEADER_LEGACY**, string value: `legacy`. Used for allocation classes
	prior to 1.3 version of the library. Not recommended for use.
	Incurs 64 byte metadata overhead for every objects.

s/objects/object/


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

 - **POBJ_HEADER_COMPACT**, string value: `compact`. Used as default for all
	predefined allocation classes.
	Incurs 16 bytes metadata overhead for every objects.

ditto


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

This query, if executed, will create an allocation class with an id of 128 that
has a unit size of 500 bytes, has approximately 1000 units per block and uses

s/approximately/at least/ ?


Comments from Reviewable

Member

marcinslusarz commented Jul 25, 2017

Reviewed 6 of 42 files at r1, 2 of 11 files at r3.
Review status: all files reviewed at latest revision, 7 unresolved discussions.


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

If one wants to retrieve information about all allocation classes, the
recommended method is to simply call this entry point for all class ids between
0 and 255 and discard those results for which the function returned an error.

s/255/254/


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

 - **POBJ_HEADER_LEGACY**, string value: `legacy`. Used for allocation classes
	prior to 1.3 version of the library. Not recommended for use.
	Incurs 64 byte metadata overhead for every objects.

s/objects/object/


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

 - **POBJ_HEADER_COMPACT**, string value: `compact`. Used as default for all
	predefined allocation classes.
	Incurs 16 bytes metadata overhead for every objects.

ditto


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

This query, if executed, will create an allocation class with an id of 128 that
has a unit size of 500 bytes, has approximately 1000 units per block and uses

s/approximately/at least/ ?


Comments from Reviewable

pbalcer added some commits Jun 7, 2017

obj: slab allocator example
This is a simple example demonstrating the usage of the alloc class
CTL API to construct a slab-like allocator mechanism that
eliminates fragmentation for predefined structures.
@pbalcer

This comment has been minimized.

Show comment
Hide comment
@pbalcer

pbalcer Jul 25, 2017

Contributor

Review status: 40 of 42 files reviewed at latest revision, 7 unresolved discussions.


doc/libpmemobj.3.md, line 2417 at r2 (raw file):

Previously, marcinslusarz (Marcin Ślusarz) wrote…

You just state there's such limitation. You don't have to tell the exact size of the bitmap.

Done.


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

Previously, marcinslusarz (Marcin Ślusarz) wrote…

s/255/254/

Done.


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

Previously, marcinslusarz (Marcin Ślusarz) wrote…

s/objects/object/

Done.


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

Previously, marcinslusarz (Marcin Ślusarz) wrote…

ditto

Done.


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

Previously, marcinslusarz (Marcin Ślusarz) wrote…

s/approximately/at least/ ?

Done.


src/include/libpmemobj/ctl.h, line 118 at r3 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

I mean, for me "archetype" means something slightly different. IMHO "prototype" sounds better, but it's a detail.

Done.


Comments from Reviewable

Contributor

pbalcer commented Jul 25, 2017

Review status: 40 of 42 files reviewed at latest revision, 7 unresolved discussions.


doc/libpmemobj.3.md, line 2417 at r2 (raw file):

Previously, marcinslusarz (Marcin Ślusarz) wrote…

You just state there's such limitation. You don't have to tell the exact size of the bitmap.

Done.


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

Previously, marcinslusarz (Marcin Ślusarz) wrote…

s/255/254/

Done.


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

Previously, marcinslusarz (Marcin Ślusarz) wrote…

s/objects/object/

Done.


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

Previously, marcinslusarz (Marcin Ślusarz) wrote…

ditto

Done.


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

Previously, marcinslusarz (Marcin Ślusarz) wrote…

s/approximately/at least/ ?

Done.


src/include/libpmemobj/ctl.h, line 118 at r3 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

I mean, for me "archetype" means something slightly different. IMHO "prototype" sounds better, but it's a detail.

Done.


Comments from Reviewable

@marcinslusarz

This comment has been minimized.

Show comment
Hide comment
@marcinslusarz

marcinslusarz Jul 25, 2017

Member

I looked only at the interface and documentation and this part :lgtm:.


Reviewed 2 of 2 files at r5.
Review status: all files reviewed at latest revision, 3 unresolved discussions.


Comments from Reviewable

Member

marcinslusarz commented Jul 25, 2017

I looked only at the interface and documentation and this part :lgtm:.


Reviewed 2 of 2 files at r5.
Review status: all files reviewed at latest revision, 3 unresolved discussions.


Comments from Reviewable

@krzycz

This comment has been minimized.

Show comment
Hide comment
@krzycz

krzycz Jul 25, 2017

Contributor

Reviewed 2 of 2 files at r5.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

Contributor

krzycz commented Jul 25, 2017

Reviewed 2 of 2 files at r5.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@krzycz krzycz merged commit 852860d into pmem:master Jul 25, 2017

3 checks passed

code-review/reviewable 42 files reviewed
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