webgl: texture leak fix #139
Conversation
fs-eire
force-pushed
the
webgl-texture-leak-fix
branch
from
c9a09c0
to
a370c03
Compare
| adjustedKernelShape, 4, [adjustedKernelShape[0], adjustedKernelShape[1] * 4], {breakAxis: 1}); | ||
|
|
||
| let bLayout: TextureLayout|undefined; | ||
| const rank = outputShape.length; | ||
|
|
||
| const inputLayouts = [im2colLayout, kLayout]; | ||
| if (inputs.length === 3) { | ||
| bLayout = inferenceHandler.createBasicTextureLayout(inputs[2].dims.slice()); | ||
| bLayout = inferenceHandler.createTextureLayoutFromShape(inputs[2].dims.slice()); |
There was a problem hiding this comment.
There seem to be a lot of unnecessary slicing (copying) of dims in general - as long as the methodd accepts const dims and guarantees that any given dims will not be mutated - can we avoid creating copies unnecessarily ? Granted -it should be only adding to the warm up overhead, but it still kind of stands out...
There was a problem hiding this comment.
thanks for figuring out this. actually there are quite some unnecessary calls to Array.prototype.slice in a lot of places in the code base.
I think it worth another separated change to optimize that
There was a problem hiding this comment.
Oh yeah, I seem to have this comment already - too many slice calls :)
|
|
||
| /** | ||
| * Create a TextureLayout object from a tensor. If a related texture data is found, returns the cached texture layout. | ||
| */ | ||
| getOrCreateTextureLayout(tensor: Tensor, channels = 1, unpackedShape?: ReadonlyArray<number>): TextureLayout { |
There was a problem hiding this comment.
Nit: This name is slightly misleading. I would expect that getOrCreateTexturelayout() to only focus on Layout creation. There is a step that looks into the cache preceding it. Any possible better names for this function ?
There was a problem hiding this comment.
This is what the function is doing: try to get (find from cache) the texture layout of the given tensor; if failed to find then create a new one.. so from the naming perspective I cannot find a better name for it. This is the same for getOrCreateTextureData.
|
|
||
| /** | ||
| * Create a TextureData object, using the given texture. | ||
| * This function does not create new texture. Usually used in scenarios using texture sharing. (eg. Reshape) |
There was a problem hiding this comment.
Can you expand on texture sharing for my understanding please ?
There was a problem hiding this comment.
A WebGLTexture object represents a texture in GPU.
A TextureData object represents a corresponding texture to a tensor. It contains a WebGLTexture object, a reference to the tensor and the texture layout.
A Tensor.Id object represents a unique data unit, regardless where the data is hold (it can be in memory, ie. TypedArray, or in GPU, ie. a texture).
A Tensor object represent a value node in the model graph. It's one of those:
- a model input
- a model initializer
- a model output
- an intermedia value through the model execution
ATensorobject can have multiple way to connect to the data that its tensorId mapped to: - a cached result (TypedArray instance)
- a data provider function ( a function that returns a TypedArray instance )
- an async data provider function (this is not supported yet)
Logically, WebGLTexture objects should be 1:1 mapping to Tensor.Id objects, and TextureData objects should be a 1:1 mapping to Tensor objects.
Some operators, for example, Reshape, will not modify any data of its input. In this case, we are creating a new Tensor object with different dims, using the same Tensor.Id. This is the sharing scenario. We extend this scenario with the equivalent concepts in webgl.
|
Overall looks good to me - I focussed on the "core" WebGL related changes. I skimmed through the related WebGL operator changes that needs to align with the base change itself. I just need to understand the concept of |
This change fixes the texture leak bug that happened in some operators.