Skip to content

[Performance] Reduce Memory footprint for tracks #1049

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.

Sign up for GitHub

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

Merged
subdavis merged 2 commits into from
Nov 17, 2021
Merged

[Performance] Reduce Memory footprint for tracks #1049

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9baa706
Remove vue instance from track
subdavis Nov 15, 2021
c478702
Vue is unused
subdavis Nov 15, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 19 additions & 8 deletions client/src/track.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import Vue from 'vue';
import { Ref, ref } from '@vue/composition-api';
import { RectBounds } from './utils';
import {
Expand All @@ -13,6 +12,9 @@ export type ConfidencePair = [string, number];
export type TrackId = number;
export type TrackSupportedFeature = (
GeoJSON.Point | GeoJSON.Polygon | GeoJSON.LineString | GeoJSON.Point);
type TrackNotifier = (
{ track, event, oldValue }: { track: Track; event: string; oldValue: unknown }
) => void;
export interface StringKeyObject {
[key: string]: unknown;
}
Expand Down Expand Up @@ -50,6 +52,7 @@ interface TrackParams {
features?: Array<Feature>;
confidencePairs?: Array<ConfidencePair>;
attributes?: StringKeyObject;
notifier?: TrackNotifier;
}

/**
Expand Down Expand Up @@ -84,7 +87,8 @@ export default class Track {
*/
revision: Ref<number>;

bus: Vue;
/** A callback to notify about changes to the track. */
notifier?: TrackNotifier;
Copy link
Copy Markdown
Collaborator

@BryonLewis BryonLewis Nov 15, 2021

Choose a reason for hiding this comment

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

Looking at this now is it necessary for each Track to have it's own notifier. Could it be a static property and setNotifier be a static method that is assigned once for the entire class? setNotifier is always setting the same function to each track which depends on the function passing in it's own values into it? This would remove the need to unset the notifier from removed tracks because they no longer exist? I guess the worry would be that some track after calling removeTrack still calls a notify even though it isn't in the trackMap? Let me know if there are things I haven't thought of and counter examples.

Copy link
Copy Markdown
Contributor Author

@subdavis subdavis Nov 17, 2021

Choose a reason for hiding this comment

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

After thinking about it, I don't think we should make it a static class method. Looking ahead to stereo or multi-camera views, we may need to be able to differentiate between the event sourcees.

The property is just a pointer, so it costs almost nothing in memory for each track to hold a reference (8 bytes for a pointer).

So for 100K tracks, we're talking 800KB.


constructor(trackId: TrackId, {
meta = {},
Expand All @@ -93,8 +97,8 @@ export default class Track {
features = [],
confidencePairs = [],
attributes = {},
notifier = undefined,
}: TrackParams) {
this.bus = new Vue();
this.trackId = trackId;
this.meta = meta;
this.attributes = attributes;
Expand All @@ -106,6 +110,7 @@ export default class Track {
this.begin = begin;
this.end = end;
this.confidencePairs = confidencePairs;
this.notifier = notifier;
}

get length() {
Expand Down Expand Up @@ -198,11 +203,13 @@ export default class Track {
/* Prevent broadcast until the first feature is initialized */
if (this.isInitialized()) {
this.revision.value += 1;
this.bus.$emit('notify', {
track: this,
event: name,
oldValue,
});
if (this.notifier) {
this.notifier({
track: this,
event: name,
oldValue,
});
}
}
}

Expand Down Expand Up @@ -327,6 +334,10 @@ export default class Track {
}
}

setNotifier(notifier?: TrackNotifier) {
this.notifier = notifier;
}

setFeature(feature: Feature, geometry: GeoJSON.Feature<TrackSupportedFeature>[] = []): Feature {
const f = this.features[feature.frame] || {};
this.features[feature.frame] = {
Expand Down
5 changes: 3 additions & 2 deletions client/src/use/useTrackStore.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ export default function useTrackStore({ markChangesPending }: UseTrackStoreParam
const trackIds: Ref<Array<TrackId>> = ref([]);
const canary = ref(0);


function _depend(): number {
return canary.value;
}
Expand All @@ -82,7 +83,7 @@ export default function useTrackStore({ markChangesPending }: UseTrackStoreParam
}

function insertTrack(track: Track, args?: InsertArgs) {
track.bus.$on('notify', onChange);
track.setNotifier(onChange);
trackMap.set(track.trackId, track);
intervalTree.insert([track.begin, track.end], track.trackId.toString());
if (args && args.afterId) {
Expand Down Expand Up @@ -117,7 +118,7 @@ export default function useTrackStore({ markChangesPending }: UseTrackStoreParam
if (!intervalTree.remove(range, trackId.toString())) {
throw new Error(`TrackId ${trackId} with range ${range} not found in tree.`);
}
track.bus.$off(); // remove all event listeners
track.setNotifier(undefined);
trackMap.delete(trackId);
const listIndex = trackIds.value.findIndex((v) => v === trackId);
if (listIndex === -1) {
Expand Down