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

Problem: upsdebug*() routines pass data and allocate vars always #685

Merged
merged 10 commits into from Sep 20, 2021

Conversation

jimklimov
Copy link
Member

@jimklimov jimklimov commented Apr 1, 2019

Solution: wrap old well-known API routine names into macros that check debugging level first, and only invest into calling routines and passing data later - if that would not be in vain.

This should make "production-mode" runs a bit more efficient in CPU churn. We have tens of thousands debugging calls that are typically ignored during the daemon lifetime.

Signed-off-by: Jim Klimov EvgenyKlimov@eaton.com

@jimklimov jimklimov added the need testing Code looks reasonable, but the feature would better be tested against hardware or OSes label Apr 1, 2019
Solution: wrap old well-known API routines into macros that
check debugging level first, and only invest into calling
routines and passing data later - if that would not be in vain.

Signed-off-by: Jim Klimov <EvgenyKlimov@eaton.com>
@jimklimov
Copy link
Member Author

jimklimov commented Apr 1, 2019

The results are not extremely conclusive due to fluctuation in numbers and other congestion in the test system.

With both older and newer binaries in place, difference between lack of debugging and full debug (6 -D's) is up to 1 second from about 8 it takes to initialize an ePDU daisy chain, tested like this:

$ time sh -c 'NUT_STATEPATH=/tmp  ./drivers/snmp-ups -s test -x port=${host_pdu} -d 1 -DDDDDD >/dev/null 2>&1'

$ time sh -c 'NUT_STATEPATH=/tmp  ./drivers/snmp-ups -s test -x port=${host_pdu} -d 1 >/dev/null 2>&1'

It seems that the version in PR adds a few fractions of a second benefit compared to "old" version, but this is the part hard to prove :)

With high debugging level in place, both old and new binaries return comparable amounts of lines (29020 vs 29023, when piped to wc -l instead of >/dev/null) so at least no regression here.

@jimklimov jimklimov removed the need testing Code looks reasonable, but the feature would better be tested against hardware or OSes label Apr 4, 2019
@jimklimov jimklimov changed the title [WIP] Problem: upsdebug*() routines pass data and allocate vars always Problem: upsdebug*() routines pass data and allocate vars always Apr 4, 2019
Copy link
Member

@clepple clepple left a comment

Choose a reason for hiding this comment

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

LGTM

@jimklimov jimklimov merged commit 7d2e148 into networkupstools:master Sep 20, 2021
@jimklimov jimklimov deleted the upsdebug-less-overhead branch September 20, 2021 16:05
jimklimov added a commit to jimklimov/nut that referenced this pull request Sep 21, 2021
The parenthesized "(label)" in debug macros is a safer expansion than the original "label"
jimklimov added a commit to jimklimov/nut that referenced this pull request Sep 21, 2021
* The parenthesized "(label)" in debug macros is a safer expansion than the original "label"
* Avoid the dangling "args..." that may be not-specified by caller (C99 compat)
jimklimov added a commit to jimklimov/nut that referenced this pull request Sep 21, 2021
* The parenthesized "(label)" in debug macros is a safer expansion than the original "label"
* Avoid the dangling "args..." that may be not-specified by caller (strict-C99 compat)
jimklimov added a commit to jimklimov/nut that referenced this pull request Sep 21, 2021
* The parenthesized "(label)" in debug macros is a safer expansion than the original "label"
* Avoid the dangling "args..." that may be not-specified by caller (strict-C99 compat)
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

2 participants