Skip to content

Roll forward of "#51293"#51415

Closed
pmvald wants to merge 2 commits intoangular:mainfrom
pmvald:jit-rdt-rollout2
Closed

Roll forward of "#51293"#51415
pmvald wants to merge 2 commits intoangular:mainfrom
pmvald:jit-rdt-rollout2

Conversation

@pmvald
Copy link
Copy Markdown
Member

@pmvald pmvald commented Aug 17, 2023

After the merging of #51500 now this PR passes the aio local test and so it is ready to roll forward.

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@pmvald pmvald force-pushed the jit-rdt-rollout2 branch 2 times, most recently from 90aff10 to bee3801 Compare August 24, 2023 02:13
devversion added a commit to devversion/angular that referenced this pull request Aug 28, 2023
…ng, rxjs-interop)

Fixes that there was code duplication between the primary entry-point,
the testing entry-point and the rxjs-interop entry-point.

This code duplication resulted in additional code size (really
neglibible here because rxjs-interop did not duplicate large parts of
core, and `testing` is not used in production).

On the other hand though, the duplication resulted in a subtle JIT
dependency tracking issue due to the `depsTracker` no longer being a
singleton. This caused test failures as in:
angular#51415.
jessicajaniuk pushed a commit that referenced this pull request Aug 29, 2023
…ng, rxjs-interop) (#51500)

Fixes that there was code duplication between the primary entry-point,
the testing entry-point and the rxjs-interop entry-point.

This code duplication resulted in additional code size (really
neglibible here because rxjs-interop did not duplicate large parts of
core, and `testing` is not used in production).

On the other hand though, the duplication resulted in a subtle JIT
dependency tracking issue due to the `depsTracker` no longer being a
singleton. This caused test failures as in:
#51415.

PR Close #51500
@pmvald pmvald marked this pull request as ready for review August 29, 2023 22:58
@pullapprove pullapprove bot requested a review from jessicajaniuk August 29, 2023 22:58
@pmvald pmvald added action: review The PR is still awaiting reviews from at least one requested reviewer area: core Issues related to the framework runtime target: minor This PR is targeted for the next minor release PullApprove: disable compiler: jit labels Aug 29, 2023
@ngbot ngbot bot added this to the Backlog milestone Aug 29, 2023
@pmvald pmvald requested review from crisbeto and devversion and removed request for jessicajaniuk August 29, 2023 22:59
Copy link
Copy Markdown
Member

@devversion devversion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Original PR was already approved, so we can ignore pull approve

@devversion devversion removed the request for review from crisbeto August 30, 2023 08:07
@devversion devversion added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Aug 30, 2023
@pmvald pmvald added action: presubmit The PR is in need of a google3 presubmit and removed action: merge The PR is ready for merge by the caretaker labels Aug 30, 2023
@pmvald
Copy link
Copy Markdown
Member Author

pmvald commented Aug 30, 2023

Hmm, there is a new target that breaks: link to recent TGP I'll take a look and see if it is related or not. For now lets hold.

…racker

Using verification helpers such as `isComponent` may trigger JIT compilation. Now in some tests such compilation is made purposely to fail, and so in such cases any reference to the `depsTracker.clearScopeCacheFor` method will cause the exception to be thrown earlier than expected which results in teh test failure. Such scenario is the case in the next commit when we enable using the deps tracker in the jit compilation. Note that such failure is only for the framework tests and is a very edge case. The tests in downstream apps will not lead to such scenario of failure at all.
This change simply flip the flag which enables using the deps tracker in JIT compilation (the logic is already implemented in a previous PR). Some tests which depend on the old JIT implementation (e.g., patching the scope info into the type) are modified accordingly.
@pmvald
Copy link
Copy Markdown
Member Author

pmvald commented Sep 20, 2023

Ok, TGP is green now after merging #51791 link (the single failure is flaky, was able to pass the test locally)

@pmvald pmvald added action: merge The PR is ready for merge by the caretaker and removed action: presubmit The PR is in need of a google3 presubmit labels Sep 20, 2023
LayZeeDK pushed a commit to LayZeeDK/angular__angular that referenced this pull request Sep 20, 2023
…ng, rxjs-interop) (angular#51500)

Fixes that there was code duplication between the primary entry-point,
the testing entry-point and the rxjs-interop entry-point.

This code duplication resulted in additional code size (really
neglibible here because rxjs-interop did not duplicate large parts of
core, and `testing` is not used in production).

On the other hand though, the duplication resulted in a subtle JIT
dependency tracking issue due to the `depsTracker` no longer being a
singleton. This caused test failures as in:
angular#51415.

PR Close angular#51500
@dylhunn
Copy link
Copy Markdown
Contributor

dylhunn commented Sep 22, 2023

This PR was merged into the repository by commit 077534e.

@dylhunn dylhunn closed this in 654d6ce Sep 22, 2023
dylhunn pushed a commit that referenced this pull request Sep 22, 2023
This change simply flip the flag which enables using the deps tracker in JIT compilation (the logic is already implemented in a previous PR). Some tests which depend on the old JIT implementation (e.g., patching the scope info into the type) are modified accordingly.

PR Close #51415
@angular-automatic-lock-bot
Copy link
Copy Markdown

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Oct 23, 2023
ChellappanRajan pushed a commit to ChellappanRajan/angular that referenced this pull request Jan 23, 2024
…racker (angular#51415)

Using verification helpers such as `isComponent` may trigger JIT compilation. Now in some tests such compilation is made purposely to fail, and so in such cases any reference to the `depsTracker.clearScopeCacheFor` method will cause the exception to be thrown earlier than expected which results in teh test failure. Such scenario is the case in the next commit when we enable using the deps tracker in the jit compilation. Note that such failure is only for the framework tests and is a very edge case. The tests in downstream apps will not lead to such scenario of failure at all.

PR Close angular#51415
ChellappanRajan pushed a commit to ChellappanRajan/angular that referenced this pull request Jan 23, 2024
…r#51415)

This change simply flip the flag which enables using the deps tracker in JIT compilation (the logic is already implemented in a previous PR). Some tests which depend on the old JIT implementation (e.g., patching the scope info into the type) are modified accordingly.

PR Close angular#51415
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: merge The PR is ready for merge by the caretaker area: core Issues related to the framework runtime compiler: jit PullApprove: disable target: minor This PR is targeted for the next minor release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants