Skip to content
This repository was archived by the owner on Mar 22, 2023. It is now read-only.

inline_string: do not check if inline_string is on pmem#1100

Merged
lukaszstolarczuk merged 1 commit intopmem:masterfrom
igchor:inline_string_impr
Jul 2, 2021
Merged

inline_string: do not check if inline_string is on pmem#1100
lukaszstolarczuk merged 1 commit intopmem:masterfrom
igchor:inline_string_impr

Conversation

@igchor
Copy link
Copy Markdown
Contributor

@igchor igchor commented Jun 25, 2021

In pmemkv we have a use for keeping inline_string in DRAM. It's
also safe (unlike for e.g. pmem::obj::string) since inline_string
does not manage allocations.


This change is Reviewable

Copy link
Copy Markdown
Contributor

@KFilipek KFilipek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 1 of 1 files at r1.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @igchor)

@igchor igchor force-pushed the inline_string_impr branch 2 times, most recently from 8a70604 to 3b157a7 Compare June 28, 2021 08:24
@codecov
Copy link
Copy Markdown

codecov Bot commented Jun 28, 2021

Codecov Report

Merging #1100 (6612fd4) into master (4808295) will decrease coverage by 0.03%.
The diff coverage is 100.00%.

❗ Current head 6612fd4 differs from pull request most recent head ed0e48c. Consider uploading reports for the commit ed0e48c to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1100      +/-   ##
==========================================
- Coverage   94.26%   94.23%   -0.04%     
==========================================
  Files          51       51              
  Lines        5181     5221      +40     
==========================================
+ Hits         4884     4920      +36     
- Misses        297      301       +4     
Flag Coverage Δ
tests_clang_debug_cpp17 93.78% <100.00%> (-0.02%) ⬇️
tests_gcc_debug 90.75% <100.00%> (-0.94%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...nclude/libpmemobj++/experimental/inline_string.hpp 98.57% <100.00%> (-0.06%) ⬇️
include/libpmemobj++/experimental/mpsc_queue.hpp 93.81% <0.00%> (-2.98%) ⬇️
include/libpmemobj++/experimental/radix_tree.hpp 98.36% <0.00%> (-0.10%) ⬇️
include/libpmemobj++/container/vector.hpp 92.95% <0.00%> (+0.29%) ⬆️
...ude/libpmemobj++/container/concurrent_hash_map.hpp 94.35% <0.00%> (+0.47%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4808295...ed0e48c. Read the comment docs.

Copy link
Copy Markdown
Member

@lukaszstolarczuk lukaszstolarczuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r1, 1 of 1 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @igchor)

a discussion (no related file):
pls add a sentence or two in docs



tests/inline_string/inline_string.cpp, line 249 at r2 (raw file):

	});
}

perhaps test the opposite - that we can store it on DRAM...?

@igchor igchor force-pushed the inline_string_impr branch from 3b157a7 to 6612fd4 Compare July 1, 2021 14:27
Copy link
Copy Markdown
Contributor Author

@igchor igchor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @lukaszstolarczuk)

a discussion (no related file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

pls add a sentence or two in docs

Done.



tests/inline_string/inline_string.cpp, line 249 at r2 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

perhaps test the opposite - that we can store it on DRAM...?

Done.

Copy link
Copy Markdown
Member

@lukaszstolarczuk lukaszstolarczuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 2 files at r3.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @igchor)


include/libpmemobj++/experimental/inline_string.hpp, line 33 at r3 (raw file):

 * keeps the data within the same allocation as inline_string itself.
 *
 * Unlike other containers, can be used on pmem and dram. Modifiers (like

it can be used...


include/libpmemobj++/experimental/inline_string.hpp, line 34 at r3 (raw file):

 *
 * Unlike other containers, can be used on pmem and dram. Modifiers (like
 * assign() can only be called if inline string is kept on pmem).

something's wrong with the closing parenthesis in this sentence.


include/libpmemobj++/experimental/inline_string.hpp, line 357 at r3 (raw file):

 *
 * @throw std::out_of_range if rhs is larger than capacity.
 * @throw pool_error is inline string is not on pmem.

is ... is (did you mean if ... is?)


tests/inline_string/inline_string.cpp, line 252 at r3 (raw file):

template <typename T>
void
test_dram(nvobj::pool<struct root<T>> &pop)

that's a bit unusual test, pls add some comment(s)


tests/inline_string/inline_string.cpp, line 270 at r3 (raw file):

		  nvobj::basic_string_view<T>(*dram_location));

	dram_location->~basic_inline_string();

did you mean ~string_type? (not sure if you can call it like this)


tests/inline_string/inline_string.cpp, line 275 at r3 (raw file):

	UT_ASSERT(dram_location->capacity() == string_size);
	UT_ASSERT(dram_location->size() == 0);

ASSERTeq

In pmemkv we have a use for keeping inline_string in DRAM. It's
also safe (unlike for e.g. pmem::obj::string) since inline_string
does not manage allocations.
Copy link
Copy Markdown
Contributor Author

@igchor igchor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @lukaszstolarczuk)


include/libpmemobj++/experimental/inline_string.hpp, line 33 at r3 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

it can be used...

Done.


include/libpmemobj++/experimental/inline_string.hpp, line 34 at r3 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

something's wrong with the closing parenthesis in this sentence.

Done.


include/libpmemobj++/experimental/inline_string.hpp, line 357 at r3 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

is ... is (did you mean if ... is?)

Done.


tests/inline_string/inline_string.cpp, line 252 at r3 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

that's a bit unusual test, pls add some comment(s)

Done.


tests/inline_string/inline_string.cpp, line 270 at r3 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

did you mean ~string_type? (not sure if you can call it like this)

Actually yes, done ;d


tests/inline_string/inline_string.cpp, line 275 at r3 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

ASSERTeq

Done.

@igchor igchor force-pushed the inline_string_impr branch from 6612fd4 to ed0e48c Compare July 2, 2021 10:15
Copy link
Copy Markdown
Member

@lukaszstolarczuk lukaszstolarczuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 2 of 2 files at r4.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @igchor)

Copy link
Copy Markdown

@karczex karczex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @igchor)

@lukaszstolarczuk lukaszstolarczuk merged commit a82bbb2 into pmem:master Jul 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants