Skip to content

Commit

Permalink
Optimize check for duplicate parameters
Browse files Browse the repository at this point in the history
mergeParams is hot code because it gets called a lot in combinatoric
case parameter generation (sometimes redundantly). Optimize the check
for duplicate keys by moving it out of mergeParams.

On my machine, on a particularly heavily parameterized test:
`webgpu:api,validation,encoding,cmds,render,draw:vertex_buffer_OOB:*`
loadTreeForQuery goes from 360ms to 240ms with this change.

Bug: crbug.com/1470849
  • Loading branch information
kainino0x committed Aug 9, 2023
1 parent 92fcd38 commit e29fbed
Show file tree
Hide file tree
Showing 3 changed files with 47 additions and 26 deletions.
44 changes: 28 additions & 16 deletions src/common/framework/params_builder.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { Merged, mergeParams } from '../internal/params_utils.js';
import { Merged, assertMergedWithoutOverlap, mergeParams } from '../internal/params_utils.js';
import { stringifyPublicParams } from '../internal/query/stringify_params.js';
import { assert, mapLazy } from '../util/util.js';

Expand Down Expand Up @@ -150,7 +150,7 @@ export class CaseParamsBuilder<CaseP extends {}>
expandWithParams<NewP extends {}>(
expander: (_: Merged<{}, CaseP>) => Iterable<NewP>
): CaseParamsBuilder<Merged<CaseP, NewP>> {
const newGenerator = expanderGenerator(this.cases, expander);
const newGenerator = genExpandWithParams(this.cases, expander);
return new CaseParamsBuilder(() => newGenerator({}));
}

Expand All @@ -159,11 +159,8 @@ export class CaseParamsBuilder<CaseP extends {}>
key: NewPKey,
expander: (_: Merged<{}, CaseP>) => Iterable<NewPValue>
): CaseParamsBuilder<Merged<CaseP, { [name in NewPKey]: NewPValue }>> {
return this.expandWithParams(function* (p) {
for (const value of expander(p)) {
yield { [key]: value } as { readonly [name in NewPKey]: NewPValue };
}
});
const newGenerator = genExpand(this.cases, key, expander);
return new CaseParamsBuilder(() => newGenerator({}));
}

/** @inheritDoc */
Expand Down Expand Up @@ -256,20 +253,15 @@ export class SubcaseParamsBuilder<CaseP extends {}, SubcaseP extends {}>
expandWithParams<NewP extends {}>(
expander: (_: Merged<CaseP, SubcaseP>) => Iterable<NewP>
): SubcaseParamsBuilder<CaseP, Merged<SubcaseP, NewP>> {
return new SubcaseParamsBuilder(this.cases, expanderGenerator(this.subcases, expander));
return new SubcaseParamsBuilder(this.cases, genExpandWithParams(this.subcases, expander));
}

/** @inheritDoc */
expand<NewPKey extends string, NewPValue>(
key: NewPKey,
expander: (_: Merged<CaseP, SubcaseP>) => Iterable<NewPValue>
): SubcaseParamsBuilder<CaseP, Merged<SubcaseP, { [name in NewPKey]: NewPValue }>> {
return this.expandWithParams(function* (p) {
for (const value of expander(p)) {
// TypeScript doesn't know here that NewPKey is always a single literal string type.
yield { [key]: value } as { [name in NewPKey]: NewPValue };
}
});
return new SubcaseParamsBuilder(this.cases, genExpand(this.subcases, key, expander));
}

/** @inheritDoc */
Expand Down Expand Up @@ -300,14 +292,34 @@ export class SubcaseParamsBuilder<CaseP extends {}, SubcaseP extends {}>
}
}

function expanderGenerator<Base, A, B>(
function genExpandWithParams<Base, A, B>(
baseGenerator: (_: Base) => Generator<A>,
expander: (_: Merged<Base, A>) => Iterable<B>
): (_: Base) => Generator<Merged<A, B>> {
return function* (base: Base) {
for (const a of baseGenerator(base)) {
for (const b of expander(mergeParams(base, a))) {
yield mergeParams(a, b);
const merged = mergeParams(a, b);
assertMergedWithoutOverlap([a, b], merged);

yield merged;
}
}
};
}

function genExpand<Base, A, NewPKey extends string, NewPValue>(
baseGenerator: (_: Base) => Generator<A>,
key: NewPKey,
expander: (_: Merged<Base, A>) => Iterable<NewPValue>
): (_: Base) => Generator<Merged<A, { [k in NewPKey]: NewPValue }>> {
return function* (base: Base) {
for (const a of baseGenerator(base)) {
const before = mergeParams(base, a);
assert(!(key in before), () => `Key '${key}' already exists in ${JSON.stringify(before)}`);

for (const v of expander(before)) {
yield { ...a, [key]: v } as Merged<A, { [k in NewPKey]: NewPValue }>;
}
}
};
Expand Down
12 changes: 9 additions & 3 deletions src/common/internal/params_utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -119,9 +119,15 @@ export type MergedFromFlat<A, B> = {
[K in keyof A | keyof B]: K extends keyof B ? B[K] : K extends keyof A ? A[K] : never;
};

/** Merges two objects into one `{ ...a, ...b }` and return it with a flattened type. */
export function mergeParams<A extends {}, B extends {}>(a: A, b: B): Merged<A, B> {
for (const key of Object.keys(a)) {
assert(!(key in b), 'Duplicate key: ' + key);
}
return { ...a, ...b } as Merged<A, B>;
}

/** Asserts that the result of a mergeParams didn't have overlap. This is relatively slow. */
export function assertMergedWithoutOverlap([a, b]: [{}, {}], merged: {}): void {
assert(
Object.keys(merged).length === Object.keys(a).length + Object.keys(b).length,
() => `Duplicate key between ${JSON.stringify(a)} and ${JSON.stringify(b)}`
);
}
17 changes: 10 additions & 7 deletions src/unittests/params_builder_and_utils.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,11 @@ import {
builderIterateCasesWithSubcases,
} from '../common/framework/params_builder.js';
import { makeTestGroup } from '../common/framework/test_group.js';
import { mergeParams, publicParamsEquals } from '../common/internal/params_utils.js';
import {
assertMergedWithoutOverlap,
mergeParams,
publicParamsEquals,
} from '../common/internal/params_utils.js';
import { assert, objectEquals } from '../common/util/util.js';

import { UnitTest } from './unit_test.js';
Expand Down Expand Up @@ -348,7 +352,7 @@ g.test('invalid,shadowing').fn(t => {
yield { w: 5 };
}
});
// Iterating causes e.g. mergeParams({x:1}, {x:3}), which fails.
// Iterating causes merging e.g. ({x:1}, {x:3}), which fails.
t.shouldThrow('Error', () => {
Array.from(p.iterateCasesWithSubcases());
});
Expand All @@ -368,7 +372,7 @@ g.test('invalid,shadowing').fn(t => {
yield { w: 5 };
}
});
// Iterating causes e.g. mergeParams({x:1}, {x:3}), which fails.
// Iterating causes merging e.g. ({x:1}, {x:3}), which fails.
t.shouldThrow('Error', () => {
Array.from(p.iterateCasesWithSubcases());
});
Expand All @@ -394,14 +398,13 @@ g.test('invalid,shadowing').fn(t => {
assert(subcases !== undefined);
// Iterating subcases is fine...
for (const subcaseP of subcases) {
const merged = mergeParams(caseP, subcaseP);
if (caseP.a) {
assert(subcases !== undefined);
// Only errors once we try to e.g. mergeParams({x:1}, {x:3}).
// Only errors once we try to merge e.g. ({x:1}, {x:3}).
t.shouldThrow('Error', () => {
mergeParams(caseP, subcaseP);
assertMergedWithoutOverlap([caseP, subcaseP], merged);
});
} else {
mergeParams(caseP, subcaseP);
}
}
}
Expand Down

0 comments on commit e29fbed

Please sign in to comment.