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

Refactoring common portions of stumpless_param_to_string and stumpless_element_to_string #183

Open
northernSage opened this issue Jun 8, 2021 · 4 comments
Labels
good first issue something that would be simple for a newcomer to stumpless to work on hacktoberfest a good candidate for hacktoberfest participants help wanted external contributations encouraged refactor changes that require refactoring of existing code

Comments

@northernSage
Copy link
Contributor

This is just a follow up issue to improve on #180.

Quoting from #146:

You may consider refactoring some common portions of stumpless_param_to_string into separate private functions that can be used in both places for efficiency...

Since #180 did not implement the changes described above I'll go ahead and work on a separate PR for this. I have opened the issue only to document my intent in doing so and to have somewhere to discuss related changes if the need arises.

@goatshriek goatshriek added help wanted external contributations encouraged refactor changes that require refactoring of existing code labels Jul 30, 2021
@goatshriek
Copy link
Owner

I'm adding more context to this issue so that it has enough information to be a viable good first issue for newcomers.

The common code between stumpless_param_to_string and stumpless_element_to_string may be tricky to identify, as it is not a simple case of looking at the two functions and finding similar code blocks. Instead, it is (1) the sizing of the resulting param string and (2) the writing of this string into a buffer. Both functions do this separately, and in the case of the element to string function this is redoing work that the param to string function already did.

One solution to this is to implement two new private functions that operate on already-locked parameters. One to get the needed size of the buffer for a parameter string, and another to write the string into a provided buffer and return the number of bytes written.

// declared in include/private/param.h, defined in src/param.c:
size_t locked_get_param_string_size( struct stumpless_param *param );
size_t locked_param_into_buffer( struct stumpless_param *param, char *buffer, size_t buffer_size);

stumpless_param_to_string would then be changed to use these two functions to simply allocate a buffer and write into it. stumpless_element_to_string would also use them by: (1) locking all params to ensure they do not change between calculating the buffer size and writing to the buffer, (2) getting the sum of required sizes to calculate the total needed buffer size, (3) allocating this buffer, (4) writing the params into it, and (5) releasing the params.

This will cut down on heap memory allocations and avoid duplicate work, both of which will improve performance. Be sure to check the benchmarks of stumpless_element_to_string to verify that performance has actually improved. Refer to docs/benchmark.md for details on how to use the benchmark suites.

As with all good first issues, there are a few details missing from this approach, for you to fill in as you encounter them. If you find you need help, please ask here or on the project gitter and someone can help you get past the stumbling block.

@goatshriek goatshriek added the good first issue something that would be simple for a newcomer to stumpless to work on label Aug 3, 2024
@Griezn
Copy link
Contributor

Griezn commented Aug 21, 2024

I'll work on this issue when my other pr is merged, if no one has done this already.

@ghost
Copy link

ghost commented Aug 30, 2024

can I be assigned to this?, unless Griezn has already started

@Griezn
Copy link
Contributor

Griezn commented Aug 31, 2024

I haven't started this yet so you can do it if you want.

@goatshriek goatshriek assigned ghost Sep 4, 2024
@goatshriek goatshriek added the hacktoberfest a good candidate for hacktoberfest participants label Sep 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue something that would be simple for a newcomer to stumpless to work on hacktoberfest a good candidate for hacktoberfest participants help wanted external contributations encouraged refactor changes that require refactoring of existing code
Projects
None yet
Development

No branches or pull requests

3 participants