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

examples: extend radix_tree_complex_value example#1051

Merged
lukaszstolarczuk merged 1 commit intopmem:masterfrom
igchor:example_complex_p
Mar 10, 2021
Merged

examples: extend radix_tree_complex_value example#1051
lukaszstolarczuk merged 1 commit intopmem:masterfrom
igchor:example_complex_p

Conversation

@igchor
Copy link
Copy Markdown
Contributor

@igchor igchor commented Mar 5, 2021

to show that creating city_info on dram is not possible and show
example with a structure wrapped in pmem::obj::p<>


This change is Reviewable

@igchor igchor force-pushed the example_complex_p branch from 3a78cf3 to 72c2c5a Compare March 5, 2021 15:10
@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 5, 2021

Codecov Report

Merging #1051 (ee95c83) into master (7cc2f38) will decrease coverage by 0.04%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1051      +/-   ##
==========================================
- Coverage   94.27%   94.23%   -0.05%     
==========================================
  Files          48       48              
  Lines        4720     4720              
==========================================
- Hits         4450     4448       -2     
- Misses        270      272       +2     
Flag Coverage Δ
tests_clang_debug_cpp17 93.66% <ø> (-0.07%) ⬇️
tests_gcc_debug 92.03% <ø> (-0.08%) ⬇️

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

Impacted Files Coverage Δ
...libpmemobj++/detail/enumerable_thread_specific.hpp 98.91% <0.00%> (-1.09%) ⬇️
...ude/libpmemobj++/container/concurrent_hash_map.hpp 94.35% <0.00%> (-0.48%) ⬇️
include/libpmemobj++/condition_variable.hpp 78.57% <0.00%> (+4.76%) ⬆️

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 7cc2f38...ee95c83. 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.

Reviewable status: 0 of 1 files reviewed, 7 unresolved discussions (waiting on @igchor)


examples/radix_tree/radix_tree_complex_value.cpp, line 43 at r1 (raw file):

	/* If some fields are often updated at the same time, it might
	 * be better to put the inside a structure and wrap the whole structure

to put them?


examples/radix_tree/radix_tree_complex_value.cpp, line 47 at r1 (raw file):

	 * (which means better performance).
	 *
	 * Is date structure is only used in city_info, year, month and day do

Is -> If


examples/radix_tree/radix_tree_complex_value.cpp, line 48 at r1 (raw file):

	 *
	 * Is date structure is only used in city_info, year, month and day do
	 * not have to be wrapped in pmem::obj::p<>. However, it recommended to

it's


examples/radix_tree/radix_tree_complex_value.cpp, line 49 at r1 (raw file):

	 * Is date structure is only used in city_info, year, month and day do
	 * not have to be wrapped in pmem::obj::p<>. However, it recommended to
	 * do so, if the date can be used in some other context.

I'm, not sure what you mean in this comment - we should wrap year/month/date if we use the date outside of the city_info structure...?


examples/radix_tree/radix_tree_complex_value.cpp, line 82 at r1 (raw file):

		/* WRONG: cannot create city_info on stack (DRAM) since
		 * pmem::obj::string can only be placed on pmem:

you could add info to be completely clear `can't create the structure on stack (since ...) and pass it to radix_tree
or maybe
just extend the info above, that these 3 args you pass to try_emplace are passed to the city_info ctor


examples/radix_tree/radix_tree_complex_value.cpp, line 96 at r1 (raw file):

	 * be updated or none of them (even in case of failure).
	 *
	 * This code will result in two snaphots.

perhaps will result in -> will make


examples/radix_tree/radix_tree_complex_value.cpp, line 105 at r1 (raw file):

	/* Update "Gdańsk" latest_update_date field.
	 *
	 * This code will result in only a single snaphot.

a single -> one

to show that creating city_info on dram is not possible and show
example with a structure wrapped in pmem::obj::p<>
@igchor igchor force-pushed the example_complex_p branch from 72c2c5a to ee95c83 Compare March 9, 2021 10:19
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: 0 of 1 files reviewed, 7 unresolved discussions (waiting on @lukaszstolarczuk)


examples/radix_tree/radix_tree_complex_value.cpp, line 43 at r1 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

to put them?

Done.


examples/radix_tree/radix_tree_complex_value.cpp, line 47 at r1 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

Is -> If

Done.


examples/radix_tree/radix_tree_complex_value.cpp, line 48 at r1 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

it's

Done.


examples/radix_tree/radix_tree_complex_value.cpp, line 49 at r1 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

I'm, not sure what you mean in this comment - we should wrap year/month/date if we use the date outside of the city_info structure...?

Ok, since that's not clear I removed this. What I mean is that using uint64_t for yeat/month/day would be ok if the only way to access date would be through city_info (and hence the outer p<> would do the snapshot). But, maybe it's better to show best practice only anyway.


examples/radix_tree/radix_tree_complex_value.cpp, line 82 at r1 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

you could add info to be completely clear `can't create the structure on stack (since ...) and pass it to radix_tree
or maybe
just extend the info above, that these 3 args you pass to try_emplace are passed to the city_info ctor

Done.


examples/radix_tree/radix_tree_complex_value.cpp, line 96 at r1 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

perhaps will result in -> will make

Done.


examples/radix_tree/radix_tree_complex_value.cpp, line 105 at r1 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

a single -> one

Done.

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, 1 of 1 files at r2.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @lukaszstolarczuk)

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 1 of 1 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @igchor)


examples/radix_tree/radix_tree_complex_value.cpp, line 49 at r1 (raw file):

Previously, igchor (Igor Chorążewicz) wrote…

Ok, since that's not clear I removed this. What I mean is that using uint64_t for yeat/month/day would be ok if the only way to access date would be through city_info (and hence the outer p<> would do the snapshot). But, maybe it's better to show best practice only anyway.

yeah, now as I read this again it was clear, but I guess it may be omitted to not introduce any misunderstandings.

@lukaszstolarczuk lukaszstolarczuk merged commit a365b05 into pmem:master Mar 10, 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.

3 participants