Skip to content

Commit

Permalink
[Run Timeline Perf] Don't reset timer for fetching ongoing runs when …
Browse files Browse the repository at this point in the history
…time range changes (#22464)

## Summary & Motivation

ongoing runs doesn't depend on the time range so don't make it refetch
when the time range passed to `useRunsForTimeline` changes
significantly.

Also future ticks aren't visible if time range is in the past so dont
make that query.

## How I Tested These Changes

Paginate forward/backwards on the RunTimeline and see it doesn't wait
for ongoing runs query.
  • Loading branch information
salazarm committed Jun 11, 2024
1 parent e75173f commit 2724587
Show file tree
Hide file tree
Showing 4 changed files with 41 additions and 9 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -62,11 +62,32 @@ export class HourlyDataCache<T> {
return await this.loadPromise;
}

private saveTimeout?: ReturnType<typeof setTimeout>;
private registeredUnload: boolean = false;
private async saveCacheToIndexedDB() {
if (!this.indexedDBCache) {
if (typeof jest !== undefined) {
if (!this.indexedDBCache) {
return;
}
this.indexedDBCache.set(this.indexedDBKey, this.cache, defaultOptions);
return;
}
await this.indexedDBCache.set(this.indexedDBKey, this.cache, defaultOptions);
clearTimeout(this.saveTimeout);
this.saveTimeout = setTimeout(() => {
if (!this.indexedDBCache) {
return;
}
this.indexedDBCache.set(this.indexedDBKey, this.cache, defaultOptions);
}, 10000);
if (!this.registeredUnload) {
this.registeredUnload = true;
window.addEventListener('beforeunload', () => {
if (!this.indexedDBCache) {
return;
}
this.indexedDBCache.set(this.indexedDBKey, this.cache, defaultOptions);
});
}
}

public async clearOldEntries() {
Expand All @@ -79,7 +100,6 @@ export class HourlyDataCache<T> {
this.cache.delete(ts);
}
}
await this.saveCacheToIndexedDB();
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -467,7 +467,6 @@ describe('useRunsForTimeline', () => {
mockPaginatedRuns({cursor: '1-2', result: thirdPageResult}),
mockOngoingRuns({limit: 1}),
mockOngoingRuns({limit: 1, results: [], cursor: '2'}),
mockFutureTicks(interval[0], interval[1]),
];

const mockCbs = mocks.map(getMockResultFn);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -363,7 +363,7 @@ export const useRunsForTimeline = ({
const current = {jobInfo, runsByJobKey: map};
previousRunsByJobKey.current = current;
return current;
}, [completedRuns, ongoingRunsData, loading]);
}, [loading, ongoingRunsData, completedRuns]);

const jobsWithCompletedRunsAndOngoingRuns = useMemo(() => {
const jobs: Record<string, TimelineJob> = {};
Expand Down Expand Up @@ -512,7 +512,7 @@ export const useRunsForTimeline = ({
const lastFetchRef = useRef({ongoing: 0, future: 0});
const lastRangeMs = useRef([0, 0] as readonly [number, number]);
if (Math.abs(lastRangeMs.current[0] - rangeMs[0]) > 30000) {
lastFetchRef.current = {ongoing: 0, future: 0};
lastFetchRef.current.future = 0;
}
lastRangeMs.current = rangeMs;

Expand All @@ -528,9 +528,10 @@ export const useRunsForTimeline = ({
lastFetchRef.current.ongoing = Date.now();
}
})(),
// Only fetch future ticks on a minute
// Only fetch future ticks once a minute
(async () => {
if (lastFetchRef.current.future < Date.now() - 60 * 1000) {
// If the the time range is in the past then future ticks are not visible on the timeline
if (_end > Date.now() && lastFetchRef.current.future < Date.now() - 60 * 1000) {
fetchFutureTicks();
}
})(),
Expand All @@ -539,7 +540,7 @@ export const useRunsForTimeline = ({
if (loadId === fetchIdRef.current) {
setLoading(false);
}
}, [fetchCompletedRunsQueryData, fetchFutureTicks, fetchOngoingRunsQueryData]),
}, [fetchCompletedRunsQueryData, fetchFutureTicks, fetchOngoingRunsQueryData, _end]),
intervalMs: refreshInterval,
leading: true,
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,23 +13,34 @@ export const KEY_PREFIX = 'indexdbQueryCache:';
export class CacheManager<TQuery> {
private cache: ReturnType<typeof cache<string, CacheData<TQuery>>>;
private key: string;
private current?: CacheData<TQuery>;

constructor(key: string) {
this.key = `${KEY_PREFIX}${key}`;
this.cache = cache<string, CacheData<TQuery>>({dbName: this.key, maxCount: 1});
}

async get(version: number): Promise<TQuery | null> {
if (this.current) {
return this.current.data;
}
if (await this.cache.has('cache')) {
const {value} = await this.cache.get('cache');
if (value && version === value.version) {
this.current = value;
return value.data;
}
}
return null;
}

async set(data: TQuery, version: number): Promise<void> {
if (
JSON.stringify(this.current?.data) === JSON.stringify(data) &&
this.current?.version === version
) {
return;
}
return this.cache.set('cache', {data, version}, {expiry: new Date('3030-01-01')});
}

Expand Down Expand Up @@ -134,6 +145,7 @@ export function useGetData() {
});

const {data} = queryResult;

await cacheManager.set(data, version);

const onFetchedHandlers = state.onFetched;
Expand Down

1 comment on commit 2724587

@github-actions
Copy link

Choose a reason for hiding this comment

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

Deploy preview for dagit-core-storybook ready!

✅ Preview
https://dagit-core-storybook-85g9jit6h-elementl.vercel.app

Built with commit 2724587.
This pull request is being automatically deployed with vercel-action

Please sign in to comment.