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

Rewrite time driver #2559

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open

Rewrite time driver #2559

wants to merge 17 commits into from

Conversation

bugadani
Copy link
Contributor

@bugadani bugadani commented Nov 18, 2024

This PR does a few things:

  • Reduces the use of critical sections
  • Moves the interrupt handler and/or the associated timer into Alarm, so that we don't have to maintain an array of handlers, and we don't have to index to access initialized timers.
  • Uses embassy's (un)safety requirement to avoid a null check when calling timer callbacks.
  • Checks if there are enough timers to initialize the pre-allocated alarms
  • Creates the alarms array with a macro, so that we don't have to manually define interrupt handlers.
  • Bonus change, though it's not needed with the last commit: #[handler] functions can now be references in const context.
  • The PR also adds a basic embassy benchmark and slightly optimizes the thread-mode executor, though I can submit that separately if needed.

@bugadani bugadani marked this pull request as ready for review November 18, 2024 11:53
esp-hal-embassy/src/time_driver.rs Outdated Show resolved Hide resolved
esp-hal-procmacros/src/lib.rs Show resolved Hide resolved
@bugadani bugadani marked this pull request as draft November 19, 2024 09:14
@bugadani bugadani force-pushed the embassy-cs branch 4 times, most recently from 200c14f to 7011485 Compare November 21, 2024 12:25
@bugadani
Copy link
Contributor Author

bugadani commented Nov 21, 2024

Ran the added benchmark on 0.22 and this PR:

0.22: INFO - Test OK, count=26628, cycles=45065/100
PR: INFO - Test OK, count=26616, cycles=45085/100
PR with Relaxed memory ordering: INFO - Test OK, count=27535, cycles=43580/100

This isn't surprising as the benchmark doesn't rely on the time driver, but at least it also means that set_alarm(u64::MAX) didn't get worse.

@bugadani bugadani force-pushed the embassy-cs branch 2 times, most recently from db36750 to d340d3d Compare November 21, 2024 14:36
@@ -123,7 +123,7 @@ This will use software-interrupt 3 which isn't available for anything else to wa

// we do not care about race conditions between the load and store operations,
// interrupts will only set this value to true.
if SIGNAL_WORK_THREAD_MODE[cpu].load(Ordering::SeqCst) {
if SIGNAL_WORK_THREAD_MODE[cpu].load(Ordering::Acquire) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Acquire ordering doesn't make sense here - there is no matching Release store to define a happens-before relationship with. However, for some inexplicable reason, this measures to be slightly faster than Relaxed. Manually adding a memw instruction before a Relaxed load is significantly worse, so the change with this ordering is likely non-local.

@bugadani bugadani marked this pull request as ready for review November 22, 2024 09:37
@bjoernQ
Copy link
Contributor

bjoernQ commented Nov 22, 2024

Is the benchmark supposed to stay or is it a temporary thing?

@bugadani
Copy link
Contributor Author

I'd like to keep it around

@bjoernQ
Copy link
Contributor

bjoernQ commented Nov 22, 2024

I'd like to keep it around

Just asking because it looks a bit out of place here. I'm also wondering if we should have more benchmarks (for other things) in general and if we should have such a thing in parallel to qa-test and hil-test (maybe not part of this PR)

@bugadani
Copy link
Contributor Author

Just asking because it looks a bit out of place here. I'm also wondering if we should have more benchmarks (for other things) in general and if we should have such a thing in parallel to qa-test and hil-test (maybe not part of this PR)

I guess it fits well enough into qa-test, I'll move it.

Copy link
Member

@MabezDev MabezDev left a comment

Choose a reason for hiding this comment

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

LGTM!

btw did we ever explore placing the executors in RAM? Maybe in this basic bench it won't make a difference, but I can imagine in a bigger app with esp-wifi etc it might.

@bugadani
Copy link
Contributor Author

btw did we ever explore placing the executors in RAM?

Not really, but performance isn't exactly the point of this PR, just a bit of an extra.

I guess we can try, but I'm also hesitant to auto-respond with "just place it in RAM" to perf issues. It may help, but eventually we'll run out of ram. We also can't place everything in RAM - we can't just inject a macro into embassy-executor for its various functions - namely wake_task, or Executor::poll.

@bjoernQ
Copy link
Contributor

bjoernQ commented Nov 23, 2024

Somewhat off-topic but I am thinking about a way to allow users to freely configure placing things in RAM - I should create an issue for that

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.

4 participants