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

[external-assets] Cache asset key subsets in refactored AssetGraph #20059

Merged
merged 1 commit into from
Feb 26, 2024

Conversation

smackesey
Copy link
Collaborator

@smackesey smackesey commented Feb 26, 2024

Summary & Motivation

#19900 led to an auto-materialize performance regression caught by our regression tests. After investigating with a profiler, I found the issue was lack of caching on asset key subset methods in the new AssetGraph. This PR adds them.

How I Tested These Changes

Ran perf regression tests against this branch.

@smackesey
Copy link
Collaborator Author

Current dependencies on/for this PR:

This stack of pull requests is managed by Graphite.

@smackesey smackesey requested review from sryza and gibsondan February 26, 2024 17:54
@smackesey smackesey force-pushed the sean/external-assets-cache-key-subsets branch from 1f12ecc to 4140091 Compare February 26, 2024 18:13
Copy link
Member

@gibsondan gibsondan left a comment

Choose a reason for hiding this comment

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

little surprised this was the bottleneck, was it the underlying 'is_observable/is_executable'? should that possibly be cached as well or instead?

@smackesey
Copy link
Collaborator Author

smackesey commented Feb 26, 2024

The bottleneck was entirely regenerating all_asset_keys over and over again.

should that possibly be cached as well or instead?

I think these are already quite fast

@smackesey smackesey merged commit 40b0d9c into master Feb 26, 2024
1 check passed
@smackesey smackesey deleted the sean/external-assets-cache-key-subsets branch February 26, 2024 20:08
cmpadden pushed a commit that referenced this pull request Feb 28, 2024
…20059)

## Summary & Motivation

#19900 led to an auto-materialize performance regression caught by our
regression tests. After investigating with a profiler, I found the issue
was lack of caching on asset key subset methods in the new `AssetGraph`.
This PR adds them.

## How I Tested These Changes

Ran perf regression tests against this branch.
PedramNavid pushed a commit that referenced this pull request Mar 28, 2024
…20059)

## Summary & Motivation

#19900 led to an auto-materialize performance regression caught by our
regression tests. After investigating with a profiler, I found the issue
was lack of caching on asset key subset methods in the new `AssetGraph`.
This PR adds them.

## How I Tested These Changes

Ran perf regression tests against this branch.
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.

2 participants