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

pthread: avoid heap alloc in pthread_cond_timedwait if possible (IDFGH-7409) #8987

Closed
wants to merge 2 commits into from

Conversation

MacDue
Copy link
Contributor

@MacDue MacDue commented May 19, 2022

This creates the wait sem with xSemaphoreCreateCountingStatic if possible taking around 80 bytes off the stack.
This semaphore is only used for the life of this function, which can be quite heavily called, so this avoids any possible heap/performance issues.

@espressif-bot espressif-bot added the Status: Opened Issue is new label May 19, 2022
@github-actions github-actions bot changed the title pthread: avoid heap alloc in pthread_cond_timedwait if possible pthread: avoid heap alloc in pthread_cond_timedwait if possible (IDFGH-7409) May 19, 2022
@MacDue MacDue force-pushed the no-heap branch 3 times, most recently from a754362 to 4081737 Compare May 19, 2022 14:54
@Alvin1Zhang
Copy link
Collaborator

Thanks for your contribution.

Copy link
Member

@igrr igrr left a comment

Choose a reason for hiding this comment

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

This looks like a useful improvement, thank you! Have left one minor comment.

By the way, have you done any benchmarks to see how much this change improves performance?

components/pthread/pthread_cond_var.c Outdated Show resolved Hide resolved
@MacDue
Copy link
Contributor Author

MacDue commented May 24, 2022

I don't think this changes the runtime performance really (since you normally will be waiting at least a few milliseconds in the cond_timedwait).

My main thought with this change was to avoid the possibility of heap fragmentation when waiting for longer timeouts,
though I've not got anything quantifiable for that (it was just something I thought about when looking over the code due to an unrelated issue).

@espressif-bot espressif-bot added Status: Selected for Development Issue is selected for development and removed Status: Opened Issue is new labels Aug 22, 2022
@0xjakob
Copy link
Collaborator

0xjakob commented Mar 10, 2023

sha=c104662192c35ddc01050310ec96705894a75889

@0xjakob 0xjakob added the PR-Sync-Merge Pull request sync as merge commit label Mar 10, 2023
@espressif-bot espressif-bot added Status: Reviewing Issue is being reviewed Resolution: NA Issue resolution is unavailable Status: Done Issue is done internally Resolution: Done Issue is done internally and removed Status: Selected for Development Issue is selected for development Status: Reviewing Issue is being reviewed Resolution: NA Issue resolution is unavailable labels Mar 13, 2023
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@0xjakob
Copy link
Collaborator

0xjakob commented Apr 25, 2023

@MacDue Your code is available on master already. Because it is merged already, we are closing this PR.

@0xjakob 0xjakob closed this Apr 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR-Sync-Merge Pull request sync as merge commit Resolution: Done Issue is done internally Status: Done Issue is done internally
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants