Skip to content

PERF: delay preset sync until tab completion / attribute access, track file desync#1317

Merged
tangkong merged 9 commits intopcdshub:masterfrom
tangkong:perf_preset_loading
Jan 14, 2025
Merged

PERF: delay preset sync until tab completion / attribute access, track file desync#1317
tangkong merged 9 commits intopcdshub:masterfrom
tangkong:perf_preset_loading

Conversation

@tangkong
Copy link
Contributor

@tangkong tangkong commented Jan 11, 2025

Description

Delays preset file reading until tab-completion has been attempted. Stashes this information on the device instance itself. This is not the default behavior, and you must opt into this by supplying the optional argument defer_loading to setup_presets_paths

Also tracks the modification time of the preset file on sync, making the Preset instance the source of truth to see if the presets need to be synced.

Motivation and Context

Presets have taken a very long time to load recently, and the first place to look is often file i/o. This delays this as long as possible while attempting to preserve the behavior hutch scientists use: tab completion.

This has also been made optional. Both setup_presets_paths and Presets.sync() now take an optional defer_loading argument, that if true will skip the preset data loading step

In a nutshell:

  • the device's Preset instance tracks whether the preset is out of sync with the files. This is done by tracking file modification times

  • The device checks if a sync is necessary, and if so syncs with the files when:

    • __dir__ is called (during tab-completion)
    • __getattribute__ is called with a preset-related prefix (wm_, umv_, mv_)

Presets.sync() is also called on device init, since it's part of Presets.__init__. This happens both on device-creation and setup_presets_path, though on device-creation we don't do any file loading. This is a ~0.013s double-hit, split between the device load and presets load sections for FltMvInterface devices

How Has This Been Tested?

Tests pass

Tested interactively through hutch-python, loading some subset of 559 xcs presets

  • this PR: 7.53s (0.01345s / preset)
  • current master: 16.21s (0.0289s / preset)

There's some variation but I'm not going to do a battery of tests here

Why isn't this just 0 s?

There's still some path access going on in hutch-python, it's just stopping short of actually opening the presets file.

Where Has This Been Documented?

This PR, a stray comment

Pre-merge checklist

  • Code works interactively
  • Code contains descriptive docstrings, including context and API
  • New/changed functions and methods are covered in the test suite where possible
  • Test suite passes locally
  • Test suite passes on GitHub Actions
  • Ran docs/pre-release-notes.sh and created a pre-release documentation page
  • Pre-release docs include context, functional descriptions, and contributors as appropriate

@tangkong
Copy link
Contributor Author

I did some more digging (manual profiling because I'm dumb) as to what causes the remaining add-methods to take so long, and I think it boils down to stat() calls having awful performance. I'll do some more digging but I do think this is a contribution to be considered separately

Copy link
Member

@ZLLentz ZLLentz left a comment

Choose a reason for hiding this comment

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

I like this and have a lot of opinions about presets in general.

stat() calls having awful performance

I agree this is a problem on NFS/WEKA.
Probably the main way for us to mitigate this would be reworking presets to use fewer files, but that has its own annoyances.

def __dir__(self):
if not self._tab_initialized and hasattr(self, 'presets'):
self._tab_initialized = True
self.presets.sync()
Copy link
Member

Choose a reason for hiding this comment

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

Change/addition request: we need to identify other times where presets may need a late sync. For example, if someone is expecting presets to be available in a hutch-python function call they need a clear way to trigger this without tabbing (or we need to load them automatically).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The device.presets.sync() call is still available, and seems to be how we suggest people sync presets across sections. I imagine we haven't advertised this as thoroughly as necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess we could overwrite __getattribute__ and catch attempts to access mv_*, umv_*, and wm_* methods, but are there any other access modes we can think of?

Copy link
Member

Choose a reason for hiding this comment

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

Is it possible that doing this on __getattribute__ is actually most correct to make sure the loaded values are always up-to-date?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My one qualm with this is that it does break the already fragile composition abstraction barrier we've set up with the Presets module. Maybe there's also a way around this.

Back to the mines!

@tangkong tangkong requested a review from ZLLentz January 14, 2025 00:32
@tangkong
Copy link
Contributor Author

Updated with some rather significant changes, now we store the last file modification times with every sync() call, and use that to decide whether or not to update. Also added some tests to codify this

@ZLLentz
Copy link
Member

ZLLentz commented Jan 14, 2025

I think at least one of your commits has not been pushed

@tangkong
Copy link
Contributor Author

I blame github

@tangkong
Copy link
Contributor Author

Fingers crossed that we don't see race conditions like in #1050 , and the resulting #1055

ZLLentz
ZLLentz previously approved these changes Jan 14, 2025
Copy link
Member

@ZLLentz ZLLentz left a comment

Choose a reason for hiding this comment

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

I like how this is implemented and I like how it speeds up the startup preset loading by 2x

# deferred_fast_motor_preset must come last,
# to clear cache after motor is created (and sync-ed at init)
assert fast_motor.presets.sync_needed
fast_motor.__dir__() # mimic tab completion request
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: is dir(fast_motor) equivalent?

Copy link
Contributor Author

@tangkong tangkong Jan 14, 2025

Choose a reason for hiding this comment

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

I believe so, do we prefer that to the dunder-access? I used __dir__ just to match the code implementation.

@tangkong tangkong changed the title PERF: delay preset sync until tab completion is accessed PERF: delay preset sync until tab completion / attribute access, track file desync Jan 14, 2025
@janeliu-slac
Copy link

First time seeing updates to pcdsdevices, so probably don't have good feedback this time. Are presets for device settings? Why does preset file reading need to be delayed until tab completion has been attempted?

@tangkong
Copy link
Contributor Author

Presets are basically position shortcuts that users can save/load. They're dynamically added to the devices attributes, and are backed by simple yaml files.
https://pcdshub.github.io/pcdsdevices/v8.7.0/presets.html

Because each device is backed by a file, we end up opening and reading a lot of files in some hutch-python sessions. So instead of loading them all at once, we defer loading until the user actually needs it

ZLLentz
ZLLentz previously approved these changes Jan 14, 2025
return state

@property
def sync_needed(self) -> bool:
Copy link
Member

Choose a reason for hiding this comment

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

One last tiny tiny nitpick: this is a property that touches the filesystem, usually I prefer things that may take time to be function calls. Usually we expect attribute access to be fast, so if they are not fast we can accidentally write code with performance issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A good point. I'll do that

Copy link
Member

@ZLLentz ZLLentz left a comment

Choose a reason for hiding this comment

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

I'm excited for the speedup

@tangkong tangkong merged commit bec57ae into pcdshub:master Jan 14, 2025
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants