obj: rewrite pmalloc inactive run handling logic #1366

Merged
merged 2 commits into from Jan 30, 2017

Conversation

Projects
None yet
4 participants
@pbalcer
Contributor

pbalcer commented Nov 4, 2016

This is the first patch from series related to pmem/issues#219

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.


This change is Reviewable

@pbalcer

This comment has been minimized.

Show comment
Hide comment
@pbalcer

pbalcer Nov 10, 2016

Contributor

Just to illustrate the difference this makes, here are some charts:
free_master

free_rework

An order of magnitude improvement.

Contributor

pbalcer commented Nov 10, 2016

Just to illustrate the difference this makes, here are some charts:
free_master

free_rework

An order of magnitude improvement.

@pbalcer

This comment has been minimized.

Show comment
Hide comment
@pbalcer

pbalcer Nov 14, 2016

Contributor

This also fixes pmem/issues#286

Contributor

pbalcer commented Nov 14, 2016

This also fixes pmem/issues#286

@tomaszkapela

This comment has been minimized.

Show comment
Hide comment
@tomaszkapela

tomaszkapela Nov 14, 2016

Member

Reviewed 1 of 25 files at r1, 3 of 46 files at r2.
Review status: 4 of 57 files reviewed at latest revision, 1 unresolved discussion.


src/libpmemobj/heap.c, line 611 at r2 (raw file):

          break;

  int empty = i == nval - 1 && run->bitmap[i] == r->bitmap_lastval;

I know @marcinslusarz is going to say that people should know operator precedence, but I think they improve readability if used in moderation. I think a couple of () would not hurt here.


Comments from Reviewable

Member

tomaszkapela commented Nov 14, 2016

Reviewed 1 of 25 files at r1, 3 of 46 files at r2.
Review status: 4 of 57 files reviewed at latest revision, 1 unresolved discussion.


src/libpmemobj/heap.c, line 611 at r2 (raw file):

          break;

  int empty = i == nval - 1 && run->bitmap[i] == r->bitmap_lastval;

I know @marcinslusarz is going to say that people should know operator precedence, but I think they improve readability if used in moderation. I think a couple of () would not hurt here.


Comments from Reviewable

@pbalcer

This comment has been minimized.

Show comment
Hide comment
@pbalcer

pbalcer Nov 15, 2016

Contributor

Review status: 4 of 59 files reviewed at latest revision, 1 unresolved discussion.


src/libpmemobj/heap.c, line 611 at r2 (raw file):

Previously, tomaszkapela (Tomasz Kapela) wrote…

I know @marcinslusarz is going to say that people should know operator precedence, but I think they improve readability if used in moderation. I think a couple of () would not hurt here.

Done.

Comments from Reviewable

Contributor

pbalcer commented Nov 15, 2016

Review status: 4 of 59 files reviewed at latest revision, 1 unresolved discussion.


src/libpmemobj/heap.c, line 611 at r2 (raw file):

Previously, tomaszkapela (Tomasz Kapela) wrote…

I know @marcinslusarz is going to say that people should know operator precedence, but I think they improve readability if used in moderation. I think a couple of () would not hurt here.

Done.

Comments from Reviewable

@pbalcer pbalcer referenced this pull request in pmem/issues Nov 16, 2016

Closed

TEST3 in obj_many_size_allocs fails #286

@pbalcer

This comment has been minimized.

Show comment
Hide comment
@pbalcer

pbalcer Nov 18, 2016

Contributor

I've completed the second part of the planned changes as a part of this FEAT, here are performance benefits for pmalloc:

pmalloc_master
pmalloc_seglists

Contributor

pbalcer commented Nov 18, 2016

I've completed the second part of the planned changes as a part of this FEAT, here are performance benefits for pmalloc:

pmalloc_master
pmalloc_seglists

@plebioda

This comment has been minimized.

Show comment
Hide comment
@plebioda

plebioda Jan 27, 2017

Contributor

The travis build failed. Due to the fact that we recently had some issues with memcheck, please make sure the valgrind instrumentation is fine by running tests under valgrind.


Reviewed 3 of 25 files at r1, 10 of 46 files at r2, 17 of 36 files at r3, 42 of 43 files at r4.
Review status: 72 of 73 files reviewed at latest revision, 4 unresolved discussions, some commit checks broke.


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

#include "queue.h"

struct seglist_entry {

is this stored in on-media layout ? If so I think it would be a good idea to describe some details here..


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

/*
 * run_get_data -- returns pointer to the data of a huge block

huge_get_data


src/libpmemobj/obj.h, line 174 at r4 (raw file):

	/* padding to align size of this structure to page boundary */
	/* sizeof(unused2) == 8192 - offsetof(struct pmemobjpool, unused2) */
	char unused2[1588];

? without any change in the structure ? weird...


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

	struct recycler *r = Malloc(sizeof(struct recycler));
	if (r == NULL)
		goto error_alloc_tree;

alloc_recycler


Comments from Reviewable

Contributor

plebioda commented Jan 27, 2017

The travis build failed. Due to the fact that we recently had some issues with memcheck, please make sure the valgrind instrumentation is fine by running tests under valgrind.


Reviewed 3 of 25 files at r1, 10 of 46 files at r2, 17 of 36 files at r3, 42 of 43 files at r4.
Review status: 72 of 73 files reviewed at latest revision, 4 unresolved discussions, some commit checks broke.


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

#include "queue.h"

struct seglist_entry {

is this stored in on-media layout ? If so I think it would be a good idea to describe some details here..


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

/*
 * run_get_data -- returns pointer to the data of a huge block

huge_get_data


src/libpmemobj/obj.h, line 174 at r4 (raw file):

	/* padding to align size of this structure to page boundary */
	/* sizeof(unused2) == 8192 - offsetof(struct pmemobjpool, unused2) */
	char unused2[1588];

? without any change in the structure ? weird...


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

	struct recycler *r = Malloc(sizeof(struct recycler));
	if (r == NULL)
		goto error_alloc_tree;

alloc_recycler


Comments from Reviewable

@pbalcer

This comment has been minimized.

Show comment
Hide comment
@pbalcer

pbalcer Jan 27, 2017

Contributor

Travis build failed because of a bug in C++ test...
#1617


Review status: 72 of 73 files reviewed at latest revision, 4 unresolved discussions, some commit checks broke.


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

Previously, plebioda (Pawel Lebioda) wrote…

is this stored in on-media layout ? If so I think it would be a good idea to describe some details here..

This is a transient container and its entries are, by extension, transient. They are however stored in the user data part of the memory blocks. I'll add a comment to explain that.


src/libpmemobj/obj.h, line 174 at r4 (raw file):

Previously, plebioda (Pawel Lebioda) wrote…

? without any change in the structure ? weird...

struct palloc_heap changed.


Comments from Reviewable

Contributor

pbalcer commented Jan 27, 2017

Travis build failed because of a bug in C++ test...
#1617


Review status: 72 of 73 files reviewed at latest revision, 4 unresolved discussions, some commit checks broke.


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

Previously, plebioda (Pawel Lebioda) wrote…

is this stored in on-media layout ? If so I think it would be a good idea to describe some details here..

This is a transient container and its entries are, by extension, transient. They are however stored in the user data part of the memory blocks. I'll add a comment to explain that.


src/libpmemobj/obj.h, line 174 at r4 (raw file):

Previously, plebioda (Pawel Lebioda) wrote…

? without any change in the structure ? weird...

struct palloc_heap changed.


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: 72 of 73 files reviewed at latest revision, 4 unresolved discussions, some commit checks broke.


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

Previously, plebioda (Pawel Lebioda) wrote…

huge_get_data

Done.


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

Previously, plebioda (Pawel Lebioda) wrote…

alloc_recycler

Done.


Comments from Reviewable

Contributor

pbalcer commented Jan 30, 2017

Review status: 72 of 73 files reviewed at latest revision, 4 unresolved discussions, some commit checks broke.


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

Previously, plebioda (Pawel Lebioda) wrote…

huge_get_data

Done.


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

Previously, plebioda (Pawel Lebioda) wrote…

alloc_recycler

Done.


Comments from Reviewable

@krzycz krzycz merged commit 58e010c into pmem:master Jan 30, 2017

2 of 3 checks passed

code-review/reviewable 5 files, 4 discussions left (plebioda)
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@pbalcer

This comment has been minimized.

Show comment
Hide comment
@pbalcer

pbalcer Apr 3, 2017

Contributor

image
image

Contributor

pbalcer commented Apr 3, 2017

image
image

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