Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

obj: implement statistics boilerplate macros #2459

Merged
merged 1 commit into from Dec 20, 2017

Conversation

pbalcer
Copy link
Member

@pbalcer pbalcer commented Dec 18, 2017

... and add one example statistic:
size in bytes of the curently allocated objects.

Ref: pmem/issues#676


This change is Reviewable

@pbalcer pbalcer force-pushed the obj-alloc-stats branch 2 times, most recently from 9d6c2ae to e15e13a Compare December 18, 2017 15:26
@krzycz
Copy link
Contributor

krzycz commented Dec 18, 2017

Reviewed 32 of 32 files at r1.
Review status: all files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.


src/libpmemobj/libpmemobj.vcxproj, line 53 at r1 (raw file):

    <ClCompile Include="stats.c" />
    <ClCompile Include="ringbuf.c" />
    <ClCompile Include="stats.c" />

same file twice?


src/libpmemobj/obj.h, line 139 at r1 (raw file):

	uint64_t conversion_flags;

	struct stats_persistent stats_persistent;

So the stats content is limited by the size of pmem_reserved?
What about keeping an OID here and allocate stats dynamically?


src/libpmemobj/stats.c, line 73 at r1 (raw file):

	int arg_in = *(int *)arg;

	pop->stats->enabled = arg_in;
  1. I wonder if enabling it in runtime makes sense. Perhaps it should be allowed only at pool creation/opening (before any alloc/free opertion). Then, we could initialize statistics with some reliable data (i.e. by scanning the heap for allocated objects).
  2. Shouldn't it be a part of persistent state? If the stats are enabled once, probably you want to use them next time the pool is open w/o explicit enabling it again.

src/libpmemobj/stats.c, line 88 at r1 (raw file):

/*
 * stats_new -- allocates and initialize statistics instance

initializes


src/libpmemobj/stats.c, line 113 at r1 (raw file):

stats_delete(struct stats *s)
{
	Free(s->transient);

pmem_persist(s->persistent, ...);
If these are persistent stats, they need to be flushed at some point. Perhaps it should be done at each STATS_INC/STATS_SUB, but that could be an overkill. OTH, if we do it only at pool close, the stats may get inconsistent in case of any failure.


src/libpmemobj/stats.h, line 82 at r1 (raw file):

{\
	int64_t *argv = arg;\
	*argv = (int64_t)pop->stats->type->varname;\

no atomic load here?


src/test/obj_ctl_stats/obj_ctl_stats.c, line 41 at r1 (raw file):

int
main(int argc, char *argv[])
{

obj_ctl_stats


src/test/obj_ctl_stats/TEST0, line 40 at r1 (raw file):

. ../unittest/unittest.sh

require_test_type short

fs any ?


Comments from Reviewable

@pbalcer
Copy link
Member Author

pbalcer commented Dec 19, 2017

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


src/libpmemobj/libpmemobj.vcxproj, line 53 at r1 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

same file twice?

Done.


src/libpmemobj/obj.h, line 139 at r1 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

So the stats content is limited by the size of pmem_reserved?
What about keeping an OID here and allocate stats dynamically?

  1. Yes
  2. Chicken and egg problem ;) But we can compromise and be always off by one.

src/libpmemobj/stats.c, line 73 at r1 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…
  1. I wonder if enabling it in runtime makes sense. Perhaps it should be allowed only at pool creation/opening (before any alloc/free opertion). Then, we could initialize statistics with some reliable data (i.e. by scanning the heap for allocated objects).
  2. Shouldn't it be a part of persistent state? If the stats are enabled once, probably you want to use them next time the pool is open w/o explicit enabling it again.

The majority of statistics should be transient and used to decide how to optimize the use of the library. I do not recommend using these kind of values to make runtime decisions... But we are where we are.

I don't think stats "enabled" should be persistent state - it should be off by default and if you want to debug your application you can enable them through a ctl query. If one wants to ignore recommendations and use them for unintended purposes, one can always use a config file to specify that "stats.enabled = 1;".


src/libpmemobj/stats.c, line 88 at r1 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

initializes

Done.


src/libpmemobj/stats.c, line 113 at r1 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

pmem_persist(s->persistent, ...);
If these are persistent stats, they need to be flushed at some point. Perhaps it should be done at each STATS_INC/STATS_SUB, but that could be an overkill. OTH, if we do it only at pool close, the stats may get inconsistent in case of any failure.

In the design I've decided not to flush the variables at every store - as you rightly notice, it would be too expensive. We might eventually implement a mechanism for the potential drift that might happen, but that's for the future.


src/libpmemobj/stats.h, line 82 at r1 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

no atomic load here?

Done.


src/test/obj_ctl_stats/obj_ctl_stats.c, line 41 at r1 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

obj_ctl_stats

Done.


src/test/obj_ctl_stats/TEST0, line 40 at r1 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

fs any ?

what does that change? what's the default?
Done.


Comments from Reviewable

@pbalcer pbalcer force-pushed the obj-alloc-stats branch 2 times, most recently from fcd7f57 to 191e726 Compare December 19, 2017 11:43
@krzycz
Copy link
Contributor

krzycz commented Dec 19, 2017

:lgtm:


Reviewed 11 of 11 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


src/test/obj_ctl_stats/TEST0, line 40 at r1 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

what does that change? what's the default?
Done.

AFAIR, default is "pmem non-pmem", so w/o "any" the test will be executed twice.


Comments from Reviewable

stats.enabled | rw | - | int | int | - | boolean

Enables or disables runtime collection of statistics. The statistics are never
altered after enabling.
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you mean by "are never altered after enabling"?

Copy link
Member Author

Choose a reason for hiding this comment

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

The statistics are not recalculated whenever you enable them. In other words, if you disable statistics, the operations that are performed won't count towards statistic values after you enable them again.

@codecov-io
Copy link

codecov-io commented Dec 19, 2017

Codecov Report

Merging #2459 into master will decrease coverage by 0.08%.
The diff coverage is 51.21%.

@@            Coverage Diff             @@
##           master    #2459      +/-   ##
==========================================
- Coverage   80.03%   79.94%   -0.09%     
==========================================
  Files         121      122       +1     
  Lines       21002    21038      +36     
  Branches     3791     3794       +3     
==========================================
+ Hits        16808    16819      +11     
- Misses       4194     4219      +25

@gaweinbergi
Copy link

doc/libpmemobj/pmemobj_ctl_get.3.md, line 326 at r3 (raw file):

The statistics are not
recalculated after enabling, so that any operations that happened between
disabling and enabling are not going to be reflected in the values.

Statistics are not recalculated after enabling; any operations that occur between disabling and re-enabling will not be reflected in subsequent values.


Comments from Reviewable

@gaweinbergi
Copy link

@gaweinbergi
Copy link

@krzycz
Copy link
Contributor

krzycz commented Dec 19, 2017

Reviewed 1 of 1 files at r3, 1 of 2 files at r4.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


Comments from Reviewable

... and add one example statistic:
size in bytes of the curently allocated objects.
@pbalcer
Copy link
Member Author

pbalcer commented Dec 20, 2017

Review status: 34 of 35 files reviewed at latest revision, 2 unresolved discussions.


doc/libpmemobj/pmemobj_ctl_get.3.md, line 326 at r3 (raw file):

Previously, gaweinbergi (Glenn Weinberg) wrote…

might

may

Done.


doc/libpmemobj/pmemobj_ctl_get.3.md, line 337 at r4 (raw file):

Previously, gaweinbergi (Glenn Weinberg) wrote…

might

may

Done.


Comments from Reviewable

@krzycz
Copy link
Contributor

krzycz commented Dec 20, 2017

Reviewed 1 of 1 files at r5.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


Comments from Reviewable

@krzycz krzycz merged commit 488df51 into pmem:master Dec 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants