Re: [PATCH] emacs: Rewrite scheme side in a functional manner.
Alex Kost skribis: > Ludovic Courtès (2014-09-21 23:27 +0400) wrote: > >> Alex Kost skribis: >> >>> Ludovic Courtès (2014-09-20 18:11 +0400) wrote: [...] >> I was thinking about things generic enough to be in (guix profiles), >> things that ‘guix package’ or guix-web could use. >> >> That said, it’s also fine do to things incrementally: write things >> specifically for guix.el’s need, and generalize them later when we have >> a clearer understanding of the situation. I just thought it’s worth >> keeping in mind. > > Thanks, I keep it in mind. Such incremental approach is the only way I > can write code. I'm not able to construct a proper thing from the very > beginning: I always make big changes when something new is added and I > notice that a new wave of generalization is required. Understandably, that’s pretty much the same for me. [...] >>> To get information about packages/outputs, different “search-types” are >>> used (like ‘name’, ‘all-available’, ‘installed’). These “search-types + >>> search-vals” are transformed into a list of package/output patterns. >>> >>> Package patterns are: >>> >>> - package record; >>> - list of name, version; >>> - list of name, version, manifest entries; >>> - list of name, version, manifest entries, packages. >> >> Oh, OK. Do remove any ambiguity, an option would be to call them >> “matches”, “search results”, “descriptors”, or “specifications”, >> perhaps. WDYT? >> >> (Maybe this is just bikeshedding, so your call.) > > I leave a “pattern” name for now. OK. >>> Output patterns are: >>> >>> - package record; >>> - list of package record, output; >>> - manifest entry record; >>> - list of manifest entry record, packages; >>> - list of name, version, output. >>> >>> Then these patterns are transformed into entries (alists with >>> parameters/values) using “pattern->entries” functions created by >>> ‘package-pattern-transformer’ and ‘output-pattern-transformer’. >> >> What about s/entries/sexps/? That would make it clearer that the intent >> is to serialize things, not to manipulate them internally. > > Yes, it is better, thanks. I settled to “sexp” for this thing. OK. > Thanks for all your recommendations, I tried to follow them. > I'm attaching the improved patch. Is it good enough now? Yes, please push. Thanks for taking the time! Ludo’.
Re: [PATCH] emacs: Rewrite scheme side in a functional manner.
Ludovic Courtès (2014-09-21 23:27 +0400) wrote: > Alex Kost skribis: > >> Ludovic Courtès (2014-09-20 18:11 +0400) wrote: [...] >>> Overall it looks OK. I’m concerned with code duplication between emacs/ >>> and guix/, though, and there are also a few stylistic issues, and >>> missing or terse docstrings. >> >> I don't understand what duplication you mean. > > I was thinking about things generic enough to be in (guix profiles), > things that ‘guix package’ or guix-web could use. > > That said, it’s also fine do to things incrementally: write things > specifically for guix.el’s need, and generalize them later when we have > a clearer understanding of the situation. I just thought it’s worth > keeping in mind. Thanks, I keep it in mind. Such incremental approach is the only way I can write code. I'm not able to construct a proper thing from the very beginning: I always make big changes when something new is added and I notice that a new wave of generalization is required. [...] >> We discussed using vhash, but I don't see how it can replace hash >> table. Here is the excerpt from the earlier message: > > Right, I remember that. My point was just that either this optimization > is not strictly needed, or it could go in (guix profiles), whether it > uses a vhash or a hash table actually. > > But I think we actually agree on that, so that can still be addressed at > a later stage. Thanks. [...] >> To get information about packages/outputs, different “search-types” are >> used (like ‘name’, ‘all-available’, ‘installed’). These “search-types + >> search-vals” are transformed into a list of package/output patterns. >> >> Package patterns are: >> >> - package record; >> - list of name, version; >> - list of name, version, manifest entries; >> - list of name, version, manifest entries, packages. > > Oh, OK. Do remove any ambiguity, an option would be to call them > “matches”, “search results”, “descriptors”, or “specifications”, > perhaps. WDYT? > > (Maybe this is just bikeshedding, so your call.) I leave a “pattern” name for now. >> Output patterns are: >> >> - package record; >> - list of package record, output; >> - manifest entry record; >> - list of manifest entry record, packages; >> - list of name, version, output. >> >> Then these patterns are transformed into entries (alists with >> parameters/values) using “pattern->entries” functions created by >> ‘package-pattern-transformer’ and ‘output-pattern-transformer’. > > What about s/entries/sexps/? That would make it clearer that the intent > is to serialize things, not to manipulate them internally. Yes, it is better, thanks. I settled to “sexp” for this thing. +(define (get-package/output-entries profile params entry-type +search-type search-vals) + "Return list of package or output entries." >>> >>> Never ‘get’. >> >> Do you mean I need to remove "get-" from the procedures' names? > > Yes. Done. [...] >>> The docstring is too terse. >> >> The concept of "entry" is too common for the code in “guix-main.scm” to >> write about it in every docstring. > > Yes, but still: there are 5 parameters. I can tell what ‘profile’ is, > but I have no clear idea about the type and meaning of the others at > first glance. Presumably ‘search-type’ and ‘entry-type’ are symbols, > but then that still leaves a lot to be found out. I improved some docstrings. [...] Thanks for all your recommendations, I tried to follow them. I'm attaching the improved patch. Is it good enough now? >From f93af8a3a0b4d119a0e7ff23ce81bc7b28a31b60 Mon Sep 17 00:00:00 2001 From: Alex Kost Date: Thu, 18 Sep 2014 16:24:02 +0400 Subject: [PATCH 1/2] emacs: Rewrite scheme side in a functional manner. * emacs/guix-main.scm: Rewrite in a functional way. Add support for output entries. (%current-manifest, %current-manifest-entries-table, set-current-manifest-maybe!): Replace with... (manifest-entries->hash-table, manifest->hash-table): ... this. (manifest-entries-by-name+version): Replace with... (manifest-entries-by-name): ... this. (fold-manifest-entries): Rename to... (fold-manifest-by-name): ... this. (package-installed-param-alist): Rename to... (%manifest-entry-param-alist): ... this. (package-param-alist): Rename to... (%package-param-alist): this. (manifest-entry->installed-entry): Rename to... (manifest-entry->sexp): ... this. (manifest-entries->installed-entries): Rename to... (manifest-entries->sexps): ... this. (matching-generation-entries): Replace with... (matching-generations): ... this. (last-generation-entries): Replace with... (last-generations): ... this. (get-entries): Rename to... (entries): ... this. (installed-entries-by-name+version, installed-entries-by-package, matching-package-entries, fold-object, package-entries-by-name+version, package-entries-by-spec, package-entries-by-regexp, package-entries-by-ids, newest-available-package-entries, all-
Re: [PATCH] emacs: Rewrite scheme side in a functional manner.
Alex Kost skribis: > Ludovic Courtès (2014-09-20 18:11 +0400) wrote: [...] >> There are still a bunch of ‘set!’ and hash tables, though. ;-) > > There are only 2 ‘set!’s in: > > (define manifest->hash-table > (let ((current-manifest #f) > (current-table #f)) > (lambda (manifest) > "Return hash table of name keys and lists of matching MANIFEST entries." > (unless (manifest=? manifest current-manifest) > (set! current-manifest manifest) > (set! current-table (mentries->hash-table > (manifest-entries manifest > current-table))) > > and I think they are unavoidable. > > As for the hash tables, I use them because I don't see how they can be > removed without a loss in efficiency. In theory they could be hidden behind something like ‘memoize’, but yeah, that’s fine. >> Overall it looks OK. I’m concerned with code duplication between emacs/ >> and guix/, though, and there are also a few stylistic issues, and >> missing or terse docstrings. > > I don't understand what duplication you mean. I was thinking about things generic enough to be in (guix profiles), things that ‘guix package’ or guix-web could use. That said, it’s also fine do to things incrementally: write things specifically for guix.el’s need, and generalize them later when we have a clearer understanding of the situation. I just thought it’s worth keeping in mind. > I didn't write much docstrings (but I'll add more), because: > > - Most functions are trivial and self-descriptive by their names. > - All functions are used only inside “guix-main.scm” anyway. Fair enough. I don’t completely buy the “self-descriptive by their names” thing, but okay, no big deal. >> Some remarks: >> >>> +(define (mentries->hash-table mentries) >> >> For consistency I’d make it ‘manifest-entries->hash-table entries’. > > OK, I'll rename "mentries" to "manifest-entries" in the procedures' > names, but I leave "mentry"/"mentries" for the local variables if you > don't mind. > > The thing is, I use the term "entry" to name an alist of > package/output/generation parameters that is passed to the elisp side. > It looks like this: > > ((name . "guile") > (version . "2.0.11") > (outputs "out" "debug") > ...) > > So I shortened "manifest-entry" to "mentry" to distinguish these > entities. Oh, OK. >>> +(define manifest->hash-table >>> + (let ((current-manifest #f) >>> +(current-table #f)) >>> +(lambda (manifest) >>> + "Return hash table of name keys and lists of matching MANIFEST >>> entries." >>> + (unless (manifest=? manifest current-manifest) >>> +(set! current-manifest manifest) >>> +(set! current-table (mentries->hash-table >>> + (manifest-entries manifest >>> + current-table))) >> >> What about: >> >> (define manifest->hash-table >> (memoize >> (compose manifest-entries->hash-table >>manifest-entries))) > > I should have written that I tried to use ‘memoize’ there but it was > significantly slower. I tried a test manifest of 1200 entries and on my > (very slow) computer it took in average: > > - about 3 seconds for the variant I suggest now; > - about 15 seconds for the variant with ‘memoize’ > > to get a list buffer with all packages. > > I think that happens because hash table procedures in ‘memoize’ use > ‘equal?’ to compare arguments. And ‘equal?’-ing big manifests for every > package is slow. That's why I made ‘manifest?=’ with ‘eq?’ at first > place. > > Here is what happens when information about all packages is "requested": > we need to check every package if it's installed or not, i.e. to lookup > the same manifest for each package. I don't see a better way then using > an additional hash table. Indeed, I stand corrected. >> But honestly I think this is premature optimization (I mean, there are >> 177 packages in my profile, so 10,000 ;-)), and should that optimization >> be needed, it should be done transparently in (guix profiles), as we >> discussed some time ago. > > I think this optimization is premature for putting it into (guix > profiles) but I believe it is necessary for the special needs of > "guix.el". OK, I see now. > Sorry, I don't understand what «10,000» means. Sorry, typo: I meant there’s much less than 10 thousands packages in my profile. > We discussed using vhash, but I don't see how it can replace hash > table. Here is the excerpt from the earlier message: Right, I remember that. My point was just that either this optimization is not strictly needed, or it could go in (guix profiles), whether it uses a vhash or a hash table actually. But I think we actually agree on that, so that can still be addressed at a later stage. >>> +(define* (mentries-by-name manifest name #:optional version output) >>> + "Return list of MANIFEST entries matching NAME, VERSION and OUTPUT." >>> + (let ((mentries (or (hash-ref (manifest->hash-table manifest)
Re: [PATCH] emacs: Rewrite scheme side in a functional manner.
Ludovic Courtès (2014-09-20 18:11 +0400) wrote: > Alex Kost skribis: > >> From d42829fe03271e633e43cc35cf277705203e6080 Mon Sep 17 00:00:00 2001 >> From: Alex Kost >> Date: Thu, 18 Sep 2014 16:24:02 +0400 >> Subject: [PATCH 2/3] emacs: Rewrite scheme side in a functional manner. > > Good idea! It was your idea actually :-) > There are still a bunch of ‘set!’ and hash tables, though. ;-) There are only 2 ‘set!’s in: --8<---cut here---start->8--- (define manifest->hash-table (let ((current-manifest #f) (current-table #f)) (lambda (manifest) "Return hash table of name keys and lists of matching MANIFEST entries." (unless (manifest=? manifest current-manifest) (set! current-manifest manifest) (set! current-table (mentries->hash-table (manifest-entries manifest current-table))) --8<---cut here---end--->8--- and I think they are unavoidable. As for the hash tables, I use them because I don't see how they can be removed without a loss in efficiency. > Overall it looks OK. I’m concerned with code duplication between emacs/ > and guix/, though, and there are also a few stylistic issues, and > missing or terse docstrings. I don't understand what duplication you mean. I didn't write much docstrings (but I'll add more), because: - Most functions are trivial and self-descriptive by their names. - All functions are used only inside “guix-main.scm” anyway. > Some remarks: > >> +(define (mentries->hash-table mentries) > > For consistency I’d make it ‘manifest-entries->hash-table entries’. OK, I'll rename "mentries" to "manifest-entries" in the procedures' names, but I leave "mentry"/"mentries" for the local variables if you don't mind. The thing is, I use the term "entry" to name an alist of package/output/generation parameters that is passed to the elisp side. It looks like this: ((name . "guile") (version . "2.0.11") (outputs "out" "debug") ...) So I shortened "manifest-entry" to "mentry" to distinguish these entities. >> +(define manifest->hash-table >> + (let ((current-manifest #f) >> +(current-table #f)) >> +(lambda (manifest) >> + "Return hash table of name keys and lists of matching MANIFEST >> entries." >> + (unless (manifest=? manifest current-manifest) >> +(set! current-manifest manifest) >> +(set! current-table (mentries->hash-table >> + (manifest-entries manifest >> + current-table))) > > What about: > > (define manifest->hash-table > (memoize > (compose manifest-entries->hash-table >manifest-entries))) I should have written that I tried to use ‘memoize’ there but it was significantly slower. I tried a test manifest of 1200 entries and on my (very slow) computer it took in average: - about 3 seconds for the variant I suggest now; - about 15 seconds for the variant with ‘memoize’ to get a list buffer with all packages. I think that happens because hash table procedures in ‘memoize’ use ‘equal?’ to compare arguments. And ‘equal?’-ing big manifests for every package is slow. That's why I made ‘manifest?=’ with ‘eq?’ at first place. Here is what happens when information about all packages is "requested": we need to check every package if it's installed or not, i.e. to lookup the same manifest for each package. I don't see a better way then using an additional hash table. > But honestly I think this is premature optimization (I mean, there are > 177 packages in my profile, so 10,000 ;-)), and should that optimization > be needed, it should be done transparently in (guix profiles), as we > discussed some time ago. I think this optimization is premature for putting it into (guix profiles) but I believe it is necessary for the special needs of "guix.el". Sorry, I don't understand what «10,000» means. We discussed using vhash, but I don't see how it can replace hash table. Here is the excerpt from the earlier message: Ludovic Courtès (2014-09-03 11:09 +0400) wrote: > Alex Kost skribis: > >> Ludovic Courtès (2014-09-01 16:10 +0400) wrote: > > [...] > >>> Would ‘vlist-fold’ work? >>> >>> scheme@(guile-user)> (vhash-cons 'a 1 (vhash-cons 'b 2 (vhash-cons 'a 3 >>> vlist-null))) >>> $2 = # >>> scheme@(guile-user)> (vlist-fold cons '() $2) >>> $3 = ((a . 3) (b . 2) (a . 1)) >> >> Sorry, I don't see how it could work. Here is an example: >> >> ;; What I currently have is a hash-table like this one: >> (define table (make-hash-table 3)) >> >> (hash-set! table 'a '(1 2 3)) >> (hash-set! table 'b '(4)) >> (hash-set! table 'c '(5 6)) >> >> ;; And I can easily fold through unique keys like this: >> (hash-fold (lambda (key entries res) >> (cons (cons key (apply + entries)) res)) >>'() >>table) ; => ((c . 11) (b . 4) (a . 6)) >> >> ;; Wh
Re: [PATCH] emacs: Rewrite scheme side in a functional manner.
Alex Kost skribis: > From d42829fe03271e633e43cc35cf277705203e6080 Mon Sep 17 00:00:00 2001 > From: Alex Kost > Date: Thu, 18 Sep 2014 16:24:02 +0400 > Subject: [PATCH 2/3] emacs: Rewrite scheme side in a functional manner. Good idea! There are still a bunch of ‘set!’ and hash tables, though. ;-) Overall it looks OK. I’m concerned with code duplication between emacs/ and guix/, though, and there are also a few stylistic issues, and missing or terse docstrings. Some remarks: > +(define (mentries->hash-table mentries) For consistency I’d make it ‘manifest-entries->hash-table entries’. > +(define manifest->hash-table > + (let ((current-manifest #f) > +(current-table #f)) > +(lambda (manifest) > + "Return hash table of name keys and lists of matching MANIFEST > entries." > + (unless (manifest=? manifest current-manifest) > +(set! current-manifest manifest) > +(set! current-table (mentries->hash-table > + (manifest-entries manifest > + current-table))) What about: (define manifest->hash-table (memoize (compose manifest-entries->hash-table manifest-entries))) But honestly I think this is premature optimization (I mean, there are 177 packages in my profile, so 10,000 ;-)), and should that optimization be needed, it should be done transparently in (guix profiles), as we discussed some time ago. > +(define* (mentries-by-name manifest name #:optional version output) > + "Return list of MANIFEST entries matching NAME, VERSION and OUTPUT." > + (let ((mentries (or (hash-ref (manifest->hash-table manifest) name) > + '( > +(if (or version output) > +(filter (lambda (mentry) > + (and (or (not version) > + (equal? version (manifest-entry-version mentry))) > + (or (not output) > + (equal? output (manifest-entry-output mentry) > +mentries) > +mentries))) What about using ‘manifest-lookup’ instead? > +(define (mentry-by-output mentries output) > + (find (lambda (mentry) > + (string= output (manifest-entry-output mentry))) > +mentries)) Likewise. > (define* (object-transformer param-alist #:optional (params '())) > - "Return function for transforming an object into alist of > parameters/values. > + "Return function for transforming objects into alist of parameters/values. “Return a procedure transforming an object into an list of parameter/value pairs.” > -(lambda (object) > +(lambda objects >(map (match-lambda > ((param . fun) > - (cons param (fun object > + (cons param (apply fun objects > alist s/fun/proc/ (yeah, this is Scheme. ;-)) May be worth considering moving it to (guix records), which already has ‘alist->record’. > +(define (spec->package-pattern spec) > + (call-with-values > + (lambda () (full-name->name+version spec)) > +list)) s/spec/specification/g However, the result is not a “package pattern”, right? I find the name a bit confusing. Is there any reason to use lists instead of multiple values? > +(define (manifest-package-patterns manifest) > + "Return list of package patterns for all MANIFEST entries." > + (fold-manifest-by-name manifest > + (lambda (name version mentries res) > + (cons (list name version mentries) res)) > + '())) Likewise I’m unclear why this “package pattern” abstraction is needed here. Actually, is it just for serialization between Guile and Emacs? If that is the case, then ‘package->sexp’ would seem more appropriate. > +(define (package-pattern-transformer manifest params) > + "Return 'package-pattern->package-entries' function." Damn, what does this mean? :-) > +(define (get-package/output-entries profile params entry-type > +search-type search-vals) > + "Return list of package or output entries." Never ‘get’. The docstring is too terse. > +;;; XXX move to (guix profiles) ? > (define (profile-generations profile) Definitely worth moving there (in a separate patch.) Thanks, Ludo’.
