Refactor to improve `spec format` speed by tgamblin · Pull Request #43712 · spack/spack

tgamblin · 2024-04-18T07:04:13Z

When looking at where we spend our time in solver setup, I noticed a fair bit of time is spent in Spec.format(), and Spec.format() is a pretty old, slow, convoluted method.

This PR does a number of things:

Consolidate most of what was being done manually with a character loop and several regexes into a single regex.
Precompile regexes where we keep them
Remove the transform= argument to Spec.format() which was only used in one place in the code (modules) to uppercase env var names, but added a lot of complexity
Avoid escaping and colorizing specs unless necessary
Refactor a lot of the colorization logic to avoid unnecessary object construction
Add type hints and remove some spots in the code where we were using nonexistent arguments to format().
Add trivial cases to __str__ in VariantMap and VersionList to avoid sorting
Avoid calling isinstance() in the main loop of Spec.format()
Don't bother constructing a string representation for the result of _prev_versionas it is only used for comparisons.

In my timings (on all the specs formatted in a solve of hdf5), this is over 2.67x faster than the original format(), and it seems to reduce setup time by around a second (for hdf5).

lib/spack/spack/spec.py

haampie · 2024-04-18T07:11:53Z

Note that this does not memoize the result of Spec.format(), as @haampie had suggested to do

my suggestion was to memoize the result of parsing the format string, although not 100% clear if better.

Like parsing {name}-{version} and saving lambda s: f"{s.name}-{s.version}", but that sounds like codegen that might be tricky in python.

tgamblin · 2024-04-18T07:15:57Z

If you want to try this out, I made a little benchmark here: https://github.com/tgamblin/spec-format-perf

tgamblin · 2024-04-18T07:17:19Z

Ok that makes more sense. I think actually memoizing the format result during setup is probably going to be a bigger win.

…b()`

This removes a bunch of old methods and makes the color formatting logic a lot clearer. - [x] Remove `colorize_spec()`, which uses older, now incorrect colorization logic. It is older than the current `Spec.format()` implementation and it doesn't work with `@=`. The only usages in `cmd/info.py` can be replaced wtih `Spec.cformat()`. - [x] Remove `_SEPARATORS` AND `COLOR_FORMATS`, which are no longer needed. - [x] Simplify `Spec.format()` code to use more readable color constants directly. - [x] Remove unused color constants. - [x] Remove unused parameter for `write_attribute()` - [x] Remove unused `Spec.colorized()` method

Further performance improvement by avoiding object construction `match_to_ansi` was being constructed every time. We can avoid an unnecessary object creation by not using a class here.

Optimize format speed by avoiding object construction.

We can get the regex engine to do more work and to avoid string comparisons.

Avoid a constructor call if it's not needed.

tgamblin · 2024-04-21T22:51:55Z

@alalazo can you look at 30d85cb (spec format: remove unnecessary transform= argument) and make sure you agree?

tgamblin · 2024-04-21T23:01:01Z

As of now I'm getting:

1.155e-4 s/format (this), vs
2.655e-4 s/format (develop)

So this is ~2.3x faster at least on an m1 mac.

`transform=` adds a whole lot of extra complexity to `Spec.format()`, and the thing it was originally intended for (the `decorator=` arg to `spack.cmd.display_specs()`) is now handled entirely in `display_specs()`. The only place it is still used is in the common modules code, and we can handle what it's used for there with a simple `str.upper()`. If people want to format different components of specs differently, they can call `format()` multiple times.

- [x] raise an exception for an unterminated escape character ('\') - [x] format_path preserves leading '/' as '\' when on windows

- [x] trivial cases for VariantMap and VersionList - [x] don't bother making a string in `_prev_version` (which is used only in comparisons)

tgamblin · 2024-04-22T07:38:33Z

Ok now it's 2.59x faster. I think I have wrung all I can out of this. If we can avoid somehow constructing a StandardVersion in _prev_version, it might help a bit more, but I do not see an easy way to do that.

I think this is ready to go

alalazo

I just have a few comments. This commit 0771aca LGTM

lib/spack/llnl/util/tty/__init__.py

lib/spack/llnl/util/tty/color.py

lib/spack/spack/cmd/info.py

lib/spack/spack/spec.py

lib/spack/spack/test/spec_semantics.py

lib/spack/spack/variant.py

lib/spack/spack/version/version_types.py

tgamblin · 2024-04-23T16:41:56Z

As of now, this is ~2.67x faster than develop and cuts a second off the hdf5 concretization time for me on an m1.

When looking at where we spend our time in solver setup, I noticed a fair bit of time is spent in `Spec.format()`, and `Spec.format()` is a pretty old, slow, convoluted method. This PR does a number of things: - [x] Consolidate most of what was being done manually with a character loop and several regexes into a single regex. - [x] Precompile regexes where we keep them - [x] Remove the `transform=` argument to `Spec.format()` which was only used in one place in the code (modules) to uppercase env var names, but added a lot of complexity - [x] Avoid escaping and colorizing specs unless necessary - [x] Refactor a lot of the colorization logic to avoid unnecessary object construction - [x] Add type hints and remove some spots in the code where we were using nonexistent arguments to `format()`. - [x] Add trivial cases to `__str__` in `VariantMap` and `VersionList` to avoid sorting - [x] Avoid calling `isinstance()` in the main loop of `Spec.format()` - [x] Don't bother constructing a `string` representation for the result of `_prev_version` as it is only used for comparisons. In my timings (on all the specs formatted in a solve of `hdf5`), this is over 2.67x faster than the original `format()`, and it seems to reduce setup time by around a second (for `hdf5`).

tgamblin requested review from alalazo and haampie April 18, 2024 07:04

spackbot-app bot added commands core PR affects Spack core functionality modules stand-alone-tests Stand-alone (or smoke) tests for installed packages tests General test capability(ies) utilities versions labels Apr 18, 2024

tgamblin commented Apr 18, 2024

View reviewed changes

lib/spack/spack/spec.py Outdated Show resolved Hide resolved

tgamblin added the specs label Apr 18, 2024

haampie mentioned this pull request Apr 18, 2024

setup performance improvements #43717

Merged

tgamblin requested a review from becker33 April 18, 2024 16:02

tldahlgren assigned haampie Apr 18, 2024

tgamblin added 7 commits April 21, 2024 15:41

spec format: precompile regexes and replace charater loop with `re.su…

2001863

…b()`

spec format: simplify parsing by using one regex

21a0fd7

spec format: avoid class construction during colorize()

288f319

Further performance improvement by avoiding object construction `match_to_ansi` was being constructed every time. We can avoid an unnecessary object creation by not using a class here.

spec format: avoid StandardVersion construction in VersionList.__str__

c52bbb4

Optimize format speed by avoiding object construction.

spec format: avoid string comparison in match_to_ansi

2a936b5

We can get the regex engine to do more work and to avoid string comparisons.

spec format: avoid an extra str() in cescape()

c739d8c

Avoid a constructor call if it's not needed.

tgamblin force-pushed the fast-format branch from 7f45760 to b3c931b Compare April 21, 2024 22:50

tgamblin force-pushed the fast-format branch from b3c931b to 75056a9 Compare April 21, 2024 23:17

tgamblin added 2 commits April 21, 2024 20:00

spec format: simplify special cases with attributes

50b2058

tgamblin added 2 commits April 21, 2024 20:00

handle tty.msg(None)

ea22a2c

spec format: fail on bare '\' character

14c8f70

- [x] raise an exception for an unterminated escape character ('\') - [x] format_path preserves leading '/' as '\' when on windows

tgamblin force-pushed the fast-format branch from 75056a9 to 14c8f70 Compare April 22, 2024 03:00

tgamblin added 2 commits April 22, 2024 00:09

spec format: a few more optimizations

0173be4

- [x] trivial cases for VariantMap and VersionList - [x] don't bother making a string in `_prev_version` (which is used only in comparisons)

spec format: avoid most isinstance() calls with name check

3bedc20

spec format: fix windows path handling per conversation with Peter

f9512e3

tgamblin changed the title ~~Refactor and improve spec format speed~~ Refactor to improve spec format speed Apr 23, 2024

alalazo reviewed Apr 23, 2024

View reviewed changes

tgamblin added 5 commits April 23, 2024 09:26

spec format: clean up tty.msg

4b3e450

spec format: add return type to cescape()

ce3d87b

fix type in _fmt_when

47d038c

spec format: fix format() docstring

a7b4df9

spec format: return type for safe_color

2a5181c

alalazo approved these changes Apr 23, 2024

View reviewed changes

tgamblin merged commit aa0825d into develop Apr 23, 2024

tgamblin deleted the fast-format branch April 23, 2024 17:52

greenc-FNAL mentioned this pull request Apr 24, 2024

Merge from upstream 2024-04-24 FNALssi/spack#332

Merged

alecbcs assigned tgamblin May 17, 2024

tgamblin unassigned haampie and tgamblin May 18, 2024

alecbcs assigned tgamblin May 20, 2024

tgamblin mentioned this pull request Jun 29, 2024

[WIP] Concretizer: stabilize solve input #44810

Closed

1 task

Jordan474 mentioned this pull request Sep 4, 2024

Fix regression in spec format string for indiviual variants #46206

Merged

tgamblin mentioned this pull request Apr 3, 2025

spec_parser.py: concrete variants with := #49756

Merged

haampie mentioned this pull request Sep 1, 2025

zsh: fix regression in spack env activate --prompt #51258

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor to improve `spec format` speed#43712

Refactor to improve `spec format` speed#43712
tgamblin merged 28 commits intodevelopfrom
fast-format

tgamblin commented Apr 18, 2024 •

edited

Loading

Uh oh!

Uh oh!

haampie commented Apr 18, 2024 •

edited

Loading

Uh oh!

tgamblin commented Apr 18, 2024

Uh oh!

tgamblin commented Apr 18, 2024

Uh oh!

tgamblin commented Apr 21, 2024

Uh oh!

tgamblin commented Apr 21, 2024 •

edited

Loading

Uh oh!

tgamblin commented Apr 22, 2024

Uh oh!

alalazo left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tgamblin commented Apr 23, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

tgamblin commented Apr 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

haampie commented Apr 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tgamblin commented Apr 18, 2024

Uh oh!

tgamblin commented Apr 18, 2024

Uh oh!

tgamblin commented Apr 21, 2024

Uh oh!

tgamblin commented Apr 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tgamblin commented Apr 22, 2024

Uh oh!

alalazo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tgamblin commented Apr 23, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tgamblin commented Apr 18, 2024 •

edited

Loading

haampie commented Apr 18, 2024 •

edited

Loading

tgamblin commented Apr 21, 2024 •

edited

Loading