feat(rosetta): reuse translation results from a cache #3101

rix0rrr · 2021-10-27T16:19:50Z

By fingerprinting types in an assembly and storing the fingerprints along with translations, we can reuse existing translations if we know nothing changed.

A snippet needs to be recompiled if:

Its source text changes: hash key will be different
Its fixture changes: fullSource will be different
The referenced types have changed: fingerprint will be different
The translator's capabilities have changed: translation version will be different

Since we have many duplicate snippets (especially cluster.enableSingleUserRotation() crops up a lot 🙂), I've had to encode the source location for the snippet into the key. Since that means that location encoding is now significant and needs to be correct, some refactoring around encoding snippet locations has become important.

Incidentally, this will also fix broken translations originating from monocdk from overwriting perfectly cromulent translations in the @aws-cdk packages.

Refactorings in this PR:

Snippet source locations are no longer a semi-free form string -- they are now a structured data type that captures the location accurately (and incidentally gets rendered to a filename-like string for interaction with the TS compiler and human display).
Snippet source locations now need to be passed in every place where some code is passed to Rosetta. These take the form of { api: 'member', fqn: '@ns/package.type', memberName: 'bucketName' } (an ApiLocation), and have been added to many, many places, including jsii-pacmak.
Translators now have a VERSION, which is used to invalidate cached translations if we change something about the implementation of the translator.
Existing tablet files may now need to span multiple jsii release versions, so they are now validated based on their schema version instead of the version of the tool that produced them.
TranslatedSnippet had a copy of all the data in a TranslatedSnippetSchema, and was copying back and forth. Change it to just hold on to a TranslatedSnippet internally. It now represents the operations that can be done to a TranslatedSnippetSchema.
The translation of ts.Diagnostic to RosettaDiagnostic is now done even earlier (closer to the code that deals with the TS compiler).
Snippets are no longer in an IterableIterator, but just an plain old array.
Snippets in Markdown are no longer identified by their snippet counter, but by their line number.
Mutable fields turned into readonly fields on a number of structs.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

…long with translations, we can reuse existing translations if we know nothing changed. A snippet needs to be recompiled if: - Its source text changes: hash will be different - Its fixture changes: fullSource will be different - The referenced types have changed: fingerprint will be different

…iagnostic down more in favor of RosettaDiagnostic

mergify · 2021-11-02T12:50:33Z

The title of this Pull Request does not conform with [Conventional Commits] guidelines. It will need to be adjusted before the PR can be merged.
[Conventional Commits]: https://www.conventionalcommits.org

njlynch

Honestly, my eyes did glaze over a bit here at one point, but I think I was paying attention for all the salient points.

🤞

njlynch · 2021-11-02T13:59:45Z

packages/jsii-rosetta/lib/commands/extract.ts

 }

 /**
 * Translate all snippets
 *
 * We are now always using workers, as we are targeting Node 12+.
 */
-async function translateAll(
-  snippets: IterableIterator<TypeScriptSnippet>,
+export async function translateAll(


Is this just exported for testing? Might be worth a comment if so.

It was originally, no longer needs to be. Good catch.

njlynch · 2021-11-02T14:30:15Z

packages/jsii-rosetta/lib/languages/csharp.ts

+   * Bump this when you change something in the implementation to invalidate
+   * existing cached translations.
+   */
+  public static readonly VERSION = '1';


Food for (future) thought -- this feels very much like a good intentions thing. Could we mechanize this a bit better somehow?

Maybe. Not sure!

njlynch · 2021-11-02T14:40:28Z

packages/jsii-rosetta/lib/tablets/tablets.ts

-  public get didCompile() {
-    return this._didCompile;
-  }
+  public readonly snippet: TranslatedSnippetSchema;


~ The naming here is a bit odd. TranslatedSnippet.snippet, which returns a TranslatedSnippetSchema, which reads to me like the snippet itself. It leads to a lot of snippet.snippet in the code, which is never great...

It's almost like TranslatedSnippet should implement TranslatedSnippetSchema. Am I off there?

Maybe the names would be better to say TranslatedSnippetOperations and TranslatedSnippet. The class represents the operations on the pure-data object.

But I do need the ability to get the pure-data object out in a lightweight way.

mergify · 2021-11-02T14:52:43Z

Thank you for contributing! ❤️ I will now look into making sure the PR is up-to-date, then proceed to try and merge it!

mergify · 2021-11-02T14:52:54Z

Merging (with squash)...

mergify · 2021-11-02T16:12:24Z

Merging (with squash)...

mergify · 2021-11-02T17:09:36Z

Merging (with squash)...

rix0rrr self-assigned this Oct 27, 2021

mergify bot added the contribution/core This is a PR that came from AWS. label Oct 27, 2021

rix0rrr force-pushed the huijbers/rosetta-fingerprinting branch from 28d29fc to 5399c71 Compare October 27, 2021 16:25

rix0rrr changed the title ~~feat(rosetta): reuse compilation results from a cache~~ feat(rosetta): reuse translation results from a cache Oct 27, 2021

rix0rrr added 8 commits November 1, 2021 11:19

Get rid of state duplication around TranslatedSnippet and push ts.D…

1581d98

…iagnostic down more in favor of RosettaDiagnostic

Add caching

97993dc

Add in default lang

3faec82

Fix linter error

8c382ff

Make caching work

6d00c59

Include source location in cache key

6983932

Refactor snippet locations

1ce2bb5

rix0rrr force-pushed the huijbers/rosetta-fingerprinting branch from 23747dc to 6df1bfe Compare November 1, 2021 10:20

Pulling changes through to pacmak (WIP)

2901b56

rix0rrr force-pushed the huijbers/rosetta-fingerprinting branch from 6df1bfe to 2901b56 Compare November 1, 2021 13:00

rix0rrr added 5 commits November 1, 2021 16:27

Take implementation changes in rosetta into account

4f0f222

Add docs around public API

2252058

Print filename

e43cd1a

Also remove backslashes

2266f10

Add tests

7735761

rix0rrr marked this pull request as ready for review November 2, 2021 12:50

rix0rrr requested a review from a team November 2, 2021 12:50

njlynch approved these changes Nov 2, 2021

View reviewed changes

mergify bot added the pr/ready-to-merge This PR is ready to be merged. label Nov 2, 2021

mergify bot and others added 2 commits November 2, 2021 14:53

Merge branch 'main' into huijbers/rosetta-fingerprinting

2148cb8

Update extract.ts

fd49f67

Merge branch 'main' into huijbers/rosetta-fingerprinting

2123ed5

mergify bot merged commit b08a50e into main Nov 2, 2021

mergify bot deleted the huijbers/rosetta-fingerprinting branch November 2, 2021 17:09

mergify bot removed the pr/ready-to-merge This PR is ready to be merged. label Nov 2, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(rosetta): reuse translation results from a cache #3101

feat(rosetta): reuse translation results from a cache #3101

rix0rrr commented Oct 27, 2021 •

edited

Loading

mergify bot commented Nov 2, 2021

njlynch left a comment

njlynch Nov 2, 2021

rix0rrr Nov 2, 2021

njlynch Nov 2, 2021

rix0rrr Nov 2, 2021

njlynch Nov 2, 2021

rix0rrr Nov 2, 2021

mergify bot commented Nov 2, 2021

mergify bot commented Nov 2, 2021

mergify bot commented Nov 2, 2021

mergify bot commented Nov 2, 2021

feat(rosetta): reuse translation results from a cache #3101

feat(rosetta): reuse translation results from a cache #3101

Conversation

rix0rrr commented Oct 27, 2021 • edited Loading

mergify bot commented Nov 2, 2021

njlynch left a comment

Choose a reason for hiding this comment

njlynch Nov 2, 2021

Choose a reason for hiding this comment

rix0rrr Nov 2, 2021

Choose a reason for hiding this comment

njlynch Nov 2, 2021

Choose a reason for hiding this comment

rix0rrr Nov 2, 2021

Choose a reason for hiding this comment

njlynch Nov 2, 2021

Choose a reason for hiding this comment

rix0rrr Nov 2, 2021

Choose a reason for hiding this comment

mergify bot commented Nov 2, 2021

mergify bot commented Nov 2, 2021

mergify bot commented Nov 2, 2021

mergify bot commented Nov 2, 2021

rix0rrr commented Oct 27, 2021 •

edited

Loading