-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Minor yield() cleanup: optimistic_yield, proper polledTimeout use #6869
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
Conversation
cb02120
to
3bc7d7d
Compare
3bc7d7d
to
d1e490c
Compare
d1e490c
to
6345e0a
Compare
ef9ed97
to
d6d49f2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally seems good, but I do have a style concern.
It seems to me that instead of putting in a hardcoded 10000
everywhere, a constexpr int maxyield = 10000;
(or whatever name) in a header set to that would be more maintainable. I'm not fully sure there the 10ms came from or that it is the "right" number for all SDKs. Factoring it to a const that's in one place would be a much safer bet IMHO...
9a3d0da
to
8ccc347
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The assumption of this PR is that calling yield()
hurts performance.
However, this is not a multitasking system and if there's no WIFI to service or timer callbacks, a yield
is almost a call-and-immediate-return. Yes, it's not instantaneous, but the chip is not going to go off and try to do something else unless there's really other work to be done.
This PR is about performance, but the changes done here are not in performance critical sections. Debug printing hex values, switching APs off and then on again, etc. If it was inside a loop run 1,000 times or something, sure, but these changes are not like that.
Removing forced yield() can change behavior. Before if I put a message into a queue that would be serviced by the OS, the yield() would ensure that queue was pumped by the OS. Optimistic_yield() means that in most cases the queue won't be pumped (unless it's been 10ms already).
So in general, I'm not seeing this as helping things in an appreciable way while introducing changes which might cause instability or non-repeatable behavior.
8ccc347
to
97624b2
Compare
97624b2
to
32c0a8e
Compare
32c0a8e
to
7fe6e99
Compare
7fe6e99
to
33154a9
Compare
33154a9
to
63be40d
Compare
a79f768
to
f9c0a56
Compare
f9c0a56
to
61e5c6b
Compare
14513dd
to
76deff8
Compare
76deff8
to
c06f208
Compare
c06f208
to
d8369ae
Compare
d8369ae
to
6578f1e
Compare
6578f1e
to
7571c72
Compare
A reply to the performance remark you made, @earlephilhower.
Result:
|
Thanks for the actual performance numbers! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, seems like a very minor change with not much impact except in, say, hexdump().
Suggest by @devyte, this PR attempts to point out and refactor those places in cores and libraries that appear to be calling yield() more often than needed, which would hurt performance.
Please review carefully.