perf: make generation 60x faster on large specs by reusing an existing schema index - #24953
Conversation
There was a problem hiding this comment.
2 issues found across 1 file
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java">
<violation number="1" location="modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java:681">
P2: When a model-name mapping or naming option changes after the first enum lookup, this cache still uses the old model names and can select the wrong schema or miss the schema entirely. Invalidate this cache whenever naming configuration changes, or include the naming configuration in the cache key.</violation>
<violation number="2" location="modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java:681">
P2: When two schema keys collapse to one model name and the first schema is not an object, TypeScript discriminator example generation now drops the discriminator mapping. `getDiscriminatorMappedModel` must resolve the mapped schema by its schema key, rather than using this ambiguous model-name index.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| // Create a cache to efficiently lookup schema based on model name. | ||
| Map<String, Schema> m = new HashMap<>(); | ||
| ModelUtils.getSchemas(openAPI).forEach((key, schema) -> m.put(toModelName(key), schema)); | ||
| ModelUtils.getSchemas(openAPI).forEach((key, schema) -> m.putIfAbsent(toModelName(key), schema)); |
There was a problem hiding this comment.
P2: When a model-name mapping or naming option changes after the first enum lookup, this cache still uses the old model names and can select the wrong schema or miss the schema entirely. Invalidate this cache whenever naming configuration changes, or include the naming configuration in the cache key.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java, line 681:
<comment>When a model-name mapping or naming option changes after the first enum lookup, this cache still uses the old model names and can select the wrong schema or miss the schema entirely. Invalidate this cache whenever naming configuration changes, or include the naming configuration in the cache key.</comment>
<file context>
@@ -670,14 +670,15 @@ private boolean codegenPropertyIsNew(CodegenModel model, CodegenProperty propert
- // Create a cache to efficiently lookup schema based on model name.
Map<String, Schema> m = new HashMap<>();
- ModelUtils.getSchemas(openAPI).forEach((key, schema) -> m.put(toModelName(key), schema));
+ ModelUtils.getSchemas(openAPI).forEach((key, schema) -> m.putIfAbsent(toModelName(key), schema));
modelNameToSchemaCache = Collections.unmodifiableMap(m);
}
</file context>
| // Create a cache to efficiently lookup schema based on model name. | ||
| Map<String, Schema> m = new HashMap<>(); | ||
| ModelUtils.getSchemas(openAPI).forEach((key, schema) -> m.put(toModelName(key), schema)); | ||
| ModelUtils.getSchemas(openAPI).forEach((key, schema) -> m.putIfAbsent(toModelName(key), schema)); |
There was a problem hiding this comment.
P2: When two schema keys collapse to one model name and the first schema is not an object, TypeScript discriminator example generation now drops the discriminator mapping. getDiscriminatorMappedModel must resolve the mapped schema by its schema key, rather than using this ambiguous model-name index.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java, line 681:
<comment>When two schema keys collapse to one model name and the first schema is not an object, TypeScript discriminator example generation now drops the discriminator mapping. `getDiscriminatorMappedModel` must resolve the mapped schema by its schema key, rather than using this ambiguous model-name index.</comment>
<file context>
@@ -670,14 +670,15 @@ private boolean codegenPropertyIsNew(CodegenModel model, CodegenProperty propert
- // Create a cache to efficiently lookup schema based on model name.
Map<String, Schema> m = new HashMap<>();
- ModelUtils.getSchemas(openAPI).forEach((key, schema) -> m.put(toModelName(key), schema));
+ ModelUtils.getSchemas(openAPI).forEach((key, schema) -> m.putIfAbsent(toModelName(key), schema));
modelNameToSchemaCache = Collections.unmodifiableMap(m);
}
</file context>
5e1e3eb to
17db0ee
Compare
|
Thanks — both looked at. Issue 2 is valid and I've fixed it. My first revision changed the index from Issue 1 is mitigated by call ordering. Re-verified after the change: full module suite still 5033 tests / 0 failures, and all four generator+spec combinations in the table still produce byte-identical output versus the parent commit. |
updateCodegenPropertyEnum resolved a property's referenced schema by streaming over every entry in components/schemas and recomputing toModelName() for each key until one matched. toModelName() is regex-heavy (sanitizeName + camelize), so each lookup cost O(schemas) in regex work. postProcessModelsEnum calls it eight times per model across overlapping var lists, so on large enum-heavy specs this dominated generation time. Sampling a typescript-fetch run on the Stripe spec, 117 of 119 JVM stack samples were in this method, nearly all in java.util.regex.Pattern.match. DefaultCodegen already builds exactly this index in getModelNameToSchemaCache(), but its only caller was TypeScriptClientCodegen, so every other generator paid the scan. Reuse it, unchanged. Measured against the parent commit, back to back, output compared by recursive diff and by checksum: typescript-fetch Stripe (8MB, 1454 schemas) 358.5s -> 5.3s 68x typescript-fetch GitHub (13MB, 977 schemas) 131.7s -> 4.2s 31x python Stripe 11s -> 9s go GitHub 8s -> 7s All four byte-identical. The TypeScript generators gain most because toTypescriptTypeName makes each toModelName() call expensive; other generators gain less and none regress. modelNameToSchemaCache is also cleared in setOpenAPI() so a reused codegen instance cannot serve a stale index. processOpts() runs before setOpenAPI() in DefaultGenerator, so naming options are fully applied before the index is built. Full openapi-generator module suite: 5033 tests, 0 failures. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
17db0ee to
c97befb
Compare
|
cc @wing328 @macjohnny @joscha @davidgamero for review. Flagging the TypeScript committee because that's where the measured impact is largest, though the change itself is in Summary for reviewers:
Happy to add a regression test or split out the |
macjohnny
left a comment
There was a problem hiding this comment.
thanks for this improvement
|
Excellent, LGTM! |
|
thanks for the PR cc @OpenAPITools/generator-core-team |
|
tested locally to confirm the improvement. thanks for the contribution. |
Generating a TypeScript client from the Stripe spec currently takes 6 minutes. This PR makes it 5 seconds, with byte-identical output, by calling an index that already exists in
DefaultCodegen.typescript-fetchtypescript-fetchpythongoMeasured against the parent commit, run back to back. Every output tree verified byte-identical by recursive
diffand by checksum.Cause
updateCodegenPropertyEnumfound a property's referenced schema by scanning every entry incomponents/schemas, recomputingtoModelName()on each key until one matched:toModelName()is regex-heavy (sanitizeName+camelize), so each lookup costs O(schemas) in regex work, andpostProcessModelsEnumcalls it 8x per model across overlapping var lists. Sampling atypescript-fetchrun on Stripe, 117 of 119 JVM stack samples were inside this method, nearly all injava.util.regex.Pattern.match. No output file was written until the last few seconds of the run.Fix
DefaultCodegenalready builds this index ingetModelNameToSchemaCache(), butTypeScriptClientCodegenwas its only caller, so every other generator paid the scan. This reuses it, unchanged. TypeScript generators gain most becausetoTypescriptTypeNamemakes eachtoModelName()call expensive; others gain less and none regress.Full module suite: 5033 tests, 0 failures, 12 skipped. No samples regenerated, since output is unchanged.
Note
modelNameToSchemaCachewas memoized and never invalidated;setOpenAPI()now clears it.processOpts()runs beforesetOpenAPI()inDefaultGenerator, so naming options are fully applied before the index is built.PR checklist
perf: <summary>.master.