[bug#68266,7/7] packages: rust: Memoize make-rust-sysroot results.

Hi,

Christopher Baines <mail@cbaines.net> skribis:

> To ensure that it just returns a single package record for some given
> arguments, as this helps to avoid poor performance of the store connection
> object cache.
>
> * gnu/packages/rust.scm (make-rust-sysroot): Move code to
> make-rust-sysroot/implementation.
> (make-rust-sysroot/implementation): New variable.
>
> Change-Id: Ibb30c7398328c87c032bb8828635a34ada935167

[...]

>  (define*-public (make-rust-sysroot target)
> -  (let ((base-rust rust))
> +  (make-rust-sysroot/implementation target rust))
> +
> +(define make-rust-sysroot/implementation
> +  (mlambda (target base-rust)
>      (package
>        (inherit base-rust)
>        (name (string-append "rust-sysroot-for-" target))

We should avoid using ‘mlambda’ (without ‘q’) with packages as it leads
to deep object comparisons.  That’s why for packages we typically
always have one-argument (mlambdaq (package) …).

But since ‘base-rust’ wasn’t a parameter before, let’s keep it simple
(‘diff --ignore-space-change’):
It should help when cross-compiling several ‘cargo-build-system’ packages:

--8<---------------cut here---------------start------------->8---
$ time GUIX_PROFILING=object-cache guix build --no-grafts greetd wlgreet du-dust circtools --target=aarch64-linux-gnu -d # before
/gnu/store/mivzv83wryv9gp5bjncg5m1831dx2xwr-circtools-1.0.0.drv
/gnu/store/4xf7kh9mi0vpvs8m1ak4x8w1rpsdpv6z-du-dust-0.8.6.drv
/gnu/store/ayk54gvlbc1qam6irzf9kaig56dhzni0-wlgreet-0.4.1.drv
/gnu/store/r601i40cii9ic5w1k4hy5c2yngfayh64-greetd-0.9.0.drv
Object Cache:
  fresh caches:    22
  lookups:      40435
  hits:         36821 (91.1%)
  cache size:    3613 entries

real	0m2.964s
user	0m2.925s
sys	0m0.174s
$ time GUIX_PROFILING=object-cache ./pre-inst-env guix build --no-grafts greetd wlgreet du-dust circtools --target=aarch64-linux-gnu -d # after
/gnu/store/1wsldmvigjb8w2gk418npbnfznlb0ck1-circtools-1.0.0.drv
/gnu/store/b5c73fawjdvkgy431qxz9l6l9y9a9lhz-du-dust-0.8.6.drv
/gnu/store/zwc7qzsbzf62dgbbzy74lki4hsr406bw-wlgreet-0.4.1.drv
/gnu/store/vjdd23hc82701afb132z1ajcqa7hfd74-greetd-0.9.0.drv
Object Cache:
  fresh caches:    22
  lookups:      37942
  hits:         34523 (91.0%)
  cache size:    3418 entries

real	0m2.980s
user	0m3.300s
sys	0m0.160s
--8<---------------cut here---------------end--------------->8---

(Here we removed ~2.5k nodes from the cache.)

WDYT?

Ludo’.

Ludovic Courtès <ludo@gnu.org> writes:

> Christopher Baines <mail@cbaines.net> skribis:
>
>> To ensure that it just returns a single package record for some given
>> arguments, as this helps to avoid poor performance of the store connection
>> object cache.
>>
>> * gnu/packages/rust.scm (make-rust-sysroot): Move code to
>> make-rust-sysroot/implementation.
>> (make-rust-sysroot/implementation): New variable.
>>
>> Change-Id: Ibb30c7398328c87c032bb8828635a34ada935167
>
> [...]
>
>>  (define*-public (make-rust-sysroot target)
>> -  (let ((base-rust rust))
>> +  (make-rust-sysroot/implementation target rust))
>> +
>> +(define make-rust-sysroot/implementation
>> +  (mlambda (target base-rust)
>>      (package
>>        (inherit base-rust)
>>        (name (string-append "rust-sysroot-for-" target))
>
> We should avoid using ‘mlambda’ (without ‘q’) with packages as it leads
> to deep object comparisons.  That’s why for packages we typically
> always have one-argument (mlambdaq (package) …).
>
> But since ‘base-rust’ wasn’t a parameter before, let’s keep it simple
> (‘diff --ignore-space-change’):

...

> WDYT?

Yeah, that does look good. I pushed my earlier version of this patch
this morning though.

I did have a look at trying to adapt the changes to fit in (guix
build-system cargo) instead, as I noticed that seemed to be a pattern
elsewhere, but I think there's something weird going on with the use of
make-rust-sysroot there since default-rust-sysroot takes an argument,
but doesn't use it. Maybe once that's figured out, we can move the
memoization there and switch to just using the target as the key.

Unfortunately I'm still waiting to see what effect this has on the data
service processing revisions. I'm pretty sure it's going to help, but
I'm concerned it's not going to help enough to make processing revisions
for patches feasible again.

On Fri, Jan 12, 2024 at 05:57:26PM +0000, Christopher Baines wrote:
> 
> Ludovic Courtès <ludo@gnu.org> writes:
> 
> > Christopher Baines <mail@cbaines.net> skribis:
> >
> >> To ensure that it just returns a single package record for some given
> >> arguments, as this helps to avoid poor performance of the store connection
> >> object cache.
> >>
> >> * gnu/packages/rust.scm (make-rust-sysroot): Move code to
> >> make-rust-sysroot/implementation.
> >> (make-rust-sysroot/implementation): New variable.
> >>
> >> Change-Id: Ibb30c7398328c87c032bb8828635a34ada935167
> >
> > [...]
> >
> >>  (define*-public (make-rust-sysroot target)
> >> -  (let ((base-rust rust))
> >> +  (make-rust-sysroot/implementation target rust))
> >> +
> >> +(define make-rust-sysroot/implementation
> >> +  (mlambda (target base-rust)
> >>      (package
> >>        (inherit base-rust)
> >>        (name (string-append "rust-sysroot-for-" target))
> >
> > We should avoid using ‘mlambda’ (without ‘q’) with packages as it leads
> > to deep object comparisons.  That’s why for packages we typically
> > always have one-argument (mlambdaq (package) …).
> >
> > But since ‘base-rust’ wasn’t a parameter before, let’s keep it simple
> > (‘diff --ignore-space-change’):
> 
> ...
> 
> > WDYT?
> 
> Yeah, that does look good. I pushed my earlier version of this patch
> this morning though.
> 
> I did have a look at trying to adapt the changes to fit in (guix
> build-system cargo) instead, as I noticed that seemed to be a pattern
> elsewhere, but I think there's something weird going on with the use of
> make-rust-sysroot there since default-rust-sysroot takes an argument,
> but doesn't use it. Maybe once that's figured out, we can move the
> memoization there and switch to just using the target as the key.
> 
> Unfortunately I'm still waiting to see what effect this has on the data
> service processing revisions. I'm pretty sure it's going to help, but
> I'm concerned it's not going to help enough to make processing revisions
> for patches feasible again.

I looked at the build system a bit and I think it was a combination of
cargo-culting the other cross build implementations that took a target
argument for the cross-compilers and I figured that rust-sysroot would
also need one. The other bit was I think I had in mind the possibility
of choosing seemingly arbitrary targets which were supported by rust but
not known to Guix and having it possible to cross-compile to those.
Given that rust, like go, IIRC doesn't actually need a cross-compiled
compiler to build cross-compiled packages, could it be that parts of
that logic can be rewritten/simplified?

Hello,

Christopher Baines <mail@cbaines.net> skribis:

> Ludovic Courtès <ludo@gnu.org> writes:
>
>> Christopher Baines <mail@cbaines.net> skribis:

[...]

>>>  (define*-public (make-rust-sysroot target)
>>> -  (let ((base-rust rust))
>>> +  (make-rust-sysroot/implementation target rust))
>>> +
>>> +(define make-rust-sysroot/implementation
>>> +  (mlambda (target base-rust)
>>>      (package
>>>        (inherit base-rust)
>>>        (name (string-append "rust-sysroot-for-" target))
>>
>> We should avoid using ‘mlambda’ (without ‘q’) with packages as it leads
>> to deep object comparisons.  That’s why for packages we typically
>> always have one-argument (mlambdaq (package) …).
>>
>> But since ‘base-rust’ wasn’t a parameter before, let’s keep it simple
>> (‘diff --ignore-space-change’):
>
> ...
>
>> WDYT?
>
> Yeah, that does look good. I pushed my earlier version of this patch
> this morning though.
>
> I did have a look at trying to adapt the changes to fit in (guix
> build-system cargo) instead, as I noticed that seemed to be a pattern
> elsewhere, but I think there's something weird going on with the use of
> make-rust-sysroot there since default-rust-sysroot takes an argument,

What matters is that ‘make-rust-sysroot’ takes a single argument,
‘target’, so we can safely write:

  (define make-rust-sysroot
    (mlambda (target)
      …))

I think it’s important to not bring a deep <package> comparison because
this kind of cost is then hard to pinpoint.

Perhaps you can adjust ‘make-rust-sysroot’ along these lines?

Thanks,
Ludo’.