Skip to content

Commit

Permalink
⚡️ Faster canShrinkWithoutContext for constants (#5372)
Browse files Browse the repository at this point in the history
**Description**

<!-- Please provide a short description and potentially linked issues
justifying the need for this PR -->

Make execution of `canShrinkWithoutContext` faster for `constantFrom`
cases. Execution should be slightly similar for `constant` (if not a bit
faster) and faster for `constantFrom` as soon as it gets used for
multiple values (the more values, the faster it gets compared to the
previous implementation).

<!-- * Your PR is fixing a bug or regression? Check for existing issues
related to this bug and link them -->
<!-- * Your PR is adding a new feature? Make sure there is a related
issue or discussion attached to it -->

<!-- You can provide any additional context to help into understanding
what's this PR is attempting to solve: reproduction of a bug, code
snippets... -->

**Checklist** — _Don't delete this checklist and make sure you do the
following before opening the PR_

- [x] The name of my PR follows [gitmoji](https://gitmoji.dev/)
specification
- [x] My PR references one of several related issues (if any)
- [x] New features or breaking changes must come with an associated
Issue or Discussion
- [x] My PR does not add any new dependency without an associated Issue
or Discussion
- [x] My PR includes bumps details, please run `yarn bump` and flag the
impacts properly
- [x] My PR adds relevant tests and they would have failed without my PR
(when applicable)

<!-- More about contributing at
https://github.com/dubzzz/fast-check/blob/main/CONTRIBUTING.md -->

**Advanced**

<!-- How to fill the advanced section is detailed below! -->

- [x] Category: ⚡️ Performance
- [x] Impacts: Faster

<!-- [Category] Please use one of the categories below, it will help us
into better understanding the urgency of the PR -->
<!-- * ✨ Introduce new features -->
<!-- * 📝 Add or update documentation -->
<!-- * ✅ Add or update tests -->
<!-- * 🐛 Fix a bug -->
<!-- * 🏷️ Add or update types -->
<!-- * ⚡️ Improve performance -->
<!-- * _Other(s):_ ... -->

<!-- [Impacts] Please provide a comma separated list of the potential
impacts that might be introduced by this change -->
<!-- * Generated values: Can your change impact any of the existing
generators in terms of generated values, if so which ones? when? -->
<!-- * Shrink values: Can your change impact any of the existing
generators in terms of shrink values, if so which ones? when? -->
<!-- * Performance: Can it require some typings changes on user side?
Please give more details -->
<!-- * Typings: Is there a potential performance impact? In which cases?
-->
  • Loading branch information
dubzzz authored Oct 27, 2024
1 parent 75d3ffa commit f893f50
Show file tree
Hide file tree
Showing 4 changed files with 117 additions and 34 deletions.
5 changes: 5 additions & 0 deletions .changeset/curvy-dingos-sort.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"fast-check": minor
---

⚡️ Faster `canShrinkWithoutContext` for constants
47 changes: 42 additions & 5 deletions packages/fast-check/src/arbitrary/_internals/ConstantArbitrary.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,14 @@ import { Stream } from '../../stream/Stream';
import { Arbitrary } from '../../check/arbitrary/definition/Arbitrary';
import { Value } from '../../check/arbitrary/definition/Value';
import { cloneMethod, hasCloneMethod } from '../../check/symbols';
import { Set, safeHas } from '../../utils/globals';

const safeObjectIs = Object.is;

/** @internal */
export class ConstantArbitrary<T> extends Arbitrary<T> {
private fastValues: FastConstantValuesLookup<T> | undefined;

constructor(readonly values: T[]) {
super();
}
Expand All @@ -20,12 +23,13 @@ export class ConstantArbitrary<T> extends Arbitrary<T> {
return new Value(value, idx, () => value[cloneMethod]());
}
canShrinkWithoutContext(value: unknown): value is T {
for (let idx = 0; idx !== this.values.length; ++idx) {
if (safeObjectIs(this.values[idx], value)) {
return true;
}
if (this.values.length === 1) {
return safeObjectIs(this.values[0], value);
}
return false;
if (this.fastValues === undefined) {
this.fastValues = new FastConstantValuesLookup(this.values);

Check warning on line 30 in packages/fast-check/src/arbitrary/_internals/ConstantArbitrary.ts

View workflow job for this annotation

GitHub Actions / Format & Lint

'FastConstantValuesLookup' was used before it was defined
}
return this.fastValues.has(value);
}
shrink(value: T, context?: unknown): Stream<Value<T>> {
if (context === 0 || safeObjectIs(value, this.values[0])) {
Expand All @@ -34,3 +38,36 @@ export class ConstantArbitrary<T> extends Arbitrary<T> {
return Stream.of(new Value(this.values[0], 0));
}
}

/** @internal */
class FastConstantValuesLookup<T> {
private readonly hasMinusZero: boolean;
private readonly hasPlusZero: boolean;
private readonly fastValues: Set<unknown>;

constructor(readonly values: T[]) {
this.fastValues = new Set(this.values);

let hasMinusZero = false;
let hasPlusZero = false;
if (safeHas(this.fastValues, 0)) {
for (let idx = 0; idx !== this.values.length; ++idx) {
const value = this.values[idx];
hasMinusZero = hasMinusZero || safeObjectIs(value, -0);
hasPlusZero = hasPlusZero || safeObjectIs(value, 0);
}
}
this.hasMinusZero = hasMinusZero;
this.hasPlusZero = hasPlusZero;
}

has(value: unknown): value is T {
if (value === 0) {
if (safeObjectIs(value, 0)) {
return this.hasPlusZero;
}
return this.hasMinusZero;
}
return safeHas(this.fastValues, value);
}
}
14 changes: 14 additions & 0 deletions packages/fast-check/src/utils/globals.ts
Original file line number Diff line number Diff line change
Expand Up @@ -268,19 +268,33 @@ export function safeToISOString(instance: Date): string {
// Set

const untouchedAdd = Set.prototype.add;
const untouchedHas = Set.prototype.has;
function extractAdd(instance: Set<unknown>) {
try {
return instance.add;
} catch (err) {
return undefined;
}
}
function extractHas(instance: Set<unknown>) {
try {
return instance.has;
} catch (err) {
return undefined;
}
}
export function safeAdd<T>(instance: Set<T>, value: T): Set<T> {
if (extractAdd(instance) === untouchedAdd) {
return instance.add(value);
}
return safeApply(untouchedAdd, instance, [value]);
}
export function safeHas<T>(instance: Set<T>, value: T): boolean {
if (extractHas(instance) === untouchedHas) {
return instance.has(value);
}
return safeApply(untouchedHas, instance, [value]);
}

// String

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ describe('ConstantArbitrary', () => {
});

describe('canShrinkWithoutContext', () => {
it("should mark value as 'canShrinkWithoutContext' whenever one of the original values is equal regarding Object.is", () =>
it("should mark value as 'canShrinkWithoutContext' whenever one of the original values is equal regarding Object.is", () => {
fc.assert(
fc.property(fc.array(fc.anything(), { minLength: 1 }), fc.nat(), (values, mod) => {
// Arrange
Expand All @@ -126,7 +126,24 @@ describe('ConstantArbitrary', () => {
// Assert
expect(out).toBe(true);
}),
));
);
});

it("should not mark value as 'canShrinkWithoutContext' if none of the original values is equal regarding Object.is", () => {
fc.assert(
fc.property(fc.uniqueArray(fc.anything(), { minLength: 2, comparator: 'SameValue' }), (values) => {
// Arrange
const [selectedValue, ...acceptedValues] = values;

// Act
const arb = new ConstantArbitrary(acceptedValues);
const out = arb.canShrinkWithoutContext(selectedValue); // selectedValue is unique for SameValue comparison (Object.is)

// Assert
expect(out).toBe(false);
}),
);
});

it('should not detect values not equal regarding to Object.is', () => {
// Arrange
Expand All @@ -141,33 +158,43 @@ describe('ConstantArbitrary', () => {
expect(out).toBe(false); // Object.is([], []) is falsy
});

it.each([{ source: -0 }, { source: 0 }, { source: 48 }])(
'should not accept to shrink -$source if built with $source',
({ source }) => {
// Arrange
const arb = new ConstantArbitrary([source]);

// Act
const out = arb.canShrinkWithoutContext(-source);

// Assert
expect(out).toBe(false);
},
);

it.each([{ source: -0 }, { source: 0 }, { source: 48 }, { source: Number.NaN }])(
'should accept to shrink $source if built with $source',
({ source }) => {
// Arrange
const arb = new ConstantArbitrary([source]);

// Act
const out = arb.canShrinkWithoutContext(source);

// Assert
expect(out).toBe(true);
},
);
it.each([
{ source: -0, count: 1 },
{ source: 0, count: 1 },
{ source: 48, count: 1 },
{ source: -0, count: 25 },
{ source: 0, count: 25 },
{ source: 48, count: 25 },
])('should not accept to shrink -$source if built with $source (count: $count)', ({ source, count }) => {
// Arrange
const arb = new ConstantArbitrary(Array(count).fill(source));

// Act
const out = arb.canShrinkWithoutContext(-source);

// Assert
expect(out).toBe(false);
});

it.each([
{ source: -0, count: 1 },
{ source: 0, count: 1 },
{ source: 48, count: 1 },
{ source: Number.NaN, count: 1 },
{ source: -0, count: 25 },
{ source: 0, count: 25 },
{ source: 48, count: 25 },
{ source: Number.NaN, count: 25 },
])('should accept to shrink $source if built with $source (count: $count)', ({ source, count }) => {
// Arrange
const arb = new ConstantArbitrary(Array(count).fill(source));

// Act
const out = arb.canShrinkWithoutContext(source);

// Assert
expect(out).toBe(true);
});
});

describe('shrink', () => {
Expand Down

0 comments on commit f893f50

Please sign in to comment.