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

Is the memset in clear() necessary? #9

Closed
cmooney3 opened this issue Apr 11, 2018 · 5 comments
Closed

Is the memset in clear() necessary? #9

cmooney3 opened this issue Apr 11, 2018 · 5 comments

Comments

@cmooney3
Copy link

Hello!

I've been working on a project using this library and noticed that the clear() function seemed to be taking an inordinately long time on my microcontroller. I did a little digging, and the culprit seems to be this line below, where you use memset() to erase all the data in the buffer:

memset(buffer, 0, sizeof(buffer));

Since that function sets the head, tail, and count variables, my suspicion was that this memset isn't actually required. The constructors seem to confirm this since they don't memset the buffer on creation (unless it gets initialized to 0 some other way I'm not seeing).

I went ahead and tested this out and everything seems to work great on my project at least, so I wanted to drop you a message and let you know in case this is a nice low-hanging fruit speedup. If I'm wrong and that is important, then that'd be good to know too so I don't end up hitting some sneaky bug later on.

Love the library, btw! Thanks for writing it, it's saved me a lot of time

Thanks, Charlie

@rlogiacco
Copy link
Owner

I've taken your advice and actually turned the memset switchable via a new, optional, parameter to the clear() function named sweep: it's active by default, for backward compatibility and as it might help recover unused space (under certain circumstances),but you can now provide a false value to obtain maximum performance.
The version has not been released yet, I'll wait for your confirmation it might be a valid approach.

@rlogiacco rlogiacco reopened this Apr 22, 2018
@tonton81
Copy link

if head and tail positions are reset, or a read/pop is done, theres no point to discard data in that slot as its not included in the queue, and further writes will override it anyways

adding memset is pretty redundant, but not necessary

@rlogiacco
Copy link
Owner

if we all agree there is no value in having that clear up, I will remove it in the next version...

@cmooney3
Copy link
Author

Awesome, thanks for looking into it so quick :)

@rlogiacco
Copy link
Owner

I've removed the memset call: in a couple of days I will release the new version for the plugin manager

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

No branches or pull requests

3 participants