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

Addition of new arithmetic performance counter "Count" #3206

Conversation

victor-ludorum
Copy link
Contributor

References #2455

Proposed Changes

This PR has added one new arithmetic performance counter which is count . This mainly returns the count of values present in the accumulator containing values of the underlying counters.

@victor-ludorum victor-ludorum changed the title Addition of new Count arithmetic performance counter Addition of new arithmetic performance counter "Count" Feb 28, 2018
@hkaiser hkaiser added this to the 1.1.0 milestone Mar 1, 2018
@@ -986,6 +986,14 @@ namespace hpx { namespace performance_counters
> counter_t;
gid = components::server::construct<counter_t>(
complemented_info, base_counter_names);
}
else if (p.countername_ == "count") {
Copy link
Member

Choose a reason for hiding this comment

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

The indentation seems to be off here.

{ "/arithmetics/count", performance_counters::counter_aggregating,
"returns the count value of all values of the specified "
"base counters; pass the required base counters as the parameters: "
"/arithmetics/count@<base_counter_name1>,<base_counter_name2>",
Copy link
Member

Choose a reason for hiding this comment

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

If the counter returns the number of values in the accumulator, wouldn't it always return the number of base-counters specified as its argument?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hkaiser Thanks for the review 😊 . Actually , Yes it will return the number of base-counters . Sorry for being little unclear . As mean, median and variance are done on the base counter values . Similarly count is also there to return the count of values .

@hkaiser
Copy link
Member

hkaiser commented Mar 1, 2018

@victor-ludorum while I understand that this is a small project for you to better understand how performance counters work (and kudos to you that you got it in place!), I'm not sure if adding this counter to the HPX core would be useful. What would be a possible use case for it?

@victor-ludorum
Copy link
Contributor Author

victor-ludorum commented Mar 1, 2018

@hkaiser Thanks !! Actually , I have planned to work on this as my GSOC project . Therefore, I want to check that whether the steps which I have thought to implement any new counter is correct or not . Therefore , I have created this PR so that you or any experienced one should suggest the steps is accurate or not .
So, Thanks for the review !!
Usage of Count :
As there are several counters having statistical properties like mean, median and variance are already implemented . But I have not seen any count which may state the number of values in accumulator . I know this is a basic one . But I have thought this should be implemented and should be present in the HPX performance counter . Because , sometime we may need the number of values which are present on which operations are performed . This will be given only by count .

I understand that this is a small project for you to better understand how performance counters work

Please suggest , how can I improve my work . On which part I may work that should be good and more informative . I will soon make a proposal so that you can suggest what should be done in this project to make it worth for GSOC project .

@victor-ludorum
Copy link
Contributor Author

@hkaiser Hello sir 😊 !! Do I close this PR . Before closing I just want one advice that whether this process is appropriate to add any new arithmetic performance counter to HPX .
Thanks !!

@hkaiser
Copy link
Member

hkaiser commented Mar 3, 2018

@victor-ludorum why should we close this PR? We can merge it as soon as all tests pass. Currently, inspect is unhappy (see here: https://9666-4455628-gh.circle-artifacts.com/0/tmp/circle-artifacts.SeTzLMh/hpx_inspect_report.html).

@victor-ludorum
Copy link
Contributor Author

victor-ludorum commented Mar 3, 2018 via email

@victor-ludorum
Copy link
Contributor Author

Hello @hkaiser In the details given in this report . It is giving error for "Endline whitespace" . And I have given those white spaces as to segregate them and all other counters are also added in the same way . So, how Can I rectify that error . Do I omit all white spaces and then check for the report or I may till the present state get accepted .

@hkaiser
Copy link
Member

hkaiser commented Mar 3, 2018

@victor-ludorum the empty lines are ok, they just contain some (at least one) space characters. Remove those and all will be fine.

@victor-ludorum
Copy link
Contributor Author

Okay @hkaiser sir!! I will do it and then examine . Thanks for review!!

@hkaiser hkaiser merged commit 3e55a8f into STEllAR-GROUP:master Mar 4, 2018
@victor-ludorum victor-ludorum deleted the count_arithmetic_performance_counter branch March 5, 2018 11:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants