dynamicArguments does not give an informative error if misused (Uncaught RangeError: offset is out of bounds)

oversword commented 3 years ago

What is wrong?

Throws an error when changing the length of the input data even though dynamicOutput is set to true and the output has been correctly set Here is the traceback from the unminified master version of the browser code. Also occurs on develop, though the line numbers are different.

gpu-browser.js:18774 Uncaught RangeError: offset is out of bounds
    at Float32Array.set (<anonymous>)
    at Object.flatten2dArrayTo (gpu-browser.js:18774)
    at Object.flattenTo (gpu-browser.js:18810)
    at WebGL2KernelValueSingleArray.updateValue (gpu-browser.js:17059)
    at WebGL2Kernel.run (gpu-browser.js:15030)
    at WebGL2Kernel.run (gpu-browser.js:18427)

Where does it happen?

How do we replicate the issue?

// BASIC SETUP
const gpu = new GPU()
function gpuTest(data) {
    // Perform a trivial return, the issue is not here
    const i = this.thread.x*3
    return [ data[i], data[i+1], data[i+2], i ]
}
const testKernel = gpu.createKernel(gpuTest, {
    dynamicOutput: true,
    returnType: 'Array(4)',
} )

// DEFINE INPUT & OUTPUT
const testData = [
    [1,2,3],
    [2,2,3],
    [3,2,3],
    [4,2,3],
    [5,2,3]
]
let testOutput;

// INITIAL TEST - WORKING
testKernel.setOutput([testData.length])
testOutput = testKernel(testData)
console.log(testOutput, testData)

// ADD MORE DATA
testData.push([1,1,1])

// SECOND TEST - NOT WORKING
testKernel.setOutput([testData.length])
testOutput = testKernel(testData)
console.log(testOutput, testData)

How important is this (1-5)?

1 - solved, just needs documentation update

Expected behaviour (i.e. solution)

If the input size is incorrect it should give an informative and helpful error if dynamicArguments is not set / we are not in a dynamic valueMap, this probably only needs to be caught if the new value is longer than the old one, since shrinking the value will not throw an offset error down the road

The documentation could also be updated to be clearer on what dynamicArguments does, examples on how to use it, and its relationship to dynamicOutput

Other Comments

<now irrelevant, left for posterity> I have attempted to fix this, but it causes unpredictable results (at least from my standpoint)

I think the issue lies in WebGL2KernelValueSingleArray.updateValue, as everything from utils.flattenTo and below is hard to fault.

Looking at other updateValue functions I noticed many of them had this code:

this.dimensions = utils.getDimensions(value, true);
this.textureSize = utils.getMemoryOptimizedFloatTextureSize(this.dimensions, this.bitRatio);
this.uploadArrayLength = this.textureSize[0] * this.textureSize[1] * this.bitRatio;
this.checkSize(this.textureSize[0], this.textureSize[1]);
this.uploadValue = new Float32Array(this.uploadArrayLength);
this.kernel.setUniform3iv(this.dimensionsId, this.dimensions);
this.kernel.setUniform2iv(this.sizeId, this.textureSize);

I tried adding this just above the call to utils.flattenTo. It does produce the correct result for this.uploadValue, and it avoids the error, however the output is very strange, and requires a deeper understanding of how this thing reads data to debug.

The first run produces the correct output - that is, the input followed by the index:

[
    [1,2,3,0],
    [2,2,3,3],
    [3,2,3,6],
    [4,2,3,9],
    [5,2,3,12]
]

The second run not only produces the wrong output for the new value, but also gets existing values wrong that it previously got right:

[
    [1,2,3,0],
    [2,2,3,3],
    [3,2,1,6],
    [1,0,0,9],
    [0,0,0,12],
    [0,1,1,15]
]

This SHOULD be:

[
    [1,2,3,0],
    [2,2,3,3],
    [3,2,3,6],
    [4,2,3,9],
    [5,2,3,12],
    [1,1,1,15]
]

As you can see, the first 2 values are correct, but after that some weird stuff starts happening, keep in mind the intent of the kernel function is simply to spit the input back out with thread.x*3 tacked on the end, that means this is just reading in the data all wrong.

Oh and finally - should probably test this one out actually - but I do wonder why I'm getting a WebGLKernelValueSingleArray, shouldn't it be a WebGLKernelValueDynamicSingleArray? I'll try swapping it out and see what happens, it may just be the wrong one is chosen at some point. The error does seem to occur on matrices of different dimensions though, so I'm not sure...

oversword commented 3 years ago

sigh why do I bother xD

It seems forcing kernelValueMaps.single.static.Array to be WebGL2KernelValueDynamicSingleArray does in fact do what is expected.

This means the issue is simply in choosing which valueMap is used, and also that everything I wrote in this issue except the last part is irrelevant.

I can probably fix this before you even get round to reading it...

oversword commented 3 years ago

It seems the issue is between chair and keyboard :facepalm:

I needed to flag dynamicArguments as true, not just dynamicOutput.

I will leave this issue, and modify it a little, because I believe the actual issue here is that the error message is not informative enough, like it is if you don't set dynamicOutput properly before running the same code. I think the documentation could also be a little clearer about whatdynamicArguments is supposed to do, and perhaps give some examples. I assumed it would allow you to send in more arguments to the function, not to change the size of the arguments, and because the output length is determined by the input length I assumed that dynamicOutput would allow different array sizes for the input and the output

robertleeplummerjr commented 3 years ago

We welcome the documentation and any further insight. Ty!

gpujs / gpu.js