[Roman3349]

Description

ErrorBag::addErrors() and BaseConstraint::addErrors() merged new errors with array_merge(), which copies every error collected so far. Constraints such as items and properties add the errors of their sub-schemas one by one, so collecting n errors took O(n²) time.

A failing oneOf branch over a long array is enough to hit this. The geojson corpus of jsonschema-benchmark validates FeatureCollections against a oneOf of the geometry types, and every branch except the matching one reports an error for each coordinate. Its largest instance (11 MB) took 194 s in strict mode, 49 times as long as an instance a fifth of its size. Profiling showed the extra time in addErrors().

The errors are now appended in place. For lists of errors (ErrorList), which is what every caller in the library passes, the result is the same as before: the same errors in the same order, keyed 0..n-1, and the same error mask. The only difference is for an array with string keys, which array_merge() kept (overwriting an earlier error with the same key) and which is now renumbered.

Without strict mode, the legacy validator had a second quadratic path: CollectionConstraint::validateItems() copied the errors collected so far before checking each item, to restore them if the item matched additionalItems. It now remembers the number of errors and truncates on restore. The legacy validator is not an implementation of Draft 3/4 (or any other draft, see #991), so this speeds up normal mode as a whole rather than a particular draft.

Measured on the geojson corpus in strict mode (one run, PHP 8.4):

	Before	After
6.14.0, all 500 instances	350 s	162 s

How to reproduce

The largest instance of the geojson corpus, pinned to the commit of jsonschema-benchmark it was measured with:

C=91e04afac6598cfa54cd19742431acb669e050f7
BASE=https://raw.githubusercontent.com/sourcemeta-research/jsonschema-benchmark/$C/schemas/geojson
curl -sSfL -o geojson-schema.json "$BASE/schema.json"
curl -sSfL "$BASE/instances.jsonl" | sed -n 426p > geojson-425.json  # 11 MB

cat > repro.php <<'EOF'
<?php
require __DIR__ . '/vendor/autoload.php';

use JsonSchema\Constraints\Constraint;
use JsonSchema\Constraints\Factory;
use JsonSchema\Validator;

$schema = json_decode(file_get_contents($argv[1]));
$instance = json_decode(file_get_contents($argv[2]));

$validator = new Validator(new Factory(null, null, Constraint::CHECK_MODE_STRICT));
$start = hrtime(true);
$validator->validate($instance, $schema);
printf("%.2f s, %s\n", (hrtime(true) - $start) / 1e9, $validator->isValid() ? 'valid' : 'invalid');
EOF

Then, in a checkout of this repository with composer install done, run it on main and on this branch:

XDEBUG_MODE=off php -d pcov.enabled=0 -d memory_limit=-1 repro.php geojson-schema.json geojson-425.json

Turning off Xdebug and PCOV matters, as they slow the validation down several times over (Xdebug in develop mode about seven times, PCOV about two times) and the difference gets lost.

On my machine (PHP 8.5, Xdebug and PCOV off), main takes 244.5 s and this branch 66.9 s. Both report the instance as valid.

Related Issue

None

Type of Change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Documentation update
• Code refactoring
• Other (please describe):

Checklist

• I have read the CONTRIBUTING guidelines
• My code follows the code style of this project
• I have added tests that prove my fix is effective or that my feature works
• All new and existing tests pass
• I have updated the documentation accordingly
• My changes generate no new warnings

Additional Notes

The new test pins the errors reported for items with additionalItems, the only behaviour the second commit touches. No timing assertion is added, as it would be unreliable on CI.

[DannyvdSluijs]

@Roman3349 really nice to see this improvement. This is saving quite some time on larger schema instances. It seems the gains only apply to where the strict mode is being used, which by itself is already a reason to merge this PR.

Although the description seems to hint the benefits apply to draft 3 and 4 as well. My assumption is that this is still the mostly used drafts. I was wondering how certain you are this would benefit Draft 3/4?

[DannyvdSluijs]

Thanks for this. The fix does what it says for the per-draft constraints (strict mode). The Draft 3/4 path (normal mode, CollectionConstraint/UndefinedConstraint) is still quadratic, though, and changing how addErrors() merges can't fix that.

Why

CollectionConstraint::validateItems() saves a copy of the errors before each item:

// src/JsonSchema/Constraints/CollectionConstraint.php:76-80
foreach ($value as $k => &$v) {
    $initErrors = $this->getErrors();                       // second handle on $this->errors
    $this->checkUndefined($v, $schema->items, $path, $k);   // -> addErrors() -> $this->errors[] = ...

getErrors() returns $this->errors itself, so while $initErrors holds it, the first $this->errors[] = $error in addErrors() has to copy the whole array. That's still O(errors so far) per item, the same cost array_merge() had. The Draft07 ItemsConstraint saves no copy, which is why only that path got faster.

Benchmark

An array of n integers against {"$schema": ..., "type": "array", "items": {"type": "string"}}, so every item fails. PHP 8.4, Xdebug and PCOV off, one run each. Draft-04 runs in normal mode, draft-07 in strict mode. The last column is this PR plus the diff below.

	n	main	this PR	PR + diff below
draft-04	20k	1.54 s	1.46 s	0.50 s
	40k	6.76 s	5.69 s	0.93 s
	80k	55.98 s	44.72 s	2.06 s
draft-07 strict	20k	1.88 s	0.51 s	n/a (path not touched)
	40k	8.86 s	1.02 s
	80k	62.28 s	2.13 s

Benchmark script

<?php
require __DIR__ . '/vendor/autoload.php';

use JsonSchema\Constraints\Constraint;
use JsonSchema\Validator;

foreach (['draft-04' => Constraint::CHECK_MODE_NORMAL, 'draft-07' => Constraint::CHECK_MODE_STRICT] as $d => $mode) {
    foreach ([20000, 40000, 80000] as $n) {
        $schema = json_decode(json_encode(['$schema' => "http://json-schema.org/$d/schema#", 'type' => 'array', 'items' => ['type' => 'string']]));
        $data = range(1, $n);
        $v = new Validator();
        $t = hrtime(true);
        $v->validate($data, $schema, $mode);
        printf("%s n=%-6d %6.2fs errs=%d\n", $d, $n, (hrtime(true) - $t) / 1e9, count($v->getErrors()));
    }
}

Possible direction (not complete)

Keeping a count instead of a copy is enough for the plain items case:

             foreach ($value as $k => &$v) {
-                $initErrors = $this->getErrors();
+                $initCount = count($this->errors);
 
                 // First check if its defined in "items"
                 $this->checkUndefined($v, $schema->items, $path, $k);
 
                 // Recheck with "additionalItems" if the first test fails
-                if (count($initErrors) < count($this->getErrors()) && (isset($schema->additionalItems) && $schema->additionalItems !== false)) {
+                if ($initCount < count($this->errors) && (isset($schema->additionalItems) && $schema->additionalItems !== false)) {

This diff is only the proof of concept I used for the benchmark. It is not ready to apply:

• The restore at line 92 ($this->errors = $initErrors;) still uses the old variable, so it needs to truncate to the count instead (e.g. array_splice($this->errors, $initCount)), and it needs a test with additionalItems.
• $secondErrors at line 84 still saves a full copy.
• UndefinedConstraint has the same pattern for disallow (:164), not (:179), allOf (:323), anyOf (:334/:338) and oneOf (:360), plus array_merge in a loop at :372.
• I haven't run the test suite on it.

Would you like to take this on in this PR, or should we merge this one as is (with the description noting it covers the strict / per-draft path) and handle Draft 3/4 in a follow-up?

[Roman3349]

Thanks for the thorough analysis and the benchmark, I could reproduce it.

One clarification first: the normal-mode path isn't a Draft 3/4 implementation. It's the legacy validator (UndefinedConstraint, CollectionConstraint, …), a mix of rules from several drafts that isn't compliant with any of them, and $schema doesn't change which rules it applies. #991 has a few examples. There are no per-draft validators for Draft 3/4 yet (a Draft-04 schema in strict mode throws Unknown constraint draft04), so the first commit didn't speed up Draft 3/4 either. It covered the per-draft validators used in strict mode (Draft 6, 7 and 2019-09). In #991 I proposed adding a Draft-04 constraint set for strict mode on top of the deduplicated per-draft classes, which would give Draft-04 schemas correct semantics as well.

That said, you're right that the legacy validator stayed quadratic, and the fix turned out to be small, so I added it to this PR (bf8bba5). Only CollectionConstraint::validateItems() needed it. It now remembers the number of errors instead of a copy and truncates with array_splice() on restore, which also covers the $secondErrors snapshot. The snapshots in UndefinedConstraint (disallow, not, allOf, anyOf, oneOf) aren't quadratic in the size of the instance. checkUndefined() creates a new constraint instance for every sub-schema, so they only copy the errors of a single schema node, and the number of copies depends on the schema, not on the length of the array.

The new test pins the reported errors (pointer and constraint) for items combined with additionalItems. It passes on the code before and after the change.

Your benchmark, PHP 8.5, Xdebug and PCOV off:

	n	main	first commit	this PR
legacy (normal mode)	10k	0.67 s	0.66 s	0.12 s
	20k	2.43 s	2.35 s	0.24 s
draft-07 strict	10k	1.05 s	0.34 s	0.35 s
	20k	2.82 s	0.68 s	0.67 s