Re: [PATCH] emacs: Rewrite scheme side in a functional manner.

[ludo@gnu.org (Ludovic Courtès)]

Alex Kost <alezost@gmail.com> 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) 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?
>
> It is slower

In the first arm of ‘if’, it’s really the same, but yeah, the second arm
must be more common and it’s faster.

[...]

>> 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?  :-)
>
> Here is what that all means:
>
> 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.)

> 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.

>>> +(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.

> What about ‘get-entries’ (it is the entry point for the elisp side) –
> should it be just ‘entries’ then?

If it returns the entries of ‘profile’, it could be ‘profile-entries’.

>> 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.

(Replying to the rest separately.)

> P.S.  I've just reread this mail and found that it may look rather
> offensive.  Sorry, I didn't mean to do that.

No problem, I realize my message probably wasn’t great either.

I think we’re used to different coding conventions (elisp vs. Scheme),
so we are learning to understand each other.  :-)

Thanks for the good work, and for your patience!

Ludo’.