Skip to content

database: don't call socket.getfqdn() on every write#46554

Merged
tgamblin merged 1 commit intodevelopfrom
cache-getfqdn
Sep 24, 2024
Merged

database: don't call socket.getfqdn() on every write#46554
tgamblin merged 1 commit intodevelopfrom
cache-getfqdn

Conversation

@tgamblin
Copy link
Copy Markdown
Member

@tgamblin tgamblin commented Sep 24, 2024

We've seen getfqdn() cause slowdowns on macOS in CI when added elsewhere. It's also called by database.py every time we write the DB file.

  • replace the call with a memoized version so that it is only called once per process.

@spackbot-app spackbot-app bot added the core PR affects Spack core functionality label Sep 24, 2024
@tgamblin tgamblin requested a review from alalazo September 24, 2024 04:52
@tgamblin
Copy link
Copy Markdown
Member Author

Let's see if this speeds up macOS CI.

We've seen `getfqdn()` cause slowdowns on macOS in CI when added elsewhere. It's also
called by database.py every time we write the DB file.

- [x] replace the call with a memoized version so that it is only called once per process.

Signed-off-by: Todd Gamblin <[email protected]>
@tgamblin
Copy link
Copy Markdown
Member Author

tgamblin commented Sep 24, 2024

🚀 This appears to speed up macOS CI a lot:

CI Job This PR #46548 #46536 #46529
macOS-13, 3.11 26m 54m 50m 58m
macOS-14, 3.11 14m 42m 40m 40m

Guess we can all pour one out for all the cycles lost to getfqdn() 😞

@tgamblin
Copy link
Copy Markdown
Member Author

@Jordan474: FYI, see above. Your #45522 has apparently clued us into a long-standing performance issue.

I think we can get this one merged, then maybe you can submit a refactored #45522 using a cached _getfqdn().

@alalazo any particular place you think the cached call should live? spack.util.socket? Not really sure where the best place to put the memoized call is.

@tgamblin
Copy link
Copy Markdown
Member Author

@haampie FYI

@tgamblin tgamblin added this to the v0.23.0 milestone Sep 24, 2024
Copy link
Copy Markdown
Member

@alalazo alalazo left a comment

Choose a reason for hiding this comment

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

If we want to move this into spack.util.socket that would be fine with me. In the meanwhile I'll merge this one, since we can refactor later when adding back #45522

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core PR affects Spack core functionality performance

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants