- Sponsor
-
Notifications
You must be signed in to change notification settings - Fork 95
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
Make popup-replace-displayable much faster #135
Conversation
One of my users reported a performance bug, which I tracked down to `popup-replace-displayable` (gcv/julia-snail#110). Calling `popup-replace-displayable` takes ~3 seconds on a string of about 8000 characters on my system, and becomes much slower on larger inputs. The attached PR eliminates the large number of string concatenations and significantly improves the function's performance.
I have implemented a faster version of the function in a9635a2 (defun popup-replace-displayable (str &optional rep)
"Replace non-displayable character from STR.
Optional argument REP is the replacement string of non-displayable character."
(let ((result "") (rep (or rep "")))
(mapc (lambda (ch)
(setq result (concat result
(if (char-displayable-p ch) (string ch)
rep))))
str)
result)) Would this work for you? |
The version you're proposing feels faster than the original, but a little slower than the one in my PR (using the test case I've been working with). More importantly, it seems to treat string properties differently from either the original version or the one in the PR. |
How do you test these cases? I have tested the three functions (
Can you elaborate this? Thanks! |
Here is an adapted but real-world use case:
It uses the The original version of the code takes about 6-7 seconds on my machine. Your version takes about 3 seconds. My version takes ~1 second (still not great, but I can live with it). With regard to properties, observe the column of padded propertized space strings that prefixes each line. Here's the original code and my code: Note the leading space whose background matches the background of the screen. Please ignore the ugly wraparounds: they're an artifact of incorrect string lengths in the example and do not occur in real use. Here's what happens with your code: Here, the propertized spaces disappear after your implementation processes the input string. Obviously there's an ugly hack going on in the code to produce padding between the insertion point and the popup, but I couldn't think of a better workaround at the time. Still, it appears that the original implementation of Thanks for reading, and hope this helps! |
Thanks for the elaboration! I think it make sense! Merged! |
One of my users reported a performance bug, which I tracked down to
popup-replace-displayable
(gcv/julia-snail#110).Calling
popup-replace-displayable
takes ~3 seconds on a string of about 8000 characters on my system, and becomes much slower on larger inputs.The attached PR eliminates the large number of string concatenations and significantly improves the function's performance.