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

Add an ensureSize function #6

Closed
wants to merge 2 commits into from
Closed

Conversation

alberic89
Copy link
Contributor

A suggestion for the ensureSize function.

A lot of code duplication with resize, but I don't see how I can avoid it without loosing optimisations.
Or we need three other resize function for each “group” of resize.

@alberic89
Copy link
Contributor Author

Is it a good idea to execute ensureSize during each scoreImpl ?

@fjebaker
Copy link
Owner

I am reluctant to include an ensureSize member, as I don't think I am convinced of its benefits. resize made sense, as it would allow the user to shrink or expand the buffers as needed, but the user shouldn't need to be constantly doing memory things here, and I would prefer to encourage over-allocating rather than constantly checking and re-allocating.

I would instead consider something like fn hasSize(haystack_len: usize, needle_len: usize) bool which can be used to check if resizing is needed. Then, if a user really does want to dynamically allocate the sizes on the fly, they can easily write a 3 line function that checks hasSize, calls resize if false, and then calls score.

The optimization of only resizing rows or cols respectively is a very small optimization, and it's not in the hot path, so it makes very little difference. The largest memory operation is (re)allocating the matrices, which will have to be done regardless of whether the rows or cols grow. Given that it's also a little awkward to fit into the current code, I am inclined to reject it.

Would you agree?

Is it a good idea to execute ensureSize during each scoreImpl ?

No. scoreImpl should not touch memory things to avoid possibly raising errors, and to keep its execution behaviour consistent. The idea is very much that init sets up the finder (raising errors on failing preconditions), and then score uses it.

@alberic89
Copy link
Contributor Author

With time, I understand what you want to say. I come from the Python world, so a lot of principle of memory management are not familiar to me, and my poor English don't help. For me, it was better to hide the complexity of the memory allocation as far as possible. But it is in contradiction with the Zig principles.

So yes, I recognize that it was a bad idea, so we can reject it.

I will add an hasSize function from a clean base in another PR, so we can close this.

@alberic89 alberic89 closed this Aug 13, 2024
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.

2 participants