Skip to content

Commit

Permalink
Always test for a fence with gl2.clientWaitSync(sync, 0, 0) (tensorfl…
Browse files Browse the repository at this point in the history
…ow#1529)

Fixes tensorflow/tfjs#1145

The smallest repro is calling .data() 3 times without waiting:
```ts
tf.ENV.set('WEBGL_CPU_FORWARD', false)
tf.ENV.set('WEBGL_SIZE_UPLOAD_UNIFORM', 0);
tf.square(3).data();
tf.square(3).data();
tf.square(3).data();
```

There are 3 fences that are in parallel. And on the next tick the binary search tests only the middle fence and sees that it is done, and assumes the first fence is done too. But since we never explicitly tested fence[0].isDone(), Chrome gives a warning.

The warnings started showing up more often when Layers started doing parallel reads of the scalar loss values: tensorflow/tfjs-layers#398

I changed the binary search to linear search so we also test all fences up to the one that is not done.
  • Loading branch information
dsmilkov authored Feb 4, 2019
1 parent 3a77069 commit 87ce1a8
Show file tree
Hide file tree
Showing 2 changed files with 29 additions and 34 deletions.
31 changes: 13 additions & 18 deletions src/kernels/webgl/gpgpu_context.ts
Original file line number Diff line number Diff line change
Expand Up @@ -520,9 +520,8 @@ export class GPGPUContext {
private itemsToPoll: PollItem[] = [];

pollItems(): void {
// Find the last query that has finished using binary search.
// All other queries before it are also done.
const index = binSearchLastTrue(this.itemsToPoll.map(x => x.isDoneFn));
// Find the last query that has finished.
const index = linearSearchLastTrue(this.itemsToPoll.map(x => x.isDoneFn));
for (let i = 0; i <= index; ++i) {
const {resolveFn} = this.itemsToPoll[i];
resolveFn();
Expand Down Expand Up @@ -616,22 +615,18 @@ type PollItem = {
};

/**
* Finds the index of the last true element using binary search where
* evaluation of an entry is expensive.
* Finds the index of the last true element using linear search.
* Note: We can't do binary search because Chrome expects us to explicitly
* test all fences before download:
* https://github.com/tensorflow/tfjs/issues/1145
*/
export function binSearchLastTrue(arr: Array<() => boolean>): number {
let start = 0;
let end = arr.length - 1;
let best = -1;
while (start <= end) {
const mid = (start + end) >> 1;
const isDone = arr[mid]();
if (isDone) {
best = mid;
start = mid + 1;
} else {
end = mid - 1;
export function linearSearchLastTrue(arr: Array<() => boolean>): number {
let i = 0;
for (; i < arr.length; ++i) {
const isDone = arr[i]();
if (!isDone) {
break;
}
}
return best;
return i - 1;
}
32 changes: 16 additions & 16 deletions src/kernels/webgl/gpgpu_context_test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
import {describeWithFlags} from '../../jasmine_util';
import {expectArraysClose, expectNumbersClose} from '../../test_util';
import {getGlslDifferences} from './glsl_version';
import {binSearchLastTrue, GPGPUContext} from './gpgpu_context';
import {GPGPUContext, linearSearchLastTrue} from './gpgpu_context';
import * as tex_util from './tex_util';

const DOWNLOAD_FLOAT_ENVS = {
Expand Down Expand Up @@ -316,88 +316,88 @@ describeWithFlags('GPGPUContext', DOWNLOAD_FLOAT_ENVS, () => {
});
});

describe('gpgpu_context binSearchLastTrue', () => {
describe('gpgpu_context linearSearchLastTrue', () => {
it('[false]', () => {
const a: boolean[] = [false];
const arr = a.map(x => () => x);
expect(binSearchLastTrue(arr)).toBe(-1);
expect(linearSearchLastTrue(arr)).toBe(-1);
});

it('[true]', () => {
const a: boolean[] = [true];
const arr = a.map(x => () => x);
expect(binSearchLastTrue(arr)).toBe(0);
expect(linearSearchLastTrue(arr)).toBe(0);
});

it('[false, false]', () => {
const a: boolean[] = [false, false];
const arr = a.map(x => () => x);
expect(binSearchLastTrue(arr)).toBe(-1);
expect(linearSearchLastTrue(arr)).toBe(-1);
});

it('[true, false]', () => {
const a: boolean[] = [true, false];
const arr = a.map(x => () => x);
expect(binSearchLastTrue(arr)).toBe(0);
expect(linearSearchLastTrue(arr)).toBe(0);
});

it('[true, true]', () => {
const a: boolean[] = [true, true];
const arr = a.map(x => () => x);
expect(binSearchLastTrue(arr)).toBe(1);
expect(linearSearchLastTrue(arr)).toBe(1);
});

it('[false, false, false]', () => {
const a: boolean[] = [false, false, false];
const arr = a.map(x => () => x);
expect(binSearchLastTrue(arr)).toBe(-1);
expect(linearSearchLastTrue(arr)).toBe(-1);
});

it('[true, false, false]', () => {
const a: boolean[] = [true, false, false];
const arr = a.map(x => () => x);
expect(binSearchLastTrue(arr)).toBe(0);
expect(linearSearchLastTrue(arr)).toBe(0);
});

it('[true, true, false]', () => {
const a: boolean[] = [true, true, false];
const arr = a.map(x => () => x);
expect(binSearchLastTrue(arr)).toBe(1);
expect(linearSearchLastTrue(arr)).toBe(1);
});

it('[true, true, true]', () => {
const a: boolean[] = [true, true, true];
const arr = a.map(x => () => x);
expect(binSearchLastTrue(arr)).toBe(2);
expect(linearSearchLastTrue(arr)).toBe(2);
});

it('[false, false, false, false]', () => {
const a: boolean[] = [false, false, false, false];
const arr = a.map(x => () => x);
expect(binSearchLastTrue(arr)).toBe(-1);
expect(linearSearchLastTrue(arr)).toBe(-1);
});

it('[true, false, false, false]', () => {
const a: boolean[] = [true, false, false, false];
const arr = a.map(x => () => x);
expect(binSearchLastTrue(arr)).toBe(0);
expect(linearSearchLastTrue(arr)).toBe(0);
});

it('[true, true, false, false]', () => {
const a: boolean[] = [true, true, false, false];
const arr = a.map(x => () => x);
expect(binSearchLastTrue(arr)).toBe(1);
expect(linearSearchLastTrue(arr)).toBe(1);
});

it('[true, true, true, false]', () => {
const a: boolean[] = [true, true, true, false];
const arr = a.map(x => () => x);
expect(binSearchLastTrue(arr)).toBe(2);
expect(linearSearchLastTrue(arr)).toBe(2);
});

it('[true, true, true, true]', () => {
const a: boolean[] = [true, true, true, true];
const arr = a.map(x => () => x);
expect(binSearchLastTrue(arr)).toBe(3);
expect(linearSearchLastTrue(arr)).toBe(3);
});
});

0 comments on commit 87ce1a8

Please sign in to comment.