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

Really free stack after last BearSSL obj destroyed #5185

Merged
merged 1 commit into from
Sep 30, 2018

Conversation

earlephilhower
Copy link
Collaborator

The BearSSL second stack, once allocated, was never deallocated. The
reference count of the stack pointer never hit 0 due to the initial
creation counting as one. Now, check to see if there is only one use_count
and if so then delete the stack.

@earlephilhower
Copy link
Collaborator Author

This fixes the secondary issue found in #5175 by @MarcFinns.

@MarcFinns
Copy link

This fixes the secondary issue found in #5175 by @MarcFinns.

That was quick... Thanks!
DO you plan to release the fix in the main distribution anytime soon or shall I just use the github version?
thanks again!

@earlephilhower
Copy link
Collaborator Author

December is the next "scheduled" release, so git head is going to be the best bet for some time.

@MarcFinns
Copy link

Hi Earle, I erased the installation and followed the instructions to install from git (on macOS).
I still see the issue in the test sketch. what did i miss?
thanks again.

@earlephilhower
Copy link
Collaborator Author

You need to apply the PR patch until this is merged. See https://help.github.com/articles/checking-out-pull-requests-locally/ for more info.

@MarcFinns
Copy link

Thanks. I noticed that the version from GitHub (without this patch) uses more ram than the 2.4.2. Is this expected?
As part from these teething issues, BearSSL seems a great addition.

The BearSSL second stack, once allocated, was never deallocated.  The
reference count of the stack pointer never hit 0 due to the initial
creation counting as one.  Now, check to see if there is only one use_count
and if so then delete the stack.
@MarcFinns
Copy link

I confirm the specific issue is fixed.
However, the GIT version uses about 4k RAM more than the 2.4.2 release. I can see that right after compilation and at runtime.
Is this another issue?

@earlephilhower
Copy link
Collaborator Author

Looks basically the same to me. You're going to need a new bug and MCVE.

Using your test sample and git head there's some difference, but it's <1KB:

Setup - before instantiation - Free Heap: 49688
Setup - After instantiation - Free Heap: 44920
Setup - After delete - Free Heap: 49688
Loop - Free residual Heap: 49704

tags/2.4.2:

Setup - before instantiation - Free Heap: 49192
Setup - After instantiation - Free Heap: 44480
Setup - After delete - Free Heap: 44664
Loop - Free residual Heap: 44680

@devyte
Copy link
Collaborator

devyte commented Sep 30, 2018

@MarcFinns are you using wps in your app?

@MarcFinns
Copy link

Hi Devyte, thanks for the hint, i think this might indeed be related.
I use the WifiManager library. In the latest version there is a patch to disable WPS to comply with ESP 2.4.2.
I see from the source that it kicks in if NO_EXTRA_4K_HEAP is defined.
It appears this is defined in 2.4.2 but not in the git version. can this be?
thanks

@devyte
Copy link
Collaborator

devyte commented Sep 30, 2018

In current git, if you use wps, a heap optimization is disabled, which allocates the cont stack on the heap instead of on top of the sys stack. This is because it was found that wps apparently has large stack use, so requuires the original huge size.
Latest git detects the use of wps and disables automagically. In 2.4.2 you had to select the option from the IDE menu (I think).
Bottom line: if you're using wps, the optimization isn't in effect, and your heap is 4KB smaller.

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