lib/strings: optimise hasInfix function

[danth]

Description of changes

Made the definition of hasInfix more efficient.

I measured the improvement with NIX_SHOW_STATS=1 NIX_COUNT_CALLS=1 for both the old and new definitions, searching for a 5 letter infix which was not present within a 10,000 character string. A summary of the results:

• 1.5× less CPU time
• 4× less thunks
• 2× less primop calls
• 5× less function calls
• 64× less total bytes allocated

Although the code is longer, I think this implementation is also easier to understand.

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• Tested basic functionality of all binary files (usually in ./result/bin/)
• 22.05 Release Notes (or backporting 21.11 Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
  • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
• Fits CONTRIBUTING.md.

[pennae]

hasInfix isn't called with very many different infixes in nixpkgs. might it make sense to use regexes instead of plucking strings apart over and over again?

[ajs124]

@pennae while builtins.match would save a lot of memory and probably be even faster, one issue is that lib.hasInfix is used outside of nixpkgs. So we'd somehow have to escape all regexes.

[pennae]

@ajs124 of course, escaping them was implied. we do have an escapeRegex in nixpkgs already, so that should be trivial.

[danth]

@pennae I've written up a second implementation using regexes.

In my isolated test, the effect on CPU time is roughly the same, although when applied to nixpkgs this will be greater if the compiled regex can be reused. Memory usage is much lower now.

[roberth]

if the compiled regex can be reused

Nix caches compiled regexes in a lookup table.
Like all(?) primops, it does not do anything before it's fully applied, meaning that we can't take advantage of currying, but that seems like an insignificant loss.

searching for a 5 letter infix which was not present within a 10,000 character string.

Is this a common use case?
I would expect it to be used on many short strings instead. This might lead to a somewhat different result.

[danth]

@roberth Is this a common use case?

I used this in Coricamu to check some HTML for the presence of a particular tag, hence I began with a test relevant to that. OfBorg's evaluation performance report confirms there is at least some benefit for Nixpkgs.

[ajs124]

So, how do we proceed here? Either approach seems worthwhile and better than what we currently have, but personally, I don't feel qualified to approve or disapprove of a specific implementation.

[pennae]

based on how this function is used in nixpkgs at this day the regex approach seems better. regex caching does mean that large numbers of infixes increase memory usage, costing about 900 bytes per infix. it seems to be a reasonable tradeoff. a single regex costs about 10µs to compile while a single short substring costs about 0.4µs. the cost of regex compilation is a one-time overhead, iterating over substrings is not.

would say the regex version is preferrable.

[tomberek]

In a coincidence I just encountered a need for lib.hasInfix and thought "hrm... i wonder if builtins.match can do better" and found this PR. Is there any reason not to merge?

Possible issues:

• inconsistent regex behavior: Regex behavior differs across platforms nix#1537
• evaluation with an old Nix. Looks like match has been around since 1.12, that should be fine

otherwise is seems to be used fairly infrequently. @DavHau: you do a lot of parsing, string manipulation, and usage of regex, thoughts?

[pennae]

• inconsistent regex behavior:

since the regex input is (should be) fully escaped in this PR there shouldn't be any behavioural differences. might be worth checking that escapeRegex correctly handles all metacharacters though

[DavHau]

otherwise is seems to be used fairly infrequently. @DavHau: you do a lot of parsing, string manipulation, and usage of regex, thoughts?

I use it on large amounts of small strings checking for a small number of infixes. I think a performance improvement would be great.

[danth]

regex caching does mean that large numbers of infixes increase memory usage

Although I'm searching over a small number of large strings, I too only use a handful of infixes. So this won't cause issues for me.

evaluation with an old Nix

Since the match function was added in 2014, I don't expect many people to still be using such old versions that it is not supported. If we need backwards compatibility, I would suggest falling back to this implementation, which is still faster than the original. However this adds complexity from wrapping the function with if builtins?match.

A small breaking change is that it's no longer possible to use hasInfix over a derivation (to search for an infix within its store path). There was only one case of this usage within Nixpkgs, which I fixed by only searching over the derivation name. For shorter infixes there is a chance the infix could appear within the store hash, so I think discouraging usage with derivations is a good thing, even if we bring back support by passing the input through toString.

[Izorkin]

This update breaks nixops:

/nix/store/qx3zja1j5nicw2n11xlj0dkl1d2gwbgw-python2.7-apache-libcloud-2.8.3/lib/python2.7/site-packages/libcloud/common/google.py:93: CryptographyDeprecationWarning: Python 2 is no longer supported by the Python core team. Support for it is now deprecated in cryptography, and will be removed in the next release.
  from cryptography.hazmat.backends import default_backend
trace: warning: The extraArgs argument to eval-config.nix is deprecated. Please set config._module.args instead.
trace: warning: The extraArgs argument to eval-config.nix is deprecated. Please set config._module.args instead.
trace: warning: The extraArgs argument to eval-config.nix is deprecated. Please set config._module.args instead.
building all machine configurations...
trace: warning: The extraArgs argument to eval-config.nix is deprecated. Please set config._module.args instead.
error: value is a set while a string was expected

       at /home/user/works/src-nix/nixpkgs/lib/strings.nix:256:5:

          255|   hasInfix = infix: content:
          256|     builtins.match ".*${escapeRegex infix}.*" content != null;
             |     ^
          257|
(use '--show-trace' to show detailed location information)
Traceback (most recent call last):
  File "/nix/store/mjmkmhbj4jwxn5k5gbmlm0p8pvhyw4v4-nixops-1.7/bin/..nixops-wrapped-wrapped", line 991, in <module>
    args.op()
  File "/nix/store/mjmkmhbj4jwxn5k5gbmlm0p8pvhyw4v4-nixops-1.7/bin/..nixops-wrapped-wrapped", line 412, in op_deploy
    max_concurrent_activate=args.max_concurrent_activate)
  File "/nix/store/mjmkmhbj4jwxn5k5gbmlm0p8pvhyw4v4-nixops-1.7/lib/python2.7/site-packages/nixops/deployment.py", line 1063, in deploy
    self.run_with_notify('deploy', lambda: self._deploy(**kwargs))
  File "/nix/store/mjmkmhbj4jwxn5k5gbmlm0p8pvhyw4v4-nixops-1.7/lib/python2.7/site-packages/nixops/deployment.py", line 1052, in run_with_notify
    f()
  File "/nix/store/mjmkmhbj4jwxn5k5gbmlm0p8pvhyw4v4-nixops-1.7/lib/python2.7/site-packages/nixops/deployment.py", line 1063, in <lambda>
    self.run_with_notify('deploy', lambda: self._deploy(**kwargs))
  File "/nix/store/mjmkmhbj4jwxn5k5gbmlm0p8pvhyw4v4-nixops-1.7/lib/python2.7/site-packages/nixops/deployment.py", line 1003, in _deploy
    self.configs_path = self.build_configs(dry_run=dry_run, repair=repair, include=include, exclude=exclude)
  File "/nix/store/mjmkmhbj4jwxn5k5gbmlm0p8pvhyw4v4-nixops-1.7/lib/python2.7/site-packages/nixops/deployment.py", line 671, in build_configs
    raise Exception("unable to build all machine configurations")
Exception: unable to build all machine configurations

[tomberek]

Can you “show-trace” to help pinpoint the cause? I’d suspect this is when calling hasInfix on a derivation. Perhaps it used to be coerced during the equality check?

And we should verify if

nixpkgs/pkgs/development/interpreters/python/default.nix

Line 102 in 95af224

isPyPy = lib.hasInfix "pypy" interpreter;

is an issue. (Not around a computer at the moment.)

[Izorkin]

/nix/store/qx3zja1j5nicw2n11xlj0dkl1d2gwbgw-python2.7-apache-libcloud-2.8.3/lib/python2.7/site-packages/libcloud/common/google.py:93: CryptographyDeprecationWarning: Python 2 is no longer supported by the Python core team. Support for it is now deprecated in cryptography, and will be removed in the next release.
  from cryptography.hazmat.backends import default_backend
trace: warning: The extraArgs argument to eval-config.nix is deprecated. Please set config._module.args instead.
trace: warning: The extraArgs argument to eval-config.nix is deprecated. Please set config._module.args instead.
trace: warning: The extraArgs argument to eval-config.nix is deprecated. Please set config._module.args instead.
building all machine configurations...
trace: warning: The extraArgs argument to eval-config.nix is deprecated. Please set config._module.args instead.
error: value is a set while a string was expected

       at /home/user/works/src-nix/nixpkgs/lib/strings.nix:256:5:

          255|   hasInfix = infix: content:
          256|     builtins.match ".*${escapeRegex infix}.*" content != null;
             |     ^
          257|

       … while evaluating 'hasInfix'

       at /home/user/works/src-nix/nixpkgs/lib/strings.nix:255:21:

          254|   */
          255|   hasInfix = infix: content:
             |                     ^
          256|     builtins.match ".*${escapeRegex infix}.*" content != null;

       … from call site

       at /home/user/works/src-nix/nixpkgs/nixos/modules/config/users-groups.nix:9:31:

            8|
            9|   isPasswdCompatible = str: !(hasInfix ":" str || hasInfix "\n" str);
             |                               ^
           10|   passwdEntry = type: lib.types.addCheck type isPasswdCompatible // {

       … while evaluating 'isPasswdCompatible'

       at /home/user/works/src-nix/nixpkgs/nixos/modules/config/users-groups.nix:9:24:

            8|
            9|   isPasswdCompatible = str: !(hasInfix ":" str || hasInfix "\n" str);
             |                        ^
           10|   passwdEntry = type: lib.types.addCheck type isPasswdCompatible // {

       … from call site

       at /home/user/works/src-nix/nixpkgs/lib/types.nix:753:78:

          752|     # Augment the given type with an additional type check function.
          753|     addCheck = elemType: check: elemType // { check = x: elemType.check x && check x; };
             |                                                                              ^
          754|

       … while evaluating 'check'

       at /home/user/works/src-nix/nixpkgs/lib/types.nix:753:55:

          752|     # Augment the given type with an additional type check function.
          753|     addCheck = elemType: check: elemType // { check = x: elemType.check x && check x; };
             |                                                       ^
          754|

       … from call site

       at /home/user/works/src-nix/nixpkgs/lib/modules.nix:773:22:

          772|       if isDefined then
          773|         if all (def: type.check def.value) defsFinal then type.merge loc defsFinal
             |                      ^
          774|         else let allInvalid = filter (def: ! type.check def.value) defsFinal;

       … while evaluating anonymous lambda

       at /home/user/works/src-nix/nixpkgs/lib/modules.nix:773:17:

          772|       if isDefined then
          773|         if all (def: type.check def.value) defsFinal then type.merge loc defsFinal
             |                 ^
          774|         else let allInvalid = filter (def: ! type.check def.value) defsFinal;

       … from call site

       at /home/user/works/src-nix/nixpkgs/lib/modules.nix:773:12:

          772|       if isDefined then
          773|         if all (def: type.check def.value) defsFinal then type.merge loc defsFinal
             |            ^
          774|         else let allInvalid = filter (def: ! type.check def.value) defsFinal;

       … while evaluating the attribute 'mergedValue'

       at /home/user/works/src-nix/nixpkgs/lib/modules.nix:771:5:

          770|     # Type-check the remaining definitions, and merge them. Or throw if no definitions.
          771|     mergedValue =
             |     ^
          772|       if isDefined then

       … while evaluating the option `users.users.mastodon.home':

       … while evaluating the attribute 'value'

       at /home/user/works/src-nix/nixpkgs/lib/modules.nix:737:9:

          736|     in warnDeprecation opt //
          737|       { value = builtins.addErrorContext "while evaluating the option `${showOption loc}':" value;
             |         ^
          738|         inherit (res.defsFinal') highestPrio;

       … while evaluating anonymous lambda

       at /home/user/works/src-nix/nixpkgs/lib/modules.nix:289:72:

          288|           # For definitions that have an associated option
          289|           declaredConfig = mapAttrsRecursiveCond (v: ! isOption v) (_: v: v.value) options;
             |                                                                        ^
          290|

       … from call site

       at /home/user/works/src-nix/nixpkgs/lib/attrsets.nix:401:20:

          400|               then recurse (path ++ [name]) value
          401|               else f (path ++ [name]) value;
             |                    ^
          402|         in mapAttrs g;

       … while evaluating 'g'

       at /home/user/works/src-nix/nixpkgs/lib/attrsets.nix:398:19:

          397|           g =
          398|             name: value:
             |                   ^
          399|             if isAttrs value && cond value

       … from call site

       … while evaluating the attribute 'text' of the derivation 'users-groups.json'

       at /home/user/works/src-nix/nixpkgs/pkgs/stdenv/generic/make-derivation.nix:205:7:

          204|     // (lib.optionalAttrs (attrs ? name || (attrs ? pname && attrs ? version)) {
          205|       name =
             |       ^
          206|         let

       … while evaluating the attribute 'value'

       at /home/user/works/src-nix/nixpkgs/lib/modules.nix:572:44:

          571|       defnsByName' = byName "config" (module: value:
          572|           [{ inherit (module) file; inherit value; }]
             |                                            ^
          573|         ) configs;

       … while evaluating 'dischargeProperties'

       at /home/user/works/src-nix/nixpkgs/lib/modules.nix:823:25:

          822|   */
          823|   dischargeProperties = def:
             |                         ^
          824|     if def._type or "" == "merge" then

       … from call site

       at /home/user/works/src-nix/nixpkgs/lib/modules.nix:752:137:

          751|         defs' = concatMap (m:
          752|           map (value: { inherit (m) file; inherit value; }) (builtins.addErrorContext "while evaluating definitions from `${m.file}':" (dischargeProperties m.value))
             |                                                                                                                                         ^
          753|         ) defs;

       … while evaluating definitions from `/home/user/works/src-nix/nixpkgs/nixos/modules/config/users-groups.nix':

       … while evaluating anonymous lambda

       at /home/user/works/src-nix/nixpkgs/lib/modules.nix:751:28:

          750|         # Process mkMerge and mkIf properties.
          751|         defs' = concatMap (m:
             |                            ^
          752|           map (value: { inherit (m) file; inherit value; }) (builtins.addErrorContext "while evaluating definitions from `${m.file}':" (dischargeProperties m.value))

       … from call site

       at /home/user/works/src-nix/nixpkgs/lib/modules.nix:751:17:

          750|         # Process mkMerge and mkIf properties.
          751|         defs' = concatMap (m:
             |                 ^
          752|           map (value: { inherit (m) file; inherit value; }) (builtins.addErrorContext "while evaluating definitions from `${m.file}':" (dischargeProperties m.value))

       … while evaluating the attribute 'values'

       at /home/user/works/src-nix/nixpkgs/lib/modules.nix:864:7:

          863|     in {
          864|       values = concatMap (def: if getPrio def == highestPrio then [(strip def)] else []) defs;
             |       ^
          865|       inherit highestPrio;

       … while evaluating the attribute 'values'

       at /home/user/works/src-nix/nixpkgs/lib/modules.nix:765:9:

          764|       in {
          765|         values = defs''';
             |         ^
          766|         inherit (defs'') highestPrio;

       … while evaluating the attribute 'mergedValue'

       at /home/user/works/src-nix/nixpkgs/lib/modules.nix:771:5:

          770|     # Type-check the remaining definitions, and merge them. Or throw if no definitions.
          771|     mergedValue =
             |     ^
          772|       if isDefined then

       … while evaluating the option `system.activationScripts.users.text':

       … while evaluating the attribute 'value'

       at /home/user/works/src-nix/nixpkgs/lib/modules.nix:737:9:

          736|     in warnDeprecation opt //
          737|       { value = builtins.addErrorContext "while evaluating the option `${showOption loc}':" value;
             |         ^
          738|         inherit (res.defsFinal') highestPrio;

       … while evaluating anonymous lambda

       at /home/user/works/src-nix/nixpkgs/lib/modules.nix:289:72:

          288|           # For definitions that have an associated option
          289|           declaredConfig = mapAttrsRecursiveCond (v: ! isOption v) (_: v: v.value) options;
             |                                                                        ^
          290|

       … from call site

       at /home/user/works/src-nix/nixpkgs/lib/attrsets.nix:401:20:

          400|               then recurse (path ++ [name]) value
          401|               else f (path ++ [name]) value;
             |                    ^
          402|         in mapAttrs g;

       … while evaluating 'g'

       at /home/user/works/src-nix/nixpkgs/lib/attrsets.nix:398:19:

          397|           g =
          398|             name: value:
             |                   ^
          399|             if isAttrs value && cond value

       … from call site

       … while evaluating the attribute 'text'

       at /home/user/works/src-nix/nixpkgs/nixos/modules/system/activation/activation-script.nix:9:5:

            8|   addAttributeName = mapAttrs (a: v: v // {
            9|     text = ''
             |     ^
           10|       #### Activation script snippet ${a}:

       … while evaluating 'id'

       at /home/user/works/src-nix/nixpkgs/lib/trivial.nix:14:5:

           13|     # The value to return
           14|     x: x;
             |     ^
           15|

       … from call site

       … while evaluating 'textClosureMap'

       at /home/user/works/src-nix/nixpkgs/lib/strings-with-deps.nix:75:35:

           74|
           75|   textClosureMap = f: predefined: names:
             |                                   ^
           76|     concatStringsSep "\n" (map f (textClosureList predefined names));

       … from call site

       at /home/user/works/src-nix/nixpkgs/nixos/modules/system/activation/activation-script.nix:49:9:

           48|
           49|       ${textClosureMap id (withDrySnippets) (attrNames withDrySnippets)}
             |         ^
           50|

       … while evaluating 'systemActivationScript'

       at /home/user/works/src-nix/nixpkgs/nixos/modules/system/activation/activation-script.nix:20:33:

           19|
           20|   systemActivationScript = set: onlyDry: let
             |                                 ^
           21|     set' = mapAttrs (_: v: if isString v then (noDepEntry v) // { supportsDryActivation = false; } else v) set;

       … from call site

       at /home/user/works/src-nix/nixpkgs/nixos/modules/system/activation/activation-script.nix:137:18:

          136|       apply = set: set // {
          137|         script = systemActivationScript set false;
             |                  ^
          138|       };

       … while evaluating the attribute 'system.activationScripts.script'

       at /home/user/works/src-nix/nixpkgs/nixos/modules/system/activation/activation-script.nix:137:9:

          136|       apply = set: set // {
          137|         script = systemActivationScript set false;
             |         ^
          138|       };

       … while evaluating the attribute 'activationScript' of the derivation 'nixos-system-web-22.05.git.b94bc9d2d05'

       at /home/user/works/src-nix/nixpkgs/pkgs/stdenv/generic/make-derivation.nix:205:7:

          204|     // (lib.optionalAttrs (attrs ? name || (attrs ? pname && attrs ? version)) {
          205|       name =
             |       ^
          206|         let

       … while evaluating anonymous lambda

       at /nix/store/mjmkmhbj4jwxn5k5gbmlm0p8pvhyw4v4-nixops-1.7/share/nix/nixops/eval-machine-info.nix:371:46:

          370|         mkdir -p $out
          371|         ${toString (attrValues (mapAttrs (n: v: ''
             |                                              ^
          372|           ln -s ${v.config.system.build.toplevel} $out/${n}

       … from call site

       … while evaluating the attribute 'buildCommand' of the derivation 'nixops-machines'

       at /home/user/works/src-nix/nixpkgs/pkgs/stdenv/generic/make-derivation.nix:205:7:

          204|     // (lib.optionalAttrs (attrs ? name || (attrs ? pname && attrs ? version)) {
          205|       name =
             |       ^
          206|         let
Traceback (most recent call last):
  File "/nix/store/mjmkmhbj4jwxn5k5gbmlm0p8pvhyw4v4-nixops-1.7/bin/..nixops-wrapped-wrapped", line 991, in <module>
    args.op()
  File "/nix/store/mjmkmhbj4jwxn5k5gbmlm0p8pvhyw4v4-nixops-1.7/bin/..nixops-wrapped-wrapped", line 412, in op_deploy
    max_concurrent_activate=args.max_concurrent_activate)
  File "/nix/store/mjmkmhbj4jwxn5k5gbmlm0p8pvhyw4v4-nixops-1.7/lib/python2.7/site-packages/nixops/deployment.py", line 1063, in deploy
    self.run_with_notify('deploy', lambda: self._deploy(**kwargs))
  File "/nix/store/mjmkmhbj4jwxn5k5gbmlm0p8pvhyw4v4-nixops-1.7/lib/python2.7/site-packages/nixops/deployment.py", line 1052, in run_with_notify
    f()
  File "/nix/store/mjmkmhbj4jwxn5k5gbmlm0p8pvhyw4v4-nixops-1.7/lib/python2.7/site-packages/nixops/deployment.py", line 1063, in <lambda>
    self.run_with_notify('deploy', lambda: self._deploy(**kwargs))
  File "/nix/store/mjmkmhbj4jwxn5k5gbmlm0p8pvhyw4v4-nixops-1.7/lib/python2.7/site-packages/nixops/deployment.py", line 1003, in _deploy
    self.configs_path = self.build_configs(dry_run=dry_run, repair=repair, include=include, exclude=exclude)
  File "/nix/store/mjmkmhbj4jwxn5k5gbmlm0p8pvhyw4v4-nixops-1.7/lib/python2.7/site-packages/nixops/deployment.py", line 671, in build_configs
    raise Exception("unable to build all machine configurations")
Exception: unable to build all machine configurations

[pennae]

the regex-based version will also fail on paths (which are also used in user checks):

nix-repl> builtins.match ".*/.*" ./default.nix
error: value is a path while a string was expected

       at «string»:1:1:

            1| builtins.match ".*/.*" ./default.nix
             | ^
            2|

nix-repl> lib.hasInfix "/" ./default.nix    
true

looks like we really ought to toString the input first :/