obj: run dynamic size index #1629

Merged
merged 1 commit into from Apr 14, 2017

Conversation

Projects
None yet
3 participants
@pbalcer
Contributor

pbalcer commented Jan 31, 2017

ref: pmem/issues#377


This change is Reviewable

@krzycz

This comment has been minimized.

Show comment
Hide comment
@krzycz

krzycz Feb 1, 2017

Contributor

Reviewed 15 of 17 files at r1, 5 of 5 files at r2.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


src/libpmemobj/alloc_class.c, line 54 at r2 (raw file):

 * The last size that is handled by runs.
 */
#define MAX_RUN_SIZE (CHUNKSIZE * 10)

I don't get it... I thought this is the max. size of an allocation that is allocated from a run. Anything larger than that will simply use N adjacent chunks (N = size / chunksize). Does this change mean we can have runs of allocations i.e. 1M?


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

8	;0	;1	;1	;tx_free
7	;0	;1	;1	;tx_free_next
15	;0	;2	;1	;tx_add

Why does it change?


Comments from Reviewable

Contributor

krzycz commented Feb 1, 2017

Reviewed 15 of 17 files at r1, 5 of 5 files at r2.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


src/libpmemobj/alloc_class.c, line 54 at r2 (raw file):

 * The last size that is handled by runs.
 */
#define MAX_RUN_SIZE (CHUNKSIZE * 10)

I don't get it... I thought this is the max. size of an allocation that is allocated from a run. Anything larger than that will simply use N adjacent chunks (N = size / chunksize). Does this change mean we can have runs of allocations i.e. 1M?


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

8	;0	;1	;1	;tx_free
7	;0	;1	;1	;tx_free_next
15	;0	;2	;1	;tx_add

Why does it change?


Comments from Reviewable

@pbalcer

This comment has been minimized.

Show comment
Hide comment
@pbalcer

pbalcer Feb 1, 2017

Contributor

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


src/libpmemobj/alloc_class.c, line 54 at r2 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

I don't get it... I thought this is the max. size of an allocation that is allocated from a run. Anything larger than that will simply use N adjacent chunks (N = size / chunksize). Does this change mean we can have runs of allocations i.e. 1M?

This defines the absolute hard limit in bytes on what's the last allocation class maximum allowed allocation size. The actual limit depends on the allocation classes.


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

Previously, krzycz (Krzysztof Czurylo) wrote…

Why does it change?

Because there's one additional flush when creating a new run.


Comments from Reviewable

Contributor

pbalcer commented Feb 1, 2017

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


src/libpmemobj/alloc_class.c, line 54 at r2 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

I don't get it... I thought this is the max. size of an allocation that is allocated from a run. Anything larger than that will simply use N adjacent chunks (N = size / chunksize). Does this change mean we can have runs of allocations i.e. 1M?

This defines the absolute hard limit in bytes on what's the last allocation class maximum allowed allocation size. The actual limit depends on the allocation classes.


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

Previously, krzycz (Krzysztof Czurylo) wrote…

Why does it change?

Because there's one additional flush when creating a new run.


Comments from Reviewable

@krzycz

This comment has been minimized.

Show comment
Hide comment
@krzycz

krzycz Feb 1, 2017

Contributor

Review status: all files reviewed at latest revision, 1 unresolved discussion.


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

Previously, pbalcer (Piotr Balcer) wrote…

Because there's one additional flush when creating a new run.

OK, so this affects only the first tx_add(), or at least, not every tx_add(), right?
But why tx_alloc() is not affected - no run is created?


Comments from Reviewable

Contributor

krzycz commented Feb 1, 2017

Review status: all files reviewed at latest revision, 1 unresolved discussion.


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

Previously, pbalcer (Piotr Balcer) wrote…

Because there's one additional flush when creating a new run.

OK, so this affects only the first tx_add(), or at least, not every tx_add(), right?
But why tx_alloc() is not affected - no run is created?


Comments from Reviewable

@pbalcer

This comment has been minimized.

Show comment
Hide comment
@pbalcer

pbalcer Feb 1, 2017

Contributor

Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.


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

Previously, krzycz (Krzysztof Czurylo) wrote…

OK, so this affects only the first tx_add(), or at least, not every tx_add(), right?
But why tx_alloc() is not affected - no run is created?

Yes.
tx_alloc is not affected because the test actually performs one dummy allocation to create that run beforehand. We theoretically should do the same for tx_add but the size of the cache is implementation-defined and not known here.


Comments from Reviewable

Contributor

pbalcer commented Feb 1, 2017

Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.


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

Previously, krzycz (Krzysztof Czurylo) wrote…

OK, so this affects only the first tx_add(), or at least, not every tx_add(), right?
But why tx_alloc() is not affected - no run is created?

Yes.
tx_alloc is not affected because the test actually performs one dummy allocation to create that run beforehand. We theoretically should do the same for tx_add but the size of the cache is implementation-defined and not known here.


Comments from Reviewable

@krzycz

This comment has been minimized.

Show comment
Hide comment
@krzycz

krzycz Feb 1, 2017

Contributor

:lgtm:


Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from Reviewable

Contributor

krzycz commented Feb 1, 2017

:lgtm:


Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from Reviewable

@krzycz

This comment has been minimized.

Show comment
Hide comment
@krzycz

krzycz Feb 7, 2017

Contributor
  1. Please, rebase.
  2. I think we can define the first "incompat" flag right now (as a separate patch) - no need to wait for the next release. It would be great to have some unit tests for that as well.
  3. What about "pmempool convert"? What about auto-conversion?
    I'm not sure if the pool should be auto-converted to the new format when the library is upgraded. If it was created w/o dynamic size index feature/dynamic range cache (no "incompat" feature flag), it should stay like that unless the user decides to explicitly convert it to the new format.
Contributor

krzycz commented Feb 7, 2017

  1. Please, rebase.
  2. I think we can define the first "incompat" flag right now (as a separate patch) - no need to wait for the next release. It would be great to have some unit tests for that as well.
  3. What about "pmempool convert"? What about auto-conversion?
    I'm not sure if the pool should be auto-converted to the new format when the library is upgraded. If it was created w/o dynamic size index feature/dynamic range cache (no "incompat" feature flag), it should stay like that unless the user decides to explicitly convert it to the new format.
@plebioda

This comment has been minimized.

Show comment
Hide comment
@plebioda

plebioda Feb 7, 2017

Contributor

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


src/libpmemobj/alloc_class.c, line 120 at r2 (raw file):

 * Hard limit of chunks per single run.
 */
#define RUN_SIZE_IDX_CAP (8)

do you need the braces here ?


src/libpmemobj/heap_layout.h, line 81 at r2 (raw file):

	CHUNK_TYPE_USED,
	CHUNK_TYPE_RUN,
	CHUNK_TYPE_RUN_DATA,

since there is new type of a chunk you should update the pmempool info


src/test/ex_libpmemobj/out10.log.match, line 1 at r2 (raw file):

nblocks: 184320

$(N) ?


Comments from Reviewable

Contributor

plebioda commented Feb 7, 2017

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


src/libpmemobj/alloc_class.c, line 120 at r2 (raw file):

 * Hard limit of chunks per single run.
 */
#define RUN_SIZE_IDX_CAP (8)

do you need the braces here ?


src/libpmemobj/heap_layout.h, line 81 at r2 (raw file):

	CHUNK_TYPE_USED,
	CHUNK_TYPE_RUN,
	CHUNK_TYPE_RUN_DATA,

since there is new type of a chunk you should update the pmempool info


src/test/ex_libpmemobj/out10.log.match, line 1 at r2 (raw file):

nblocks: 184320

$(N) ?


Comments from Reviewable

@pbalcer

This comment has been minimized.

Show comment
Hide comment
@pbalcer

pbalcer Feb 7, 2017

Contributor
  1. Will do.
  2. Doing what you are suggesting would effectively force us to ship all new versions of the library with all of the older versions. I'm strongly against that. I'd rather we fail to open after upgrading so that the user must opt-in for the new features, otherwise he won't be able to open the pool. What would be the point of that anyway? Experimentation? I can kinda understand that for general purpose file systems, but for our library, where the target is servers, you should never experiment on production data.

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


Comments from Reviewable

Contributor

pbalcer commented Feb 7, 2017

  1. Will do.
  2. Doing what you are suggesting would effectively force us to ship all new versions of the library with all of the older versions. I'm strongly against that. I'd rather we fail to open after upgrading so that the user must opt-in for the new features, otherwise he won't be able to open the pool. What would be the point of that anyway? Experimentation? I can kinda understand that for general purpose file systems, but for our library, where the target is servers, you should never experiment on production data.

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


Comments from Reviewable

@pbalcer

This comment has been minimized.

Show comment
Hide comment
@pbalcer

pbalcer Feb 7, 2017

Contributor

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


src/libpmemobj/alloc_class.c, line 120 at r2 (raw file):

Previously, plebioda (Pawel Lebioda) wrote…

do you need the braces here ?

Done.


src/libpmemobj/heap_layout.h, line 81 at r2 (raw file):

Previously, plebioda (Pawel Lebioda) wrote…

since there is new type of a chunk you should update the pmempool info

Done.


src/test/ex_libpmemobj/out10.log.match, line 1 at r2 (raw file):

Previously, plebioda (Pawel Lebioda) wrote…

$(N) ?

Done.


Comments from Reviewable

Contributor

pbalcer commented Feb 7, 2017

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


src/libpmemobj/alloc_class.c, line 120 at r2 (raw file):

Previously, plebioda (Pawel Lebioda) wrote…

do you need the braces here ?

Done.


src/libpmemobj/heap_layout.h, line 81 at r2 (raw file):

Previously, plebioda (Pawel Lebioda) wrote…

since there is new type of a chunk you should update the pmempool info

Done.


src/test/ex_libpmemobj/out10.log.match, line 1 at r2 (raw file):

Previously, plebioda (Pawel Lebioda) wrote…

$(N) ?

Done.


Comments from Reviewable

@krzycz

This comment has been minimized.

Show comment
Hide comment
@krzycz

krzycz Feb 7, 2017

Contributor

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


Comments from Reviewable

Contributor

krzycz commented Feb 7, 2017

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


Comments from Reviewable

@krzycz

This comment has been minimized.

Show comment
Hide comment
@krzycz

krzycz Feb 9, 2017

Contributor

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


src/test/obj_basic_integration/TEST0, line 48 at r4 (raw file):

setup

create_holey_file 16M $DIR/testfile1

If we need to double the pool size in many tests, perhaps we should rather double the PMEMOBJ_MIN_POOL as well?


Comments from Reviewable

Contributor

krzycz commented Feb 9, 2017

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


src/test/obj_basic_integration/TEST0, line 48 at r4 (raw file):

setup

create_holey_file 16M $DIR/testfile1

If we need to double the pool size in many tests, perhaps we should rather double the PMEMOBJ_MIN_POOL as well?


Comments from Reviewable

@pbalcer

This comment has been minimized.

Show comment
Hide comment
@pbalcer

pbalcer Feb 10, 2017

Contributor

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


src/test/obj_basic_integration/TEST0, line 48 at r4 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

If we need to double the pool size in many tests, perhaps we should rather double the PMEMOBJ_MIN_POOL as well?

That's an API change. Technically speaking, the size at which the user would be able to allocate at least one object of each size is: fixed metadata size (~5 megabytes) + 256 kilobytes * ~50 * nthreads (but up to 2 * ncpus), so it's not a constant.

I'm not against, and I think setting that define to 32 megabytes would be good enough, but not sure if we can do it without bumping any version number.


Comments from Reviewable

Contributor

pbalcer commented Feb 10, 2017

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


src/test/obj_basic_integration/TEST0, line 48 at r4 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

If we need to double the pool size in many tests, perhaps we should rather double the PMEMOBJ_MIN_POOL as well?

That's an API change. Technically speaking, the size at which the user would be able to allocate at least one object of each size is: fixed metadata size (~5 megabytes) + 256 kilobytes * ~50 * nthreads (but up to 2 * ncpus), so it's not a constant.

I'm not against, and I think setting that define to 32 megabytes would be good enough, but not sure if we can do it without bumping any version number.


Comments from Reviewable

@krzycz

This comment has been minimized.

Show comment
Hide comment
@krzycz

krzycz Feb 10, 2017

Contributor

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


src/test/obj_basic_integration/TEST0, line 48 at r4 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

That's an API change. Technically speaking, the size at which the user would be able to allocate at least one object of each size is: fixed metadata size (~5 megabytes) + 256 kilobytes * ~50 * nthreads (but up to 2 * ncpus), so it's not a constant.

I'm not against, and I think setting that define to 32 megabytes would be good enough, but not sure if we can do it without bumping any version number.

Right, that's the API change. Perhaps the min size should depend on the pool format version (which is changing somehow), but then we need to modify the order of checks, as the header must be processed first, then the min pool size can be determined.
Well, let's leave it for now.


Comments from Reviewable

Contributor

krzycz commented Feb 10, 2017

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


src/test/obj_basic_integration/TEST0, line 48 at r4 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

That's an API change. Technically speaking, the size at which the user would be able to allocate at least one object of each size is: fixed metadata size (~5 megabytes) + 256 kilobytes * ~50 * nthreads (but up to 2 * ncpus), so it's not a constant.

I'm not against, and I think setting that define to 32 megabytes would be good enough, but not sure if we can do it without bumping any version number.

Right, that's the API change. Perhaps the min size should depend on the pool format version (which is changing somehow), but then we need to modify the order of checks, as the header must be processed first, then the min pool size can be determined.
Well, let's leave it for now.


Comments from Reviewable

@plebioda

This comment has been minimized.

Show comment
Hide comment
@plebioda

plebioda Feb 23, 2017

Contributor

Reviewed 8 of 11 files at r3, 36 of 36 files at r4.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.


src/test/obj_basic_integration/TEST0, line 48 at r4 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

Right, that's the API change. Perhaps the min size should depend on the pool format version (which is changing somehow), but then we need to modify the order of checks, as the header must be processed first, then the min pool size can be determined.
Well, let's leave it for now.

API change ? This is a define with size so it doesn't break binary compatibility, if an application will be rebuild it will use new size - where is a problem here ? I think we should increase the PMEMOBJ_MIN_POOL..


src/test/obj_basic_integration/TEST3.PS1, line 53 at r4 (raw file):

setup

create_poolset $DIR\testset1 16M:$DIR\testfile1:x r 16M:$DIR\testfile2:x

Maybe we should provide a variable in unittest.sh which holds the minimum size of pmemobj pool ?


src/tools/pmempool/output.c, line 641 at r4 (raw file):

		return "run";
	case CHUNK_TYPE_RUN_DATA:
		return "run data";

is this enough for pmempool info ? The pmempool info prints all the objects allocated from run so if now the run spawns into multiple chunks this should be probably handled right ?


Comments from Reviewable

Contributor

plebioda commented Feb 23, 2017

Reviewed 8 of 11 files at r3, 36 of 36 files at r4.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.


src/test/obj_basic_integration/TEST0, line 48 at r4 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

Right, that's the API change. Perhaps the min size should depend on the pool format version (which is changing somehow), but then we need to modify the order of checks, as the header must be processed first, then the min pool size can be determined.
Well, let's leave it for now.

API change ? This is a define with size so it doesn't break binary compatibility, if an application will be rebuild it will use new size - where is a problem here ? I think we should increase the PMEMOBJ_MIN_POOL..


src/test/obj_basic_integration/TEST3.PS1, line 53 at r4 (raw file):

setup

create_poolset $DIR\testset1 16M:$DIR\testfile1:x r 16M:$DIR\testfile2:x

Maybe we should provide a variable in unittest.sh which holds the minimum size of pmemobj pool ?


src/tools/pmempool/output.c, line 641 at r4 (raw file):

		return "run";
	case CHUNK_TYPE_RUN_DATA:
		return "run data";

is this enough for pmempool info ? The pmempool info prints all the objects allocated from run so if now the run spawns into multiple chunks this should be probably handled right ?


Comments from Reviewable

@krzycz

This comment has been minimized.

Show comment
Hide comment
@krzycz

krzycz Feb 23, 2017

Contributor

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


src/test/obj_basic_integration/TEST0, line 48 at r4 (raw file):

Previously, plebioda (Pawel Lebioda) wrote…

API change ? This is a define with size so it doesn't break binary compatibility, if an application will be rebuild it will use new size - where is a problem here ? I think we should increase the PMEMOBJ_MIN_POOL..

It changes the constant that might be compiled-into the user program.
I.e. if the min pool size used to be 16M and now it's 32M, then old programs (and old pools) would stop working with the new version of the library.
So, for me, this macro must be assumed a part of the API.
Or, we can change the implementation and check the min pool size in pmemobj_create() and not in pmemobj_open(0.


Comments from Reviewable

Contributor

krzycz commented Feb 23, 2017

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


src/test/obj_basic_integration/TEST0, line 48 at r4 (raw file):

Previously, plebioda (Pawel Lebioda) wrote…

API change ? This is a define with size so it doesn't break binary compatibility, if an application will be rebuild it will use new size - where is a problem here ? I think we should increase the PMEMOBJ_MIN_POOL..

It changes the constant that might be compiled-into the user program.
I.e. if the min pool size used to be 16M and now it's 32M, then old programs (and old pools) would stop working with the new version of the library.
So, for me, this macro must be assumed a part of the API.
Or, we can change the implementation and check the min pool size in pmemobj_create() and not in pmemobj_open(0.


Comments from Reviewable

@pbalcer

This comment has been minimized.

Show comment
Hide comment
@pbalcer

pbalcer Feb 24, 2017

Contributor

Review status: 8 of 14 files reviewed at latest revision, 3 unresolved discussions.


src/test/obj_basic_integration/TEST3.PS1, line 53 at r4 (raw file):

Previously, plebioda (Pawel Lebioda) wrote…

Maybe we should provide a variable in unittest.sh which holds the minimum size of pmemobj pool ?

That's a good idea that we can do as a separate patch.


src/tools/pmempool/output.c, line 641 at r4 (raw file):

Previously, plebioda (Pawel Lebioda) wrote…

is this enough for pmempool info ? The pmempool info prints all the objects allocated from run so if now the run spawns into multiple chunks this should be probably handled right ?

spans*

I've previously refactored info_obj to remove any code that duplicated functionality already implemented in libpmemobj and simply linked the tool with library internals.

So yes, that's all that's needed.


Comments from Reviewable

Contributor

pbalcer commented Feb 24, 2017

Review status: 8 of 14 files reviewed at latest revision, 3 unresolved discussions.


src/test/obj_basic_integration/TEST3.PS1, line 53 at r4 (raw file):

Previously, plebioda (Pawel Lebioda) wrote…

Maybe we should provide a variable in unittest.sh which holds the minimum size of pmemobj pool ?

That's a good idea that we can do as a separate patch.


src/tools/pmempool/output.c, line 641 at r4 (raw file):

Previously, plebioda (Pawel Lebioda) wrote…

is this enough for pmempool info ? The pmempool info prints all the objects allocated from run so if now the run spawns into multiple chunks this should be probably handled right ?

spans*

I've previously refactored info_obj to remove any code that duplicated functionality already implemented in libpmemobj and simply linked the tool with library internals.

So yes, that's all that's needed.


Comments from Reviewable

@krzycz

This comment has been minimized.

Show comment
Hide comment
@krzycz

krzycz Feb 27, 2017

Contributor

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


Comments from Reviewable

Contributor

krzycz commented Feb 27, 2017

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


Comments from Reviewable

@krzycz

This comment has been minimized.

Show comment
Hide comment
@krzycz

krzycz Mar 1, 2017

Contributor
  1. You mean in another pull request? I can't see such change in this patch.
  2. I'm a bit lost. Are you against auto-conversion to the new format when a new version of the library is installed, or against the user-controlled upgrade? Also, are you suggesting users should be forced to upgrade the pool fromat - otherwise we'll refuse to open the old pools? Then, effectively, it means we change the format number, not just some compat/incompat flag.

Reviewed 1 of 1 files at r6.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

Contributor

krzycz commented Mar 1, 2017

  1. You mean in another pull request? I can't see such change in this patch.
  2. I'm a bit lost. Are you against auto-conversion to the new format when a new version of the library is installed, or against the user-controlled upgrade? Also, are you suggesting users should be forced to upgrade the pool fromat - otherwise we'll refuse to open the old pools? Then, effectively, it means we change the format number, not just some compat/incompat flag.

Reviewed 1 of 1 files at r6.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@pbalcer

This comment has been minimized.

Show comment
Hide comment
@pbalcer

pbalcer Mar 1, 2017

Contributor
  1. Yes, it's still not clear to me how to do it (as evident by this discussion). And you yourself suggested to do it "as a separate patch" ;)
  2. I'm against "If it was created w/o dynamic size index feature/dynamic range cache (no "incompat" feature flag), it should stay like that unless the user decides to explicitly convert it to the new format". That requirement would introduce HUGE development overhead for every single thing we do for very little value. I understand that this comes from file systems, but I argue that our library has a significantly different upgrade path.

How we handle the actual 'upgrading' of the pool, is a separate discussion.


Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from Reviewable

Contributor

pbalcer commented Mar 1, 2017

  1. Yes, it's still not clear to me how to do it (as evident by this discussion). And you yourself suggested to do it "as a separate patch" ;)
  2. I'm against "If it was created w/o dynamic size index feature/dynamic range cache (no "incompat" feature flag), it should stay like that unless the user decides to explicitly convert it to the new format". That requirement would introduce HUGE development overhead for every single thing we do for very little value. I understand that this comes from file systems, but I argue that our library has a significantly different upgrade path.

How we handle the actual 'upgrading' of the pool, is a separate discussion.


Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from Reviewable

@plebioda

This comment has been minimized.

Show comment
Hide comment
@plebioda

plebioda Mar 3, 2017

Contributor

Reviewed 16 of 22 files at r5, 1 of 1 files at r6.
Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from Reviewable

Contributor

plebioda commented Mar 3, 2017

Reviewed 16 of 22 files at r5, 1 of 1 files at r6.
Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from Reviewable

@pbalcer

This comment has been minimized.

Show comment
Hide comment
@pbalcer

pbalcer Mar 30, 2017

Contributor

image

image

Contributor

pbalcer commented Mar 30, 2017

image

image

@krzycz

This comment has been minimized.

Show comment
Hide comment
@krzycz

krzycz Mar 31, 2017

Contributor
  1. At the beginning of NVML development, we made some assumptions on how would we support backward compatibility, what would be the upgrade path, etc. That's why we have pool format version numbers, and compat/incompat flags in the pool headers, etc.
    The general rule was that the libraries, if possible, should be always backward compatible and should be able to open the pools created with older versions of NVML.
    And this is feasible. In the worst case, you'd need to duplicate some amount of code (i.e. the entire allocator) and keep both versions in the library, switching between them in run time, based on the pool format version.
    In practice, we broke this rule for a couple of times (mainly because you were pushing for that), but even if we did a few exceptions it doesn't mean the rules have changed. Of course, we can discuss it once again and change the rules, but I think we should not do that...

My impression is that when you start working on a new feature/optimization, you don't care about backward compatibility at all, assuming that eventually you'll simply bump up the format version number, and that's it.
So, the changes are designed/implemented in the way that really makes it hard to support both new and old pool formats, and the only the users can do is to stay with the old library version or to upgrade/convert their pools to the new format.

Perhaps we still have time to do the changes in libpmemobj that break compatibility, as NVML is not widely used yet. But I think it might be a good exercise to make a try and do some changes in a way that keeps backward compatibility. At least to see how much effort it really requires, how much of the code needs to be duplicated, and how to test compatibility. I bet one day we'll need to do it anyway.


Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from Reviewable

Contributor

krzycz commented Mar 31, 2017

  1. At the beginning of NVML development, we made some assumptions on how would we support backward compatibility, what would be the upgrade path, etc. That's why we have pool format version numbers, and compat/incompat flags in the pool headers, etc.
    The general rule was that the libraries, if possible, should be always backward compatible and should be able to open the pools created with older versions of NVML.
    And this is feasible. In the worst case, you'd need to duplicate some amount of code (i.e. the entire allocator) and keep both versions in the library, switching between them in run time, based on the pool format version.
    In practice, we broke this rule for a couple of times (mainly because you were pushing for that), but even if we did a few exceptions it doesn't mean the rules have changed. Of course, we can discuss it once again and change the rules, but I think we should not do that...

My impression is that when you start working on a new feature/optimization, you don't care about backward compatibility at all, assuming that eventually you'll simply bump up the format version number, and that's it.
So, the changes are designed/implemented in the way that really makes it hard to support both new and old pool formats, and the only the users can do is to stay with the old library version or to upgrade/convert their pools to the new format.

Perhaps we still have time to do the changes in libpmemobj that break compatibility, as NVML is not widely used yet. But I think it might be a good exercise to make a try and do some changes in a way that keeps backward compatibility. At least to see how much effort it really requires, how much of the code needs to be duplicated, and how to test compatibility. I bet one day we'll need to do it anyway.


Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from Reviewable

@pbalcer

This comment has been minimized.

Show comment
Hide comment
@pbalcer

pbalcer Mar 31, 2017

Contributor

?

This change is backward compatible (i.e, after merging, the new code will seamlessly support the old pools without any conversion), it's just that the old versions of libpmemobj won't work with the layout that would be created by libpmemobj /w changes included in this patch.

Contributor

pbalcer commented Mar 31, 2017

?

This change is backward compatible (i.e, after merging, the new code will seamlessly support the old pools without any conversion), it's just that the old versions of libpmemobj won't work with the layout that would be created by libpmemobj /w changes included in this patch.

@krzycz

This comment has been minimized.

Show comment
Hide comment
@krzycz

krzycz Mar 31, 2017

Contributor

OK, so it looks like we only need to have some incompatibility flag set in the pool header.


Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from Reviewable

Contributor

krzycz commented Mar 31, 2017

OK, so it looks like we only need to have some incompatibility flag set in the pool header.


Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from Reviewable

@krzycz krzycz merged commit 7efeaa6 into pmem:master Apr 14, 2017

3 checks passed

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