Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(provider-generator): namespace every resource / data source / provider #2087

Merged
merged 39 commits into from
Sep 29, 2022

Conversation

DanielMSchmidt
Copy link
Contributor

@DanielMSchmidt DanielMSchmidt commented Sep 5, 2022

This allows reducing the compile time across all languages since users only need to import
the used subset in the provider bindings instead of the entire one. Namespaces force JSII
to split the generated code up into multiple files that can be individually parsed and used.

I also put the provider in a namespaced and renamed the file from e.g. aws-provider.ts to provider.ts so that import paths are a bit nicer and namespaces are aws.provider.AwsProvider vs aws.awsProvider.AwsProvider.

BREAKING CHANGE: This changes the generated API surface a lot, if you use an already namespaced provider (read: AWS) you only need to import the provider declaration differently. For any other provider the imports are changing so that users would need to change the imports for their resources / data sources to use namespaces or barrell imports.

TODOs

  • Adjust convert to work with the namespaces
  • Make Unit Tests pass
    • hcl2cdk: data local_file
    • hcl2cdk: complex for each loops
    • hcl2cdk: data references
    • hcl2cdk: simple data source
    • hcl2cdk: null provider
    • hcl2cdk: convert project (succeeds but synth fails)
  • Fix Integration Tests
  • Fix Examples
  • Document breaking change and migration guide (docs follow-up issue)
  • Update all the docs (follow-up issue above)
  • Documentation: add "Use specific / namespaced imports" in best practice section (follow-up issue above)
  • Add doc string for each of the namespaces with a link to the TF docs (follow-up issue)

@jsteinich
Copy link
Collaborator

This is a tough one. I certainly understand the motivation for doing this; however, the usage is definitely less clean.

@DanielMSchmidt
Copy link
Contributor Author

DanielMSchmidt commented Sep 5, 2022

I measured through a subset of the examples to give us an idea of the improvements:

example get before get after get delta synth before synth after synth delta
Python AWS (namespaced) 137s 113s -17% 27s 22s -18%
Python Azure 35s 34s -2% 10s 8s -20%
C# AWS (namespaced) 80s 79s -1% 35s 16s -54%
C# Azure 66s 70s +6% 35s 14s -60%
Go AWS (namespaced) 341s 322s -5% 20s 15s -25%
Go Azure 247s 247s 0% 22s 9s -59%

@DanielMSchmidt
Copy link
Contributor Author

This is a tough one. I certainly understand the motivation for doing this; however, the usage is definitely less clean.

Yeah I agree, it's a trade-off. We will need to explore which APIs get worse / better and how much it impacts the compilation speed. In an ideal world I'd like to have the current API surface (and more) and fast synth / get times at the same time, but this seems to be impossible. This is one way we are looking into the issue, we'll also improve JSII directly, but that will most likely only affect the get times, not the synth times. One positive impact of this was autocompletion for python was working for the first time for me 🎉 I think the sub-modules make it easier for the language server to follow

@jsteinich
Copy link
Collaborator

Were the synth benchmarks for locally generated providers? Would be interesting to compare with the published bindings.

While JSII has some fundamental slowness to it, we've seen some glaring performance issues so I'm optimistic that there is room for significant improvement there.

Since this seems to be primarily an optimization, starting with profiling data to determine what aspects are slow should give more insight into the problem and better inform the choice.

@DanielMSchmidt
Copy link
Contributor Author

Were the synth benchmarks for locally generated providers? Would be interesting to compare with the published bindings.

Locally, I haven't tested pre-built yet, but I think we need to. in the old bindings there was already a stark contrast between pre-built and local providers.

While JSII has some fundamental slowness to it, we've seen some glaring performance issues so I'm optimistic that there is room for significant improvement there.

Yeah, we are also looking into the JSII performance, mostly for the cdktf get performance.

Since this seems to be primarily an optimization, starting with profiling data to determine what aspects are slow should give more insight into the problem and better inform the choice.

Do you mean profiling the synth operation?

@jsteinich
Copy link
Collaborator

Do you mean profiling the synth operation?

Yep. Should also profile the get operation, but that didn't feel like the focus of this PR.

@github-actions
Copy link
Contributor

I'm going to lock this pull request because it has been closed for 30 days. This helps our maintainers find and focus on the active issues. If you've found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
performance Gotta go fast
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants