SOAR-0015: Add namespace file splitting proposal - #930
Conversation
nac5504
changed the title
|
|
||
| - `output.types.fileSplitting.strategy`, currently supporting `namespace`. | ||
|
|
||
| The same model is used by YAML configuration, direct command-line invocation, and programmatic callers that construct `_OpenAPIGeneratorCore.Config` directly. |
There was a problem hiding this comment.
Please remove these implementation details. The design details should be about the conceptual approach and user-visible concepts.
There are many possible implementations, so no need to lock that in the proposal, in fact it can distract from where users should provide feedback - on the API and config file, not on the implementation.
|
|
||
| #### Build-tool plugin boundary | ||
|
|
||
| SwiftPM and Xcode build-tool plugins must declare generated output files before invoking the generator executable. PR #925 therefore rejects build-tool plugin invocations when `output.types.fileSplitting` is configured. |
There was a problem hiding this comment.
Hmm this is unfortunate - we should discuss more what our options are.
There was a problem hiding this comment.
Not being able to support this in the build plugin might actually be an argument for switching this to be the default and making this not even configurable. What do folks think?
There was a problem hiding this comment.
I think we should be open to moving to a prebuild plugin if it unlocks significant benefits. If we need a dynamic number of files generated then that's what prebuild plugins are advertised as the solution for.
|
|
||
| - `Types.swift` for root declarations. | ||
| - `Types+Components.swift` for the generated `Components` namespace. | ||
| - `Types+Operations.swift` for the generated `Operations` namespace. |
There was a problem hiding this comment.
I wonder if we should go one level deeper - one file for #/components/schemas, one file for #/components/parameters, etc.
There was a problem hiding this comment.
This is exactly the follow-up PR I am drafting. It provides a config option to specify if you want the first-level namespaces split or the second-level.
There was a problem hiding this comment.
We should discuss this more - whether and how much can actually be configurable, considering the limitation on build plugins.
There was a problem hiding this comment.
What speedup do we get from the next level. My original PoC centred around each section in components being split into its own extension, e.g. Components+Schemas.swift and Components+Parameters.swift and then further sharding these decl-wise, e.g. Components+Schemas.{0..N}.swift.
If we think there's a meaningful gain from the next level in the general case I think we just do it unilaterally. People are already opting into sharding and the fewest levers they have to pull to get maximum benefit the better. So I'd be inclined to have these phases:
- Shard based on namespace, but to the next level of granularity.
- Further shard these based on number of decls.
- Something more advanced.
(1) I think we could defend with few options. (2) we need to discuss as we probably need to move to a prebuild plugin.
There was a problem hiding this comment.
I like 1 at the next level - out of the box, no configuration. I'm hopeful that can bring a lot of benefits already without any user action (for build plugin users; users generating manually will just need to check in a few more files).
There was a problem hiding this comment.
In earlier tests you also had the GitHub OpenAPI doc - can you test that one as well?
These outputs are typically dominated in size by Types+Components+Schemas.swift, which is why I believe the further splitting has no effect.
This is only true for OpenAPI docs that don't have much content in #/components/parameters, #/components/requestBodies, and #/components/responses. But I've definitely worked with OpenAPI docs that have quite a lot of content there, and I believe GitHub has some as well.
I'm quite surprised by the error bars showing that this change made performance worse for the Stripe API, in debug builds (where the files are compiled in parallel). How could that be? Is that just measurement noise?
There was a problem hiding this comment.
Here is the full suite including Github's doc:
I'm quite surprised by the error bars showing that this change made performance worse for the Stripe API, in debug builds
I'm not sure which graph you're referring to here? The generator time or the swift compile time? Both splitting configs were statistically the same, the slightly longer build time for depth 2 splitting is just noise from the testing setup. In general, there could be some measurement noise since these runs exist on MacOS VMs and don't have full CPU usage, but results are averaged from 5 trials with outliers removed so it should be accurate.
There was a problem hiding this comment.
This is only true for OpenAPI docs that don't have much content in #/components/parameters, #/components/requestBodies, and #/components/responses. But I've definitely worked with OpenAPI docs that have quite a lot of content there, and I believe GitHub has some as well.
Looking at the size breakdown of Github's API spec, and it is still very heavily favored towards the schemas:
| File | LOC | Size |
|---|---|---|
| Types+Components+Headers.swift | 30 | 1.5 KiB |
| Types+Components+Parameters.swift | 608 | 35.5 KiB |
| Types+Components+RequestBodies.swift | 15 | 557 B |
| Types+Components+Responses.swift | 717 | 33.9 KiB |
| Types+Components+Schemas.swift | 323,319 | 20.8 MiB |
| Types+Components.swift | 13 | 496 B |
| Types+Operations.swift | 174,576 | 8.8 MiB |
| Types.swift | 23,993 | 1.8 MiB |
| Total | 523,271 | 31.4 MiB |
I'd imagine a spec with lots of headers, parameters, request bodies, responses, would certainly benefit from this depth 2 sharding. I'd be in favor of making this the default config to benefit those specs also.
There was a problem hiding this comment.
@simonjbeaumont @czechboy0 Just to clarify before I rewrite the proposal and the implementation PR, I will update to:
- Remove the splitting config entirely, and have the depth-2 splitting as the default
- Add a pre-build plugin to support SwiftPM build plugin users
Is there anything else worth changing?
There was a problem hiding this comment.
- Remove the splitting config entirely, and have the depth-2 splitting as the default
Yes.
- Add a pre-build plugin to support SwiftPM build plugin users
No. But call it out as a potential requirement in the future directions if we were to adopt further, non-determnistic sharding.
Is there anything else worth changing?
Nah, once you've done the above I think we can kick off the forums review and have some folks chime in there.
simonjbeaumont
left a comment
simonjbeaumont
left a comment
There was a problem hiding this comment.
3 participants
Summary
Types.swiftoutput.Notes
The proposal is scoped to the namespace split from PR #925:
Types.swift,Types+Components.swift, andTypes+Operations.swift,Types+Components+Schemas.swift, etc. More advanced sharding remains listed only as future direction.