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

Prefer n-ary over unary combinators where possible #308

Merged
merged 1 commit into from
Jul 8, 2021
Merged

Prefer n-ary over unary combinators where possible #308

merged 1 commit into from
Jul 8, 2021

Conversation

basil-conto
Copy link
Collaborator

This is a more focused and efficient rewrite of PR #72 in order to address issue #306.

Cc: @vapniks

@basil-conto
Copy link
Collaborator Author

basil-conto commented Aug 29, 2019

I have marked this PR as a draft because I have not yet got around to auditing relevant docs and tests.

I have also benchmarked -on but not the other combinators, and am not yet convinced trading generality for performance, as was suggested in #72, is worth it.

@basil-conto
Copy link
Collaborator Author

I have also benchmarked -on but not the other combinators, and am not yet convinced trading generality for performance, as was suggested in #72, is worth it.

Indeed I am now of the opinion that generality in these combinators is a more desirable trait than maintaining identical performance with their previous versions; see below.

Here are some benchmarks inspired by #72 that use -on as an example:

;;; on.el --- benchmarks for -on -*- lexical-binding: t -*-

(defun on-binary (op trans)
  (lambda (x y)
    (funcall op (funcall trans x) (funcall trans y))))

(defun on-biniadic (op trans)
  (lambda (x y &rest args)
    (if args
        (apply op (funcall trans x) (funcall trans y)
               (mapcar trans args))
      (funcall op (funcall trans x) (funcall trans y)))))

(defun on-binariadic (op trans)
  (lambda (x y &rest args)
    (apply op (mapcar trans `(,x ,y ,@args)))))

(defun on-variadic (op trans)
  (lambda (&rest args)
    (apply op (mapcar trans args))))

(dolist (on '(on-binary on-binary on-biniadic on-binariadic on-variadic))
  (let ((list (mapcar #'number-to-string (number-sequence 0 10000)))
        (cmp  (funcall on #'> #'string-to-number)))
    (garbage-collect)
    (apply #'message "%-14s %9.6f %2d %.3f" on
           (benchmark-run 100
             (sort list cmp)))))

;;; on.el ends here

Running them in the following Emacs version:

In GNU Emacs 27.0.50 (build 1, x86_64-pc-linux-gnu, X toolkit, Xaw3d scroll bars)
 of 2019-08-27 built on thunk
Repository revision: 4118297ae2fab4886b20d193ba511a229637aea3
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12004000
System Description: Debian GNU/Linux bullseye/sid

Configured using:
 'configure 'CC=ccache gcc' 'CFLAGS=-O2 -march=native' --config-cache
 --prefix=/home/blc/.local --with-mailutils --with-x-toolkit=lucid
 --with-modules --with-file-notification=yes --with-x'

Configured features:
XAW3D XPM JPEG TIFF GIF PNG RSVG SOUND GPM DBUS GSETTINGS GLIB NOTIFY
INOTIFY ACL LIBSELINUX GNUTLS LIBXML2 FREETYPE HARFBUZZ M17N_FLT LIBOTF
XFT ZLIB TOOLKIT_SCROLL_BARS LUCID X11 XDBE XIM MODULES THREADS
LIBSYSTEMD JSON PDUMPER LCMS2 GMP

Gives the following results:

emacs -batch -f batch-byte-compile on.el && emacs -script on.elc
on-binary       0.027191  0 0.000
on-binary       0.015568  0 0.000
on-biniadic     0.015991  0 0.000
on-binariadic   0.065449  5 0.040
on-variadic     0.063849  5 0.040

The extra consing involved in the variadic implementations clearly slows down the returned function.

One solution would be to go with the implementation of on-biniadic, in which the &rest arglist is only allocated if the returned function is called with more than two arguments. This implementation exhibits the same performance as the original on-binary implementation. This is a win-win, because it improves on the current implementation without sacrificing performance.

My argument, however, is that we ought to go with on-variadic, despite the performance hit. Here is an outline of my reasoning:

  • The performance of &rest args is inherent to Elisp and neither specific to nor the responsibility of the Dash library. Until Emacs core is further optimised, everyone must eventually pay this price.
  • dash-functional is a library providing higher order combinators for convenience.
    • Such functions are inherently prone to lower performance, especially in a language like Elisp.
    • Performance-critical Elisp shouldn't be using these combinators to begin with.
    • The purpose of such functions is to strive for greater expressiveness, composability, conciceness, elegance, etc.
      • Restricting the calling conventions of such functions goes against all of these goals.
  • The benchmark above shows very reasonable performance for repeatedly sorting a large list, even if slower than the original implementation. I think we can live with this.

WDYT?

@basil-conto
Copy link
Collaborator Author

I intend to mark this PR as ready to review after the issues that PR #309 addresses are fixed, so that this branch can be rebased.

@vapniks
Copy link

vapniks commented Aug 30, 2019

I have spent the past few weeks learning txr lisp which has it's own (n-ary) versions of these combinators which I found useful. I wrote some txr code for comparing function runtimes and simple code complexity measures (maxdepth, number of symbols, etc), and was thinking that these measures might also be useful for elsa.

In principle there ought to be some empirical way of determining the best trade off between runtime & complexity under different situations, e.g. by doing some data analysis on large amounts of lisp code pulled from github, but as it stands I agree with the prognosis of @basil-conto

@basil-conto
Copy link
Collaborator Author

I have marked this PR as a draft because I have not yet got around to auditing relevant docs and tests.

I intend to mark this PR as ready to review after the issues that PR #309 addresses are fixed, so that this branch can be rebased.

I have now adapted the docs and tests, and have rebased the branch on PR #309 so that the tests can pass on all Emacs versions. Ideally if that PR is merged I will then mark this PR as ready to review.

@basil-conto basil-conto marked this pull request as ready for review September 19, 2019 15:45
@basil-conto
Copy link
Collaborator Author

I intend to mark this PR as ready to review after the issues that PR #309 addresses are fixed, so that this branch can be rebased.

I have now rebased this branch on top of the merged #309 and have marked this PR as ready for review.

@basil-conto basil-conto self-assigned this Dec 18, 2020
@basil-conto basil-conto added the enhancement Suggestion to improve or extend existing behavior label Dec 18, 2020
@basil-conto
Copy link
Collaborator Author

My argument, however, is that we ought to go with on-variadic, despite the performance hit.

Of course, it's possible to optimise on-variadic for the two argument case, to avoid the mapcar+apply overhead, if desired. ;)

I think I'll wait until after #356 is addressed to implement that, though.

README.md Outdated
(-filter (-not (-partial '< 4)) '(1 2 3 4 5 6 7 8)) ;; => '(1 2 3 4)
(funcall (-not #'even?) 5) ;; => t
(-filter (-not (-partial #'< 4)) '(1 2 3 4 5 6 7 8)) ;; => '(1 2 3 4)
(funcall (-not #'>) 1 2) ;; => t
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just tried something like this yesterday and was sad it doesn't work with a binary predicate. Definitely a welcome change.

@Fuco1
Copy link
Collaborator

Fuco1 commented Jan 10, 2021

I agree completely that the performance here is secondary. First come the users and then we can optimize what is reported to be slow. This has so far always turned out to be the correct way forward.

@basil-conto basil-conto added this to the 2.19.0 milestone Feb 15, 2021
@basil-conto basil-conto added this to In Progress in Release 2.19.0 Mar 9, 2021
@basil-conto basil-conto moved this from In Progress to Blocking in Release 2.19.0 Jun 29, 2021
@basil-conto basil-conto mentioned this pull request Jun 29, 2021
@basil-conto basil-conto moved this from Blocking to To Do in Release 2.19.0 Jun 29, 2021
@basil-conto basil-conto moved this from To Do to In Progress in Release 2.19.0 Jul 6, 2021
This is a rewrite and extension of PR #72 to address issue #306.
For discussion, especially wrt performance, see PRs #72 and #308.

* dash.el (-on, -flip, -not, -orfn, -andfn): Return a variadic
function.  Declare as pure and side-effect-free.
(-rotate-args): New combinator suggested by @vapniks in PR #72.
(-const): Declare as pure and side-effect-free.

* NEWS.md (2.19.0): Announce -rotate-args and variadic combinators.

* dev/examples.el (-partition-after-pred): Fix oddp bug waiting to
happen with negative dividends.
(-cons*): Check that &rest args are safe to mutate.
(-on, -flip, -const, -not, -orfn, -andfn): Extend tests.
(-rotate-args): New test.

* README.md:
* dash.texi: Regenerate docs.
@basil-conto basil-conto merged commit 350b4c3 into magnars:master Jul 8, 2021
@basil-conto basil-conto moved this from In Progress to Done in Release 2.19.0 Jul 8, 2021
@basil-conto basil-conto linked an issue Jul 8, 2021 that may be closed by this pull request
@basil-conto basil-conto deleted the blc/variadic branch July 8, 2021 21:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Suggestion to improve or extend existing behavior
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

adding orElse for partial functions
3 participants