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

Improve the performance of s-shared-start #134

Merged

Conversation

cpitclaudel
Copy link
Contributor

This yields a 2-3x speedup on short strings and 5-15x speedup on longer ones on
my machine. I haven't done in-the-wild benchmarking because I don't currently use s.el, but I noticed this while reading the implementation. Here's a mico-benchmark:

(defvar ~/s1 "/home/clement/.emacs.d/.cask/28.0/bootstrap/s-20180406.808/s.el")
(defvar ~/s2 "/home/clement/.emacs.d/.cask/28.0/bootstrap/commander-20140120.1852/")
(benchmark-run-compiled 100000 (old-s-shared-start ~/s1 ~/s2))
;; Right after GC: (1.114678701 0 0.0); Normal use: (1.2542185909999999 1 0.13476934700000243)
(benchmark-run-compiled 100000 (new-s-shared-start ~/s1 ~/s2))
;; Right after GC: (0.072596075 0 0.0); Normal use: (0.22450463 1 0.1343407259999907) 

A similar trick could be used for s-shared-end, but I don't know of a way to do it without reversing the input strings (it's still ~2x faster, but it allocates, so the benefit is less clear-cut).

Do you need an update to the contributors list as well for the PR?
Also, I have copyright papers for Emacs on file, if that's needed.

This yields a 2-3x speedup on short strings and 5-15x speedup on longer ones on
my machine.
@alphapapa
Copy link

This looks like a nice patch. :)

@cpitclaudel
Copy link
Contributor Author

Ping on this.

@magnars
Copy link
Owner

magnars commented Jun 3, 2021

Thanks!

@magnars magnars merged commit e1710af into magnars:master Jun 3, 2021
@magnars
Copy link
Owner

magnars commented Jun 3, 2021

And thanks for the ping. :)

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.

3 participants