improve the memory footprint of is_prefix and is_suffix - #36
Closed
hannesm wants to merge 1 commit into
Closed
Conversation
Collaborator
|
This seems to be a reimplementation of a subset of #24, however in my PR i was unable to find any performance improvement from such a change. What kind of benchmark did you use? |
kit-ty-kate
reviewed
Oct 14, 2025
| let len_s = String.length str | ||
| and len_suf = String.length suffix in | ||
| let diff = len_s - len_suf in | ||
| let rec aux i = |
Collaborator
There was a problem hiding this comment.
this allocates a closure for str, diff, suffix and len_suf
| and len_suf = String.length suffix in | ||
| let diff = len_s - len_suf in | ||
| let rec aux i = | ||
| if i = len_suf then true |
Collaborator
There was a problem hiding this comment.
Suggested change
| if i = len_suf then true | |
| if Int.equal i len_suf then true |
| let diff = len_s - len_suf in | ||
| let rec aux i = | ||
| if i = len_suf then true | ||
| else if String.unsafe_get str (diff + i) <> String.unsafe_get suffix i then false |
Collaborator
There was a problem hiding this comment.
Suggested change
| else if String.unsafe_get str (diff + i) <> String.unsafe_get suffix i then false | |
| else if not (Char.equal (String.unsafe_get str (diff + i)) (String.unsafe_get suffix i) then false |
| else if String.unsafe_get str i <> String.unsafe_get prefix i then false | ||
| else aux (i + 1) | ||
| in | ||
| len_s >= len_pre && aux 0 |
Collaborator
There was a problem hiding this comment.
Suggested change
| len_s >= len_pre && aux 0 | |
| len_s >= (len_pre : int) && aux 0 |
Owner
Author
|
My benchmark was conex, in which I figured via memtrace that is_prefix consumes a lot of memory. But for patch I don't have figures. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
/cc @kit-ty-kate