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

Display list of non-deleted WebGL objects #195

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 3 additions & 3 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@
"sass-loader": "^8.0.2",
"style-loader": "^1.1.3",
"tslint": "^6.1.3",
"typescript": "^3.8.3",
"typescript": "^4.2.4",
"webpack": "^4.42.0",
"webpack-cli": "^3.3.11"
}
Expand Down
43 changes: 42 additions & 1 deletion src/backend/recorders/baseRecorder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { WebGlConstants, WebGlConstantsByValue } from "../types/webglConstants";
import { FunctionCallbacks, IFunctionInformation } from "../types/functionInformation";
import { ICapture } from "../../shared/capture/capture";
import { WebGlObjects } from "../webGlObjects/baseWebGlObject";
import {StackTrace} from "../../shared/utils/stackTrace";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

nit: missing spaces


export interface IRecorder {
registerCallbacks(onFunctionCallbacks: FunctionCallbacks): void;
Expand Down Expand Up @@ -85,6 +86,8 @@ export abstract class BaseRecorder<T extends WebGLObject> implements IRecorder {
protected readonly startTime: number;
protected readonly memoryPerSecond: { [second: number]: number };

private objects: { [id: number]: WeakRef<WebGLObject> };
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should this be called instances instead? The code below appears to refer to individual objects as instance.


private totalMemory: number;
private frameMemory: number;
private capturing: boolean;
Expand Down Expand Up @@ -131,6 +134,22 @@ export abstract class BaseRecorder<T extends WebGLObject> implements IRecorder {
public appendRecordedInformation(capture: ICapture): void {
capture.frameMemory[this.objectName] = this.frameMemory;
capture.memory[this.objectName] = this.memoryPerSecond;
capture.objects[this.objectName] = {};
for (const id in this.objects) {
if (this.objects.hasOwnProperty(id)) {
const object = this.objects[id].deref();
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 idea of a WeakRef was to allow garbage collection on WebGL objects (although I've never seen it in practice yet).

However, objects which no longer exist must also have their WeakRef removed from this.objects, or those WeakRef themselves still leak memory.
We should probably auto-removed dead references from the this.objects from time to time.
There could be a garbage collector every N object creations, which would look if any WeakRef are dead, to remove the WeakRef itself from the list.
FinalizerRegistry might work to get auto notification when the list would have to be cleaned up.

There are also WeakMap and WeakSet but we probably can't use them here, because we can't iterate over them.

Thoughts?

if (object) {
const source = WebGlObjects.getWebGlObjectSource(object);
if (source) {
capture.objects[this.objectName][id] = source;
} else {
console.log("Object without source");
Copy link
Contributor Author

@JannikGM JannikGM May 7, 2021

Choose a reason for hiding this comment

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

This currently doesn't pass the linter.
I need a way to display this properly; I never saw these messages, and I assume they got compiled away?

}
} else {
console.log("Object which was already free'd");
Copy link
Contributor Author

@JannikGM JannikGM May 7, 2021

Choose a reason for hiding this comment

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

This currently doesn't pass the linter.
I need a way to display this properly; I never saw these messages, and I assume they got compiled away?

}
}
}
}

protected abstract getCreateCommandNames(): string[];
Expand All @@ -141,7 +160,19 @@ export abstract class BaseRecorder<T extends WebGLObject> implements IRecorder {
protected abstract delete(instance: T): number;

protected create(functionInformation: IFunctionInformation): void {
// Nothing tracked currently on create.
// Removes the spector internal calls to leave only the relevant part.
const stackTrace = StackTrace.getStackTrace(5, 0);
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 range for this StackTrace trimming was not checked. Overall, I wasn't sure why the first entry would have to be removed? Is this only relevant for specific browsers?


const createdWebGlObject = functionInformation.result as T;

// Add information where this object was created
WebGlObjects.attachWebGlObjectSource(createdWebGlObject, stackTrace);

// Add our object to the list of tracked objects (object should only exist once).
const tag = WebGlObjects.getWebGlObjectTag(createdWebGlObject);
this.objects = this.objects || [];
if (this.objects[tag.id]) { debugger; }
Copy link
Contributor Author

@JannikGM JannikGM May 7, 2021

Choose a reason for hiding this comment

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

This currently doesn't pass the linter.
This should never happen, so I'll probably remove this line or early out, just to shut up the compiler.
Ideally I'd like to have some sort of assertion or sanity check here.

(A similar case already existed in updateWithoutSideEffects, which I consider problematic, too)

this.objects[tag.id] = new WeakRef(createdWebGlObject);
}

protected createWithoutSideEffects(functionInformation: IFunctionInformation): void {
Expand Down Expand Up @@ -186,6 +217,16 @@ export abstract class BaseRecorder<T extends WebGLObject> implements IRecorder {
}

this.options.toggleCapture(false);

const tag = WebGlObjects.getWebGlObjectTag(instance);
if (!tag) {
this.options.toggleCapture(true);
return;
}

// Remove object from list of objects
delete this.objects[tag.id];

const size = this.delete(instance);
this.changeMemorySize(-size);
this.options.toggleCapture(true);
Expand Down
1 change: 1 addition & 0 deletions src/backend/spies/contextSpy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,7 @@ export class ContextSpy {
analyses: [],
frameMemory: {},
memory: {},
objects: {},
};

// Refreshes canvas info in case it changed beffore the capture.
Expand Down
1 change: 1 addition & 0 deletions src/backend/states/baseState.ts
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,7 @@ export abstract class BaseState {

return {
__SPECTOR_Object_TAG: WebGlObjects.getWebGlObjectTag(object) || this.options.tagWebGlObject(object),
__SPECTOR_Object_Source: WebGlObjects.getWebGlObjectSource(object),
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 believe this is dead code now? If not, this should be __SPECTOR_Object_Source although I plan to rename it to __SPECTOR_Object_ORIGIN or __SPECTOR_Object_LIFETIME or something.

__SPECTOR_Object_CustomData: object.__SPECTOR_Object_CustomData,
__SPECTOR_Metadata: object.__SPECTOR_Metadata,
};
Expand Down
11 changes: 11 additions & 0 deletions src/backend/webGlObjects/baseWebGlObject.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ export type WebGlObjectTag = {
customData?: any;
};

export type WebGlObjectSource = string[];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Instead, this should be an object which also stores when the object was created.
There could also be a flag where even removed objects are listed / kept, but their delete location / time is also recorded.


export class WebGlObjects {
public static getWebGlObjectTag(object: WebGLObject): WebGlObjectTag {
return (object as any)[WebGlObjects.SPECTOROBJECTTAGKEY];
Expand All @@ -22,7 +24,16 @@ export class WebGlObjects {
return `${tag.typeName} - ID: ${tag.id}`;
}

public static getWebGlObjectSource(object: WebGLObject): WebGlObjectSource {
return (object as any)[WebGlObjects.SPECTOROBJECTSOURCEKEY];
}

public static attachWebGlObjectSource(object: WebGLObject, source: WebGlObjectSource): void {
(object as any)[WebGlObjects.SPECTOROBJECTSOURCEKEY] = source;
}

private static readonly SPECTOROBJECTTAGKEY = "__SPECTOR_Object_TAG";
private static readonly SPECTOROBJECTSOURCEKEY = "__SPECTOR_Object_SOURCE";
}

// tslint:disable-next-line:max-classes-per-file
Expand Down
6 changes: 6 additions & 0 deletions src/embeddedFrontend/resultView/resultView.ts
Original file line number Diff line number Diff line change
Expand Up @@ -407,6 +407,8 @@ export class ResultView {
}
this.displayJSONGroup(rightJsonContentStateId, "Frame Memory Changes", capture.frameMemory);
this.displayJSONGroup(rightJsonContentStateId, "Total Memory (seconds since application start: bytes)", capture.memory);
this.displayJSONGroup(rightJsonContentStateId, "Objects", capture.objects);

}

private displayJSON(parentGroupId: number, json: any) {
Expand Down Expand Up @@ -501,6 +503,10 @@ export class ResultView {
return null;
}

if (json.__SPECTOR_Object_SOURCE) {
return json.__SPECTOR_Object_SOURCE;
}
Copy link
Contributor Author

@JannikGM JannikGM May 7, 2021

Choose a reason for hiding this comment

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

I don't think this ever did anything. I'd like to display the origin somewhere in the command list detail pane, too. I simply wasn't sure how to do it (I have a lot of trouble understanding the codebase to be honest).


if (json.__SPECTOR_Object_TAG) {
return json.__SPECTOR_Object_TAG.displayText;
}
Expand Down
1 change: 1 addition & 0 deletions src/shared/capture/capture.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,4 +16,5 @@ export interface ICapture {
analyses: IAnalysis[];
frameMemory: { [objectName: string]: number };
memory: { [objectName: string]: { [second: number]: number } };
objects: { [objectName: string]: { [id: number]: string[] } };
}
8 changes: 6 additions & 2 deletions src/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,10 @@
"experimentalDecorators": true,
"module": "es6",
"moduleResolution": "node",
"target": "es5"
"target": "es5",
"lib": [
"ESNext",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was necessary for WeakRef, and it also required an update of typescript

"dom"
]
}
}
}