Display list of non-deleted WebGL objects #195
Conversation
| "target": "es5" | ||
| "target": "es5", | ||
| "lib": [ | ||
| "ESNext", |
There was a problem hiding this comment.
This was necessary for WeakRef, and it also required an update of typescript
| @@ -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"; | |||
There was a problem hiding this comment.
nit: missing spaces
| @@ -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> }; | |||
There was a problem hiding this comment.
Should this be called instances instead? The code below appears to refer to individual objects as instance.
| if (source) { | ||
| capture.objects[this.objectName][id] = source; | ||
| } else { | ||
| console.log("Object without source"); |
There was a problem hiding this comment.
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?
| console.log("Object without source"); | ||
| } | ||
| } else { | ||
| console.log("Object which was already free'd"); |
There was a problem hiding this comment.
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?
| @@ -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); | |||
There was a problem hiding this comment.
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?
| // 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; } |
There was a problem hiding this comment.
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)
| @@ -5,6 +5,8 @@ export type WebGlObjectTag = { | |||
| customData?: any; | |||
| }; | |||
|
|
|||
| export type WebGlObjectSource = string[]; | |||
There was a problem hiding this comment.
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.
| @@ -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), | |||
There was a problem hiding this comment.
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.
| @@ -501,6 +503,10 @@ export class ResultView { | |||
| return null; | |||
| } | |||
|
|
|||
| if (json.__SPECTOR_Object_SOURCE) { | |||
| return json.__SPECTOR_Object_SOURCE; | |||
| } | |||
There was a problem hiding this comment.
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).
| capture.objects[this.objectName] = {}; | ||
| for (const id in this.objects) { | ||
| if (this.objects.hasOwnProperty(id)) { | ||
| const object = this.objects[id].deref(); |
There was a problem hiding this comment.
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?
|
I like it overall !!! but I am currently moving home with almost no time this week so I ll be able to have a deeper look next week. At least I got internet back :-) |
I'm currently adding WebGL cleanup routines to our code and required tooling to display not only how much memory is leaking, but also where it leaked from.
This is a first PoC / draft to test this.
I'll leave some comments in the code where input would be appreciated.
(I probably won't be able to reserve much time to clean this up, so I want to get this right in as few review passes as possible; if I don't find time to fix this, please feel free to continue where I left off, or take this as inspiration for implementing this from scratch)