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

fetchzip: cleanup and improve metrics #247527

Merged
2 commits merged into from Aug 23, 2023
Merged

fetchzip: cleanup and improve metrics #247527

2 commits merged into from Aug 23, 2023

Conversation

oxij
Copy link
Member

@oxij oxij commented Aug 6, 2023

This cleans up fetchzip and fetchurl derivations a bit, improving Nixpkgs metrics.

fetchzip seems to change semantics with me removing arguments there, but it does not, since all that pname logic was ignored by underlying fetchurl anyway.

pkgs/build-support/fetchzip/default.nix Outdated Show resolved Hide resolved
pkgs/build-support/fetchzip/default.nix Show resolved Hide resolved
pkgs/build-support/fetchzip/default.nix Outdated Show resolved Hide resolved
@oxij
Copy link
Member Author

oxij commented Aug 8, 2023

@amjoseph-nixpkgs

.. although the eval performance report shows only ~0.1% improvement on any metric other than cpu time (which is a very noisy metric)...

Yes, this whole thing was a bit disheartening for me. Changes that make huge improvements to local eval produce negative results on OfBorg and vice versa.

@oxij
Copy link
Member Author

oxij commented Aug 8, 2023

Okay, so, apparently OfBorg's gc-heapSize in evaluation result is just very noisy. Applying 9437e4d, which is basically a noop (since the condition is never true), improved everything, except gc-heapSize, which got worse. It is also noisy on my machine, but not that noisy. But I'm not evaluating everything, my evaluations usually take 14GiB of RAM, OfBorg's ~41GiB. So, it makes sense it's about 4x as noisy.

Compare:
https://github.com/NixOS/nixpkgs/runs/15715151966
https://github.com/NixOS/nixpkgs/runs/15717732275

So, this is done, I think.

@oxij
Copy link
Member Author

oxij commented Aug 11, 2023

ping @SuperSandro2000 @hraban who edited pname logic before.

Note how fetchurl already does this part in https://github.com/NixOS/nixpkgs/blob/master/pkgs/build-support/fetchurl/default.nix#L135 completely ignoring the name in fetchzip when pname and version are set, so there's no need to duplicate that piece of code.

''

# Remove non-owner write permissions
# Fixes https://github.com/NixOS/nixpkgs/issues/38649
Copy link
Contributor

Choose a reason for hiding this comment

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

why did you remove this comment? Is it obsolete?

Copy link
Member Author

Choose a reason for hiding this comment

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

I did not, I moved it down below.

Copy link
Contributor

Choose a reason for hiding this comment

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

Gotcha. So this is all moved around as an optimization, if I understand your other comments--Is removing these string literal concatenations actually a noticeable improvement? Seems like a pretty basic optimization step for nix to take, I'm surprised this is noticeable :/ what's the speedup?

Copy link
Member Author

Choose a reason for hiding this comment

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

There's probably no speedup there, but it does take less allocations.

@ghost ghost merged commit be547cb into NixOS:master Aug 23, 2023
17 checks passed
@oxij
Copy link
Member Author

oxij commented Aug 26, 2023

Thanks @amjoseph-nixpkgs!

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants