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

tests/periph_gpio: reduce benchmark iterations #14640

Merged

Conversation

kaspar030
Copy link
Contributor

Contribution description

Our MCU's are predictable enough that 100k iterations should suffice.
No need to wait tens of seconds.

Testing procedure

Issues/PRs references

Found in #12457, was about to increase test time for z1.

@kaspar030 kaspar030 added Area: tests Area: tests and testing framework Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: run tests If set, CI server will run tests on hardware for the labeled PR labels Jul 28, 2020
@kaspar030 kaspar030 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jul 28, 2020
Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

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

If Murdock is happy, this sould be fine.
(But can Murdock even be truly happy with the current nrf52dk situation?)

@gschorcht
Copy link
Contributor

I'm wondering whether it will solve the benchmark problem on ATmega. When I was benchmarking the new GPIO API in PR #14607, I had either to reduce the number of tests to 1000 or to remove the irq_disable and irq_enable calls. With disabled interrupts, the results of the benchmarks were completely nonsense probabyl because the timer was not working correctly. I will try later today.

@kaspar030
Copy link
Contributor Author

I'm wondering whether it will solve the benchmark problem on ATmega. When I was benchmarking the new GPIO API in PR #14607, I had either to reduce the number of tests to 1000 or to remove the irq_disable and irq_enable calls. With disabled interrupts, the results of the benchmarks were completely nonsense probabyl because the timer was not working correctly. I will try later today.

that (results completely nonsense) probably was caused by #14639!

@kaspar030
Copy link
Contributor Author

remove the irq_disable and irq_enable calls.

ah, didn't read good, you had that figured out already...

@gschorcht
Copy link
Contributor

Unfortunatly, 100k iterations are is still too much for ATmega. With bench 0 4, I get

                 gpio_set:    231624us  ---   2.316us per call  ---     431734 calls per sec
               gpio_clear:      6984us  ---   0.069us per call  ---   14318442 calls per sec
              gpio_toggle:      1444us  ---   0.014us per call  ---   69252077 calls per sec
                gpio_read:      6984us  ---   0.069us per call  ---   14318442 calls per sec
               gpio_write:     13232us  ---   0.132us per call  ---    7557436 calls per sec

It makes no sense that gpio_toggle would need only 0.014 us while gpio_set takes 2.316 us although toggle is realized as a combination of first reading the pin and then setting or clearing it 🤔

Just for information, real values are:

                 gpio_set:   4940248us  ---   4.940us per call  ---     202418 calls per sec
               gpio_clear:   5315464us  ---   5.315us per call  ---     188130 calls per sec
              gpio_toggle:  10505756us  ---  10.505us per call  ---      95185 calls per sec
                gpio_read:   5315388us  ---   5.315us per call  ---     188133 calls per sec
               gpio_write:   5377964us  ---   5.377us per call  ---     185943 calls per sec

which makes much more sense.

@gschorcht
Copy link
Contributor

gschorcht commented Jul 29, 2020

remove the irq_disable and irq_enable calls.

ah, didn't read good, you had that figured out already...

Yeah, a year ago when I was already working on the new GPIO API. But this should be solved with PR #14639.

@gschorcht
Copy link
Contributor

remove the irq_disable and irq_enable calls.

ah, didn't read good, you had that figured out already...

That's why, I also used an own version in tests/periph_gpio_exp in PR #14602 😉

/**
* BENCHMARK_FUNC disables the interrupts and does not work correctly on
* some platforms, e.g. ATmega. On other platforms, such as STM32, the
* benchmark test gives the same results with interrupts not disabled.
* Therefore a modified version is used here that does not disable the
* interrupts.
*/
#define MY_BENCHMARK_FUNC(name, runs, func) \
{ \
uint32_t _benchmark_time = xtimer_now_usec(); \
for (unsigned long i = 0; i < runs; i++) { \
func; \
} \
_benchmark_time = (xtimer_now_usec() - _benchmark_time);\
benchmark_print_time(_benchmark_time, runs, name); \
}
So, I can remove it once this PR is merged.

@gschorcht
Copy link
Contributor

It seems that Murdock failed due to a reason that is not related to this PR.

@gschorcht gschorcht added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jul 29, 2020
Our MCU's are predictable enough that 100k iterations should suffice.
@kaspar030 kaspar030 force-pushed the reduce_periph_gpio_benchmark_iterations branch from f751684 to 07c70a9 Compare July 29, 2020 10:06
@kaspar030
Copy link
Contributor Author

Unfortunatly, 100k iterations are is still too much for ATmega. With bench 0 4, I get

too much shouldn't happen, just take longer. I think this pr didn't include the irq disable fix PR. I've rebased, so it should now. Can you double check?

@gschorcht
Copy link
Contributor

Unfortunatly, 100k iterations are is still too much for ATmega. With bench 0 4, I get

too much shouldn't happen, just take longer. I think this pr didn't include the irq disable fix PR. I've rebased, so it should now. Can you double check?

Cool, now it is correct 👍

                 gpio_set:    494064us  ---   4.940us per call  ---     202402 calls per sec
               gpio_clear:    531564us  ---   5.315us per call  ---     188124 calls per sec
              gpio_toggle:   1050596us  ---  10.505us per call  ---      95184 calls per sec
                gpio_read:    531636us  ---   5.316us per call  ---     188098 calls per sec
               gpio_write:    537812us  ---   5.378us per call  ---     185938 calls per sec

@kaspar030
Copy link
Contributor Author

Unrelated tests failed:

--- run_test job results (3 failed, 202 passed, 205 total):
    failed:
    tests/pkg_relic/nrf52dk:gnu
    tests/suit_manifest/nrf52dk:gnu
    tests/xtimer_usleep/nrf52dk:llvm

will re-trigger.

@kaspar030 kaspar030 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: run tests If set, CI server will run tests on hardware for the labeled PR labels Jul 29, 2020
@kaspar030
Copy link
Contributor Author

Cool, now it is correct +1

thanks for confirming!

@gschorcht gschorcht added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jul 29, 2020
@gschorcht
Copy link
Contributor

Murdock is happy, failed tests before were not related to this PR.

@gschorcht gschorcht merged commit d482711 into RIOT-OS:master Jul 29, 2020
@kaspar030 kaspar030 deleted the reduce_periph_gpio_benchmark_iterations branch July 29, 2020 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants