obj: introduce customizable allocation header support #1533

Merged
merged 11 commits into from Jan 30, 2017

Conversation

Projects
None yet
3 participants
@pbalcer
Contributor

pbalcer commented Dec 15, 2016

This is the second and final patch related to: pmem/issues#347

This pull request also contains #1435 and #1366 because it depends
on those changes.


This change is Reviewable

@krzycz

This comment has been minimized.

Show comment
Hide comment
@krzycz

krzycz Jan 16, 2017

Contributor

A few general comments:

  1. "obj: simplify memblock abstraction layer"
    Really? Looks like we have different understanding on what "simplify" means. ;-)

  2. The functions/variable names in some files are very long (i.e. 40 chars), which makes it hard to format the code nicely. Self-descriptive type/var names are welcome, but if the single assignment (very_long_name_var1 = very_long_name_var2) can't fit an 80-chars, there must be something wrong there...

Example: container_seglists_insert_block(), container_seglists_get_rm_block_bestfit(), struct block_container_seglists
Why not just container_seglist_insert() or cntnr_sglist_insert() or simply seglist_insert()? And if it comes from the files name, then... rename the file to "seglist.c".
Also, could the structure name(s) be shortened somehow? I.e. blk_ctnr_sglist, bc_seglist, ...


Reviewed 120 of 126 files at r1, 6 of 6 files at r2, 8 of 8 files at r3, 3 of 3 files at r4.
Review status: all files reviewed at latest revision, 26 unresolved discussions.


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

	union {
		/* struct { ... } huge; */

placeholder ? "XXX"?


src/libpmemobj/container_ctree.c, line 2 at r3 (raw file):

/*
 * Copyright 2015-2016, Intel Corporation

2017


src/libpmemobj/container_ctree.c, line 76 at r3 (raw file):

	/*
	 * Even though the memory block representation of an object uses
	 * relatively large types in practise the entire memory block structure

practice


src/libpmemobj/container_seglists.c, line 34 at r2 (raw file):

/*
 * container_seglists.c -- implementation of segregated lists block container

Interesting. 3 years ago we developed almost exactly the same data structure (for the same purpose) for the PMEM allocator used in in the very first prototype of PMEM-aware Redis. :-)


src/libpmemobj/container_seglists.c, line 36 at r2 (raw file):

 * container_seglists.c -- implementation of segregated lists block container
 *
 * This container is constructed from N instrusive lists and a single 8 byte

N or 64 (or "up to 64")?
"instrusive"?


src/libpmemobj/container_seglists.c, line 212 at r2 (raw file):

	bc->super.c_ops = &container_seglists_ops;

	for (uint32_t i = 0; i < SEGLIST_BLOCK_LISTS; ++i)

int or unsigned?


src/libpmemobj/ctree.c, line 395 at r3 (raw file):

	}

	struct node_leaf *l = *dst;

l => leaf


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

	struct allocation_header_legacy *hdr = m->m_ops->get_real_data(m);

	return (uint16_t)(hdr->root_size >> 48ULL);

magic number


src/libpmemobj/obj.c, line 684 at r3 (raw file):

	pmemops_persist(p_ops, &pop->root_offset, sizeof(pop->root_offset));
	pop->root_size = 0;
	pmemops_persist(p_ops, &pop->root_size, sizeof(pop->root_size));
  1. Good opportunity to move initialization of those fields (run_id, root_offset, root_size) to the end. This would make it clear they are not in the persistent (immutable) part of the descriptor.
  2. Seems like it might be a good idea to zero out (and flush) the entire descriptor (4K) first, not only 2K (OBJ_DSC_P_SIZE) -
    pmemops_memset_persist(). Then, we don't need to initialize mentioned fields at all.

src/libpmemobj/palloc.c, line 165 at r3 (raw file):

	 * To provide optimal scaling for multi-threaded applications and reduce
	 * fragmentation the appropriate bucket is chosen depending on the
	 * current thread context and to which allocation class the requested*

*


src/libpmemobj/palloc.h, line 51 at r4 (raw file):

	struct heap_rt *rt;
	uint64_t size;
	uint64_t run_id;

What is the purpose of this field?


src/libpmemobj/recycler.c, line 2 at r4 (raw file):

/*
 * Copyright 2016, Intel Corporation

2017


src/libpmemobj/recycler.c, line 47 at r4 (raw file):

#define RUN_KEY_GET_ZONE_ID(k)\
((uint16_t)(k))

Even though it's correct, it looks strange w/o "& 0xFFFF".


src/libpmemobj/recycler.c, line 108 at r4 (raw file):

	for (int i = 0; i < MAX_BITMAP_VALUES; ++i)
		if (run->bitmap[i] != UINT64_MAX)
			score += !__builtin_popcountll(run->bitmap[i]);

Does it compile on Windows?


src/libpmemobj/recycler.h, line 36 at r4 (raw file):

 * recycler.h -- internal definitions of run recycler
 *
 * This is a container that stores runs that are currently not used by any of

Does it means it is some sort of a "shared" bucket (with respect to threads)?


src/test/obj_constructor/TEST2, line 3 at r3 (raw file):

#!/bin/bash -e
#
# Copyright 2015-2016, Intel Corporation

2017


src/test/obj_first_next/out0.log.match, line 33 at r3 (raw file):

constructor(id = 4)
type:
id = $(nW)

There is no point in printing any id's if we ignore the values.


src/test/obj_heap/obj_heap.c, line 74 at r3 (raw file):

static void
init_run_with_score(struct heap_layout *l, uint32_t chunk_id, int score)

"l" => "hl" ?


src/test/obj_heap/obj_heap.c, line 84 at r3 (raw file):

	run->block_size = 1024;
	memset(run->bitmap, 0xFF, sizeof(run->bitmap));
	UT_ASSERT(score % 64 == 0);

UT_ASSERTeq / UT_ASSERTne (here and below)


src/test/obj_list_valgrind/TEST4, line 47 at r2 (raw file):


require_fs_type pmem non-pmem
#configure_valgrind pmemcheck force-enable ../obj_list/obj_list

?


src/test/obj_pmalloc_basic/Makefile, line 40 at r3 (raw file):

LIBPMEM=y
LIBPMEMOBJ=internal-debug

?


src/test/obj_pmalloc_basic/obj_pmalloc_basic.c, line 192 at r3 (raw file):

	UT_ASSERTeq(palloc_extra(&pop->heap, val), PMALLOC_EXTRA);
	UT_ASSERT((palloc_flags(&pop->heap, val) & PALLOC_FLAG) == PALLOC_FLAG);
	UT_ASSERT(palloc_usable_size(&pop->heap, val) == 112);

magic number


src/test/obj_realloc/Makefile, line 41 at r3 (raw file):

LIBPMEMCOMMON=y
LIBPMEM=y
LIBPMEMOBJ=internal-debug

?


src/test/obj_rpmem_basic_integration/node_0_out5.log.match, line 8 at r2 (raw file):

free
realloc: 0 => 777, size: $(N)
realloc: 777 => 1, size: $(N)

Well, now this test does not check if realloc works as expected. I.e. you don't know if in case of reallocation from size 777 to 1, the object is really downsized. (there was a time it didn't work - that's why this case was added to the test).
Perhaps, instead of using match file, we should move this logic to the test and do something like this:
ASSERTeq(actual_size, ALIGN(requested_size, size_to_alloc_class(requested_size)));


src/test/tools/pmemalloc/Makefile, line 43 at r2 (raw file):

LIBPMEMOBJ=y

CFLAGS += -I$(TOP)/src/libpmemobj -g

?


src/test/tools/pmemobjcli/Makefile, line 46 at r2 (raw file):

TOOLS_COMMON=y
LIBPMEMBLK_PRIV=btt_info_convert2h
CFLAGS+=-g -O0

?


Comments from Reviewable

Contributor

krzycz commented Jan 16, 2017

A few general comments:

  1. "obj: simplify memblock abstraction layer"
    Really? Looks like we have different understanding on what "simplify" means. ;-)

  2. The functions/variable names in some files are very long (i.e. 40 chars), which makes it hard to format the code nicely. Self-descriptive type/var names are welcome, but if the single assignment (very_long_name_var1 = very_long_name_var2) can't fit an 80-chars, there must be something wrong there...

Example: container_seglists_insert_block(), container_seglists_get_rm_block_bestfit(), struct block_container_seglists
Why not just container_seglist_insert() or cntnr_sglist_insert() or simply seglist_insert()? And if it comes from the files name, then... rename the file to "seglist.c".
Also, could the structure name(s) be shortened somehow? I.e. blk_ctnr_sglist, bc_seglist, ...


Reviewed 120 of 126 files at r1, 6 of 6 files at r2, 8 of 8 files at r3, 3 of 3 files at r4.
Review status: all files reviewed at latest revision, 26 unresolved discussions.


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

	union {
		/* struct { ... } huge; */

placeholder ? "XXX"?


src/libpmemobj/container_ctree.c, line 2 at r3 (raw file):

/*
 * Copyright 2015-2016, Intel Corporation

2017


src/libpmemobj/container_ctree.c, line 76 at r3 (raw file):

	/*
	 * Even though the memory block representation of an object uses
	 * relatively large types in practise the entire memory block structure

practice


src/libpmemobj/container_seglists.c, line 34 at r2 (raw file):

/*
 * container_seglists.c -- implementation of segregated lists block container

Interesting. 3 years ago we developed almost exactly the same data structure (for the same purpose) for the PMEM allocator used in in the very first prototype of PMEM-aware Redis. :-)


src/libpmemobj/container_seglists.c, line 36 at r2 (raw file):

 * container_seglists.c -- implementation of segregated lists block container
 *
 * This container is constructed from N instrusive lists and a single 8 byte

N or 64 (or "up to 64")?
"instrusive"?


src/libpmemobj/container_seglists.c, line 212 at r2 (raw file):

	bc->super.c_ops = &container_seglists_ops;

	for (uint32_t i = 0; i < SEGLIST_BLOCK_LISTS; ++i)

int or unsigned?


src/libpmemobj/ctree.c, line 395 at r3 (raw file):

	}

	struct node_leaf *l = *dst;

l => leaf


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

	struct allocation_header_legacy *hdr = m->m_ops->get_real_data(m);

	return (uint16_t)(hdr->root_size >> 48ULL);

magic number


src/libpmemobj/obj.c, line 684 at r3 (raw file):

	pmemops_persist(p_ops, &pop->root_offset, sizeof(pop->root_offset));
	pop->root_size = 0;
	pmemops_persist(p_ops, &pop->root_size, sizeof(pop->root_size));
  1. Good opportunity to move initialization of those fields (run_id, root_offset, root_size) to the end. This would make it clear they are not in the persistent (immutable) part of the descriptor.
  2. Seems like it might be a good idea to zero out (and flush) the entire descriptor (4K) first, not only 2K (OBJ_DSC_P_SIZE) -
    pmemops_memset_persist(). Then, we don't need to initialize mentioned fields at all.

src/libpmemobj/palloc.c, line 165 at r3 (raw file):

	 * To provide optimal scaling for multi-threaded applications and reduce
	 * fragmentation the appropriate bucket is chosen depending on the
	 * current thread context and to which allocation class the requested*

*


src/libpmemobj/palloc.h, line 51 at r4 (raw file):

	struct heap_rt *rt;
	uint64_t size;
	uint64_t run_id;

What is the purpose of this field?


src/libpmemobj/recycler.c, line 2 at r4 (raw file):

/*
 * Copyright 2016, Intel Corporation

2017


src/libpmemobj/recycler.c, line 47 at r4 (raw file):

#define RUN_KEY_GET_ZONE_ID(k)\
((uint16_t)(k))

Even though it's correct, it looks strange w/o "& 0xFFFF".


src/libpmemobj/recycler.c, line 108 at r4 (raw file):

	for (int i = 0; i < MAX_BITMAP_VALUES; ++i)
		if (run->bitmap[i] != UINT64_MAX)
			score += !__builtin_popcountll(run->bitmap[i]);

Does it compile on Windows?


src/libpmemobj/recycler.h, line 36 at r4 (raw file):

 * recycler.h -- internal definitions of run recycler
 *
 * This is a container that stores runs that are currently not used by any of

Does it means it is some sort of a "shared" bucket (with respect to threads)?


src/test/obj_constructor/TEST2, line 3 at r3 (raw file):

#!/bin/bash -e
#
# Copyright 2015-2016, Intel Corporation

2017


src/test/obj_first_next/out0.log.match, line 33 at r3 (raw file):

constructor(id = 4)
type:
id = $(nW)

There is no point in printing any id's if we ignore the values.


src/test/obj_heap/obj_heap.c, line 74 at r3 (raw file):

static void
init_run_with_score(struct heap_layout *l, uint32_t chunk_id, int score)

"l" => "hl" ?


src/test/obj_heap/obj_heap.c, line 84 at r3 (raw file):

	run->block_size = 1024;
	memset(run->bitmap, 0xFF, sizeof(run->bitmap));
	UT_ASSERT(score % 64 == 0);

UT_ASSERTeq / UT_ASSERTne (here and below)


src/test/obj_list_valgrind/TEST4, line 47 at r2 (raw file):


require_fs_type pmem non-pmem
#configure_valgrind pmemcheck force-enable ../obj_list/obj_list

?


src/test/obj_pmalloc_basic/Makefile, line 40 at r3 (raw file):

LIBPMEM=y
LIBPMEMOBJ=internal-debug

?


src/test/obj_pmalloc_basic/obj_pmalloc_basic.c, line 192 at r3 (raw file):

	UT_ASSERTeq(palloc_extra(&pop->heap, val), PMALLOC_EXTRA);
	UT_ASSERT((palloc_flags(&pop->heap, val) & PALLOC_FLAG) == PALLOC_FLAG);
	UT_ASSERT(palloc_usable_size(&pop->heap, val) == 112);

magic number


src/test/obj_realloc/Makefile, line 41 at r3 (raw file):

LIBPMEMCOMMON=y
LIBPMEM=y
LIBPMEMOBJ=internal-debug

?


src/test/obj_rpmem_basic_integration/node_0_out5.log.match, line 8 at r2 (raw file):

free
realloc: 0 => 777, size: $(N)
realloc: 777 => 1, size: $(N)

Well, now this test does not check if realloc works as expected. I.e. you don't know if in case of reallocation from size 777 to 1, the object is really downsized. (there was a time it didn't work - that's why this case was added to the test).
Perhaps, instead of using match file, we should move this logic to the test and do something like this:
ASSERTeq(actual_size, ALIGN(requested_size, size_to_alloc_class(requested_size)));


src/test/tools/pmemalloc/Makefile, line 43 at r2 (raw file):

LIBPMEMOBJ=y

CFLAGS += -I$(TOP)/src/libpmemobj -g

?


src/test/tools/pmemobjcli/Makefile, line 46 at r2 (raw file):

TOOLS_COMMON=y
LIBPMEMBLK_PRIV=btt_info_convert2h
CFLAGS+=-g -O0

?


Comments from Reviewable

@pbalcer

This comment has been minimized.

Show comment
Hide comment
@pbalcer

pbalcer Jan 18, 2017

Contributor
  1. That specific patch removed the MEMBLOCK_OPS set of macros and traded off a little bit of transient storage for smaller CPU footprint and easier to use API.
    In the context of the entire patch, the memblock abstraction layer got less leaky and the heap.c shared code is now much less intertwined with everything else.
  2. I agree, I'll think about changing some of the names.

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


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

Previously, krzycz (Krzysztof Czurylo) wrote…

placeholder ? "XXX"?

this is a union with only a single structure. I've initially had a second, empty structure, but vs complained. This is constructed this way to indicate the way this structure should evolve.


src/libpmemobj/container_ctree.c, line 2 at r3 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

2017

Done.


src/libpmemobj/container_ctree.c, line 76 at r3 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

practice

Done.


src/libpmemobj/container_seglists.c, line 36 at r2 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

N or 64 (or "up to 64")?
"instrusive"?

N as in "you can choose whichever number of lists you want". I've added up to 64.

An intrusive data structure is one that requires its elements to have knowledge about the container.


src/libpmemobj/container_seglists.c, line 212 at r2 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

int or unsigned?

Done.


src/libpmemobj/ctree.c, line 395 at r3 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

l => leaf

I agree with making the variables a bit more verbose, but I don't want to change a single variable. I'll refactor the entire ctree code soonish to make it a bit prettier.


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

Previously, krzycz (Krzysztof Czurylo) wrote…

magic number

Done.


src/libpmemobj/palloc.c, line 165 at r3 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

*

Done.


src/libpmemobj/palloc.h, line 51 at r4 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

What is the purpose of this field?

I needed a way to determine whether a chunk is 'owned' by any of the threads in the current incarnation of the heap. Each time a thread takes ownership of a chunk, it stores a run_id metadata and similarly when a thread no longer needs a chunk, it zeroes the relevant field.


src/libpmemobj/recycler.c, line 2 at r4 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

2017

Done.


src/libpmemobj/recycler.c, line 108 at r4 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

Does it compile on Windows?

Yes, this entire patch compiles on windows :)


src/libpmemobj/recycler.h, line 36 at r4 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

Does it means it is some sort of a "shared" bucket (with respect to threads)?

Yes, in fact initially I wanted to simply create a 'struct bucket recycler;' and use that, but it became obvious that I would have no way of accurately 'scoring' the runs without some intermediary layer. This is cleaner.


src/test/obj_first_next/out0.log.match, line 33 at r3 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

There is no point in printing any id's if we ignore the values.

We already have different tests that check correctness of first/next functions in a programmatic manner. The output of this test is dependent on the internal heap layout, and is annoying to deal with. For example, the id's can be in a different order if you change the object sizes...


src/test/obj_heap/obj_heap.c, line 74 at r3 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

"l" => "hl" ?

"l" is consistent with other usages of this structure.


src/test/obj_heap/obj_heap.c, line 84 at r3 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

UT_ASSERTeq / UT_ASSERTne (here and below)

Done.


src/test/obj_list_valgrind/TEST4, line 47 at r2 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

?

Done.


src/test/obj_pmalloc_basic/Makefile, line 40 at r3 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

?

This should be linked with debug version of the library.


src/test/obj_pmalloc_basic/obj_pmalloc_basic.c, line 192 at r3 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

magic number

Done.


src/test/obj_realloc/Makefile, line 41 at r3 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

?

I've changed this test to use real allocation classes using code from the library, hence requirement to link with it.


src/test/obj_rpmem_basic_integration/node_0_out5.log.match, line 8 at r2 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

Well, now this test does not check if realloc works as expected. I.e. you don't know if in case of reallocation from size 777 to 1, the object is really downsized. (there was a time it didn't work - that's why this case was added to the test).
Perhaps, instead of using match file, we should move this logic to the test and do something like this:
ASSERTeq(actual_size, ALIGN(requested_size, size_to_alloc_class(requested_size)));

obj_realloc tests whether or not realloc actually works with comprehensive programmatic checks.
Hard-codding numbers into match files makes it more difficult to change anything.


src/test/tools/pmemalloc/Makefile, line 43 at r2 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

?

Done.


src/test/tools/pmemobjcli/Makefile, line 46 at r2 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

?

Done.


Comments from Reviewable

Contributor

pbalcer commented Jan 18, 2017

  1. That specific patch removed the MEMBLOCK_OPS set of macros and traded off a little bit of transient storage for smaller CPU footprint and easier to use API.
    In the context of the entire patch, the memblock abstraction layer got less leaky and the heap.c shared code is now much less intertwined with everything else.
  2. I agree, I'll think about changing some of the names.

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


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

Previously, krzycz (Krzysztof Czurylo) wrote…

placeholder ? "XXX"?

this is a union with only a single structure. I've initially had a second, empty structure, but vs complained. This is constructed this way to indicate the way this structure should evolve.


src/libpmemobj/container_ctree.c, line 2 at r3 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

2017

Done.


src/libpmemobj/container_ctree.c, line 76 at r3 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

practice

Done.


src/libpmemobj/container_seglists.c, line 36 at r2 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

N or 64 (or "up to 64")?
"instrusive"?

N as in "you can choose whichever number of lists you want". I've added up to 64.

An intrusive data structure is one that requires its elements to have knowledge about the container.


src/libpmemobj/container_seglists.c, line 212 at r2 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

int or unsigned?

Done.


src/libpmemobj/ctree.c, line 395 at r3 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

l => leaf

I agree with making the variables a bit more verbose, but I don't want to change a single variable. I'll refactor the entire ctree code soonish to make it a bit prettier.


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

Previously, krzycz (Krzysztof Czurylo) wrote…

magic number

Done.


src/libpmemobj/palloc.c, line 165 at r3 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

*

Done.


src/libpmemobj/palloc.h, line 51 at r4 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

What is the purpose of this field?

I needed a way to determine whether a chunk is 'owned' by any of the threads in the current incarnation of the heap. Each time a thread takes ownership of a chunk, it stores a run_id metadata and similarly when a thread no longer needs a chunk, it zeroes the relevant field.


src/libpmemobj/recycler.c, line 2 at r4 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

2017

Done.


src/libpmemobj/recycler.c, line 108 at r4 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

Does it compile on Windows?

Yes, this entire patch compiles on windows :)


src/libpmemobj/recycler.h, line 36 at r4 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

Does it means it is some sort of a "shared" bucket (with respect to threads)?

Yes, in fact initially I wanted to simply create a 'struct bucket recycler;' and use that, but it became obvious that I would have no way of accurately 'scoring' the runs without some intermediary layer. This is cleaner.


src/test/obj_first_next/out0.log.match, line 33 at r3 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

There is no point in printing any id's if we ignore the values.

We already have different tests that check correctness of first/next functions in a programmatic manner. The output of this test is dependent on the internal heap layout, and is annoying to deal with. For example, the id's can be in a different order if you change the object sizes...


src/test/obj_heap/obj_heap.c, line 74 at r3 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

"l" => "hl" ?

"l" is consistent with other usages of this structure.


src/test/obj_heap/obj_heap.c, line 84 at r3 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

UT_ASSERTeq / UT_ASSERTne (here and below)

Done.


src/test/obj_list_valgrind/TEST4, line 47 at r2 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

?

Done.


src/test/obj_pmalloc_basic/Makefile, line 40 at r3 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

?

This should be linked with debug version of the library.


src/test/obj_pmalloc_basic/obj_pmalloc_basic.c, line 192 at r3 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

magic number

Done.


src/test/obj_realloc/Makefile, line 41 at r3 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

?

I've changed this test to use real allocation classes using code from the library, hence requirement to link with it.


src/test/obj_rpmem_basic_integration/node_0_out5.log.match, line 8 at r2 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

Well, now this test does not check if realloc works as expected. I.e. you don't know if in case of reallocation from size 777 to 1, the object is really downsized. (there was a time it didn't work - that's why this case was added to the test).
Perhaps, instead of using match file, we should move this logic to the test and do something like this:
ASSERTeq(actual_size, ALIGN(requested_size, size_to_alloc_class(requested_size)));

obj_realloc tests whether or not realloc actually works with comprehensive programmatic checks.
Hard-codding numbers into match files makes it more difficult to change anything.


src/test/tools/pmemalloc/Makefile, line 43 at r2 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

?

Done.


src/test/tools/pmemobjcli/Makefile, line 46 at r2 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

?

Done.


Comments from Reviewable

@krzycz

This comment has been minimized.

Show comment
Hide comment
@krzycz

krzycz Jan 23, 2017

Contributor

Reviewed 10 of 10 files at r5.
Review status: all files reviewed at latest revision, 10 unresolved discussions, some commit checks broke.


src/libpmemobj/container_seglists.c, line 36 at r2 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

N as in "you can choose whichever number of lists you want". I've added up to 64.

An intrusive data structure is one that requires its elements to have knowledge about the container.

typo: in_s_trusive


src/libpmemobj/ctree.c, line 395 at r3 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

I agree with making the variables a bit more verbose, but I don't want to change a single variable. I'll refactor the entire ctree code soonish to make it a bit prettier.

It's not about making it verbose - it's about not using "l" as var name. It could be "nl" (node leaf)...


src/test/obj_first_next/out0.log.match, line 33 at r3 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

We already have different tests that check correctness of first/next functions in a programmatic manner. The output of this test is dependent on the internal heap layout, and is annoying to deal with. For example, the id's can be in a different order if you change the object sizes...

So, again - it looks like there is no point in printing ID's anymore if the order can change.
Maybe we could just check the number of objects on given list and/or check if all the expected ID's are still there (i.e. sum of ID's should be still the same, even if the order changes, so this could be printed instead).


src/test/obj_pmalloc_basic/Makefile, line 40 at r3 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

This should be linked with debug version of the library.

Why? I don't see anything in this test that would require debug version of the library.


src/test/obj_pmalloc_basic/obj_pmalloc_basic.c, line 192 at r3 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

Done.

It still doesn't explain why it's 112, not 124 or 98. I thought it came from some calculation / metadata size.


src/test/obj_rpmem_basic_integration/node_0_out5.log.match, line 8 at r2 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

obj_realloc tests whether or not realloc actually works with comprehensive programmatic checks.
Hard-codding numbers into match files makes it more difficult to change anything.

  1. Maybe I'm wrong, but when I look at obj_realloc test, I can't see the code that checks whether realloc to smaller size really shrinks the object. It only checks if the usable sizes are not less than requested sizes (before and after realloc).

  2. Assuming obj_realloc test covers that - if the allocation sizes are to be ignored in the match file, there is no need to print them at all in *_basic_integration tests.


Comments from Reviewable

Contributor

krzycz commented Jan 23, 2017

Reviewed 10 of 10 files at r5.
Review status: all files reviewed at latest revision, 10 unresolved discussions, some commit checks broke.


src/libpmemobj/container_seglists.c, line 36 at r2 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

N as in "you can choose whichever number of lists you want". I've added up to 64.

An intrusive data structure is one that requires its elements to have knowledge about the container.

typo: in_s_trusive


src/libpmemobj/ctree.c, line 395 at r3 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

I agree with making the variables a bit more verbose, but I don't want to change a single variable. I'll refactor the entire ctree code soonish to make it a bit prettier.

It's not about making it verbose - it's about not using "l" as var name. It could be "nl" (node leaf)...


src/test/obj_first_next/out0.log.match, line 33 at r3 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

We already have different tests that check correctness of first/next functions in a programmatic manner. The output of this test is dependent on the internal heap layout, and is annoying to deal with. For example, the id's can be in a different order if you change the object sizes...

So, again - it looks like there is no point in printing ID's anymore if the order can change.
Maybe we could just check the number of objects on given list and/or check if all the expected ID's are still there (i.e. sum of ID's should be still the same, even if the order changes, so this could be printed instead).


src/test/obj_pmalloc_basic/Makefile, line 40 at r3 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

This should be linked with debug version of the library.

Why? I don't see anything in this test that would require debug version of the library.


src/test/obj_pmalloc_basic/obj_pmalloc_basic.c, line 192 at r3 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

Done.

It still doesn't explain why it's 112, not 124 or 98. I thought it came from some calculation / metadata size.


src/test/obj_rpmem_basic_integration/node_0_out5.log.match, line 8 at r2 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

obj_realloc tests whether or not realloc actually works with comprehensive programmatic checks.
Hard-codding numbers into match files makes it more difficult to change anything.

  1. Maybe I'm wrong, but when I look at obj_realloc test, I can't see the code that checks whether realloc to smaller size really shrinks the object. It only checks if the usable sizes are not less than requested sizes (before and after realloc).

  2. Assuming obj_realloc test covers that - if the allocation sizes are to be ignored in the match file, there is no need to print them at all in *_basic_integration tests.


Comments from Reviewable

@pbalcer

This comment has been minimized.

Show comment
Hide comment
@pbalcer

pbalcer Jan 25, 2017

Contributor

Review status: 83 of 132 files reviewed at latest revision, 10 unresolved discussions.


src/libpmemobj/container_seglists.c, line 36 at r2 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

typo: in_s_trusive

Done.


src/libpmemobj/ctree.c, line 395 at r3 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

It's not about making it verbose - it's about not using "l" as var name. It could be "nl" (node leaf)...

Done.


src/libpmemobj/obj.c, line 684 at r3 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…
  1. Good opportunity to move initialization of those fields (run_id, root_offset, root_size) to the end. This would make it clear they are not in the persistent (immutable) part of the descriptor.
  2. Seems like it might be a good idea to zero out (and flush) the entire descriptor (4K) first, not only 2K (OBJ_DSC_P_SIZE) -
    pmemops_memset_persist(). Then, we don't need to initialize mentioned fields at all.

I've moved initialization of those fields to the bottom of the function. The second part can be done as a separate patch (I don't want to make this one bigger).


src/test/obj_first_next/out0.log.match, line 33 at r3 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

So, again - it looks like there is no point in printing ID's anymore if the order can change.
Maybe we could just check the number of objects on given list and/or check if all the expected ID's are still there (i.e. sum of ID's should be still the same, even if the order changes, so this could be printed instead).

The code for this test is structured in a way that makes your suggestion quite difficult to actually implement.


src/test/obj_pmalloc_basic/Makefile, line 40 at r3 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

Why? I don't see anything in this test that would require debug version of the library.

By this logic you could say that there is no point in compiling any tests with the debug version of the library because it is not technically speaking required...

This is a test that I heavily rely on during changes of the allocator and I need it to be compiled with debug version for debugging.


src/test/obj_pmalloc_basic/obj_pmalloc_basic.c, line 192 at r3 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

It still doesn't explain why it's 112, not 124 or 98. I thought it came from some calculation / metadata size.

"FIRST_SIZE" as in the first class size.


src/test/obj_rpmem_basic_integration/node_0_out5.log.match, line 8 at r2 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…
  1. Maybe I'm wrong, but when I look at obj_realloc test, I can't see the code that checks whether realloc to smaller size really shrinks the object. It only checks if the usable sizes are not less than requested sizes (before and after realloc).

  2. Assuming obj_realloc test covers that - if the allocation sizes are to be ignored in the match file, there is no need to print them at all in *_basic_integration tests.

Because technically speaking reallocation to a smaller size does not guarantee shrinking of the underlying buffer.


Comments from Reviewable

Contributor

pbalcer commented Jan 25, 2017

Review status: 83 of 132 files reviewed at latest revision, 10 unresolved discussions.


src/libpmemobj/container_seglists.c, line 36 at r2 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

typo: in_s_trusive

Done.


src/libpmemobj/ctree.c, line 395 at r3 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

It's not about making it verbose - it's about not using "l" as var name. It could be "nl" (node leaf)...

Done.


src/libpmemobj/obj.c, line 684 at r3 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…
  1. Good opportunity to move initialization of those fields (run_id, root_offset, root_size) to the end. This would make it clear they are not in the persistent (immutable) part of the descriptor.
  2. Seems like it might be a good idea to zero out (and flush) the entire descriptor (4K) first, not only 2K (OBJ_DSC_P_SIZE) -
    pmemops_memset_persist(). Then, we don't need to initialize mentioned fields at all.

I've moved initialization of those fields to the bottom of the function. The second part can be done as a separate patch (I don't want to make this one bigger).


src/test/obj_first_next/out0.log.match, line 33 at r3 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

So, again - it looks like there is no point in printing ID's anymore if the order can change.
Maybe we could just check the number of objects on given list and/or check if all the expected ID's are still there (i.e. sum of ID's should be still the same, even if the order changes, so this could be printed instead).

The code for this test is structured in a way that makes your suggestion quite difficult to actually implement.


src/test/obj_pmalloc_basic/Makefile, line 40 at r3 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

Why? I don't see anything in this test that would require debug version of the library.

By this logic you could say that there is no point in compiling any tests with the debug version of the library because it is not technically speaking required...

This is a test that I heavily rely on during changes of the allocator and I need it to be compiled with debug version for debugging.


src/test/obj_pmalloc_basic/obj_pmalloc_basic.c, line 192 at r3 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

It still doesn't explain why it's 112, not 124 or 98. I thought it came from some calculation / metadata size.

"FIRST_SIZE" as in the first class size.


src/test/obj_rpmem_basic_integration/node_0_out5.log.match, line 8 at r2 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…
  1. Maybe I'm wrong, but when I look at obj_realloc test, I can't see the code that checks whether realloc to smaller size really shrinks the object. It only checks if the usable sizes are not less than requested sizes (before and after realloc).

  2. Assuming obj_realloc test covers that - if the allocation sizes are to be ignored in the match file, there is no need to print them at all in *_basic_integration tests.

Because technically speaking reallocation to a smaller size does not guarantee shrinking of the underlying buffer.


Comments from Reviewable

@krzycz

This comment has been minimized.

Show comment
Hide comment
@krzycz

krzycz Jan 25, 2017

Contributor

Reviewed 49 of 49 files at r6.
Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks broke.


src/test/obj_pmalloc_basic/Makefile, line 40 at r3 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

By this logic you could say that there is no point in compiling any tests with the debug version of the library because it is not technically speaking required...

This is a test that I heavily rely on during changes of the allocator and I need it to be compiled with debug version for debugging.

OK, now I see what is the problem.
Perhaps we should use internal-debug/internal-nondebug depending on whether this is a static-debug or static-nondebug version of the test binary. I.e. use LIBPMEMOBJ=internal and let make (Makefile.inc) to select the proper variant.
Unfortunately, we only have one "dynamic" version of the test binary, so it must be either debug or nondebug (with respect to statically linked libs) unless we improve our makefiles.


src/test/obj_rpmem_basic_integration/node_0_out5.log.match, line 8 at r2 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

Because technically speaking reallocation to a smaller size does not guarantee shrinking of the underlying buffer.

True, but as a user, I could expect that doing reallocation i.e. from 4M to 4B, the object is actually shrinked - not exactly to 4B, of course, but something much less than 4M. IOW, to the smallest possible size class.


Comments from Reviewable

Contributor

krzycz commented Jan 25, 2017

Reviewed 49 of 49 files at r6.
Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks broke.


src/test/obj_pmalloc_basic/Makefile, line 40 at r3 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

By this logic you could say that there is no point in compiling any tests with the debug version of the library because it is not technically speaking required...

This is a test that I heavily rely on during changes of the allocator and I need it to be compiled with debug version for debugging.

OK, now I see what is the problem.
Perhaps we should use internal-debug/internal-nondebug depending on whether this is a static-debug or static-nondebug version of the test binary. I.e. use LIBPMEMOBJ=internal and let make (Makefile.inc) to select the proper variant.
Unfortunately, we only have one "dynamic" version of the test binary, so it must be either debug or nondebug (with respect to statically linked libs) unless we improve our makefiles.


src/test/obj_rpmem_basic_integration/node_0_out5.log.match, line 8 at r2 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

Because technically speaking reallocation to a smaller size does not guarantee shrinking of the underlying buffer.

True, but as a user, I could expect that doing reallocation i.e. from 4M to 4B, the object is actually shrinked - not exactly to 4B, of course, but something much less than 4M. IOW, to the smallest possible size class.


Comments from Reviewable

@plebioda

This comment has been minimized.

Show comment
Hide comment
@plebioda

plebioda Jan 27, 2017

Contributor

Reviewed 14 of 126 files at r1, 2 of 10 files at r5, 23 of 49 files at r6.
Review status: all files reviewed at latest revision, 6 unresolved discussions, some commit checks broke.


src/libpmemobj/alloc_class.c, line 135 at r6 (raw file):

{
	int n;
	for (n = 0; n < MAX_ALLOCATION_CLASSES; ++n) {

int n = 0; ... ?


src/libpmemobj/alloc_class.c, line 341 at r6 (raw file):

			RUN_UNIT_MAX, RUN_UNIT_MAX_ALLOC);
	if (predefined_class == NULL)
		goto error_alloc_class_create;

blank line


Comments from Reviewable

Contributor

plebioda commented Jan 27, 2017

Reviewed 14 of 126 files at r1, 2 of 10 files at r5, 23 of 49 files at r6.
Review status: all files reviewed at latest revision, 6 unresolved discussions, some commit checks broke.


src/libpmemobj/alloc_class.c, line 135 at r6 (raw file):

{
	int n;
	for (n = 0; n < MAX_ALLOCATION_CLASSES; ++n) {

int n = 0; ... ?


src/libpmemobj/alloc_class.c, line 341 at r6 (raw file):

			RUN_UNIT_MAX, RUN_UNIT_MAX_ALLOC);
	if (predefined_class == NULL)
		goto error_alloc_class_create;

blank line


Comments from Reviewable

pbalcer added some commits Nov 2, 2016

obj: rewrite palloc free and unused run handling logic
This patch introduces deferred updates of the heap's transient state in the
case of free. Currently each free performs multiple tree lookups to locate
neighbours, then performs a tree insert and then finally checks if the
run has been completed freed and if that's the case it removes relevant
entries from the tree and changes the run back to a chunk. All of this is
very expensive and requires a lot of locks in order to be performed safely.
The new changes reduce the free to a simple bit flip under a single lock,
and the transient container update is performed only if the current zone
exhausted all of its memory.

This is a huge simplification of the allocator's algorithm which allows for
reduced locking and more straightforward looking code.
@pbalcer

This comment has been minimized.

Show comment
Hide comment
@pbalcer

pbalcer Jan 30, 2017

Contributor

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


src/libpmemobj/alloc_class.c, line 135 at r6 (raw file):

Previously, plebioda (Pawel Lebioda) wrote…

int n = 0; ... ?

Done.


src/libpmemobj/alloc_class.c, line 341 at r6 (raw file):

Previously, plebioda (Pawel Lebioda) wrote…

blank line

Done.


src/test/obj_rpmem_basic_integration/node_0_out5.log.match, line 8 at r2 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

True, but as a user, I could expect that doing reallocation i.e. from 4M to 4B, the object is actually shrinked - not exactly to 4B, of course, but something much less than 4M. IOW, to the smallest possible size class.

Done.


Comments from Reviewable

Contributor

pbalcer commented Jan 30, 2017

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


src/libpmemobj/alloc_class.c, line 135 at r6 (raw file):

Previously, plebioda (Pawel Lebioda) wrote…

int n = 0; ... ?

Done.


src/libpmemobj/alloc_class.c, line 341 at r6 (raw file):

Previously, plebioda (Pawel Lebioda) wrote…

blank line

Done.


src/test/obj_rpmem_basic_integration/node_0_out5.log.match, line 8 at r2 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

True, but as a user, I could expect that doing reallocation i.e. from 4M to 4B, the object is actually shrinked - not exactly to 4B, of course, but something much less than 4M. IOW, to the smallest possible size class.

Done.


Comments from Reviewable

@krzycz

This comment has been minimized.

Show comment
Hide comment
@krzycz

krzycz Jan 30, 2017

Contributor

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


Comments from Reviewable

Contributor

krzycz commented Jan 30, 2017

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


Comments from Reviewable

@krzycz

This comment has been minimized.

Show comment
Hide comment
@krzycz

krzycz Jan 30, 2017

Contributor

:lgtm:


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


Comments from Reviewable

Contributor

krzycz commented Jan 30, 2017

:lgtm:


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


Comments from Reviewable

@krzycz krzycz merged commit 5e2dded into pmem:master Jan 30, 2017

2 of 3 checks passed

code-review/reviewable 5 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