Sitelet https://web.archive.org/web/20211020052222/https://github.com/angular/angular/pull/36687
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

fix(ngcc): do not use cached file-system #36687

Closed

Conversation

@petebacondarwin
Copy link
Member

@petebacondarwin petebacondarwin commented Apr 17, 2020

The cached file-system was implemented to speed up ngcc
processing, but in reality most files are not accessed many times
and there is no noticeable degradation in speed by removing it.

Benchmarking ngcc -l debug for AIO on a local machine
gave a range of 196-236 seconds with the cache and 197-224
seconds without the cache.

Moreover, when running in parallel mode, ngcc has a separate
file cache for each process. This results in excess memory usage.
Notably the master process, which only does analysis of entry-points
holds on to up to 500Mb for AIO when using the cache compared to
only around 30Mb when not using the cache.

Finally, the file-system cache being incorrectly primed with file
contents before being processed has been the cause of a number
of bugs. For example angular/angular-cli#16860 (comment).

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

The cached file-system was implemented to speed up ngcc
processing, but in reality most files are not accessed many times
and there is no noticeable degradation in speed by removing it.

Benchmarking `ngcc -l debug` for AIO on a local machine
gave a range of 196-236 seconds with the cache and 197-224
seconds without the cache.

Moreover, when running in parallel mode, ngcc has a separate
file cache for each process. This results in excess memory usage.
Notably the master process, which only does analysis of entry-points
holds on to up to 500Mb for AIO when using the cache compared to
only around 30Mb when not using the cache.

Finally, the file-system cache being incorrectly primed with file
contents before being processed has been the cause of a number
of bugs. For example angular/angular-cli#16860 (comment).
Copy link
Member

@JoostK JoostK left a comment

There's a remaining comment in integration/ngcc/test.sh that refers to CachedFileSystem

@pullapprove pullapprove bot requested a review from kara Apr 17, 2020
JoostK
JoostK approved these changes Apr 17, 2020
Copy link
Member

@gkalpak gkalpak left a comment

Awesome! Very excited about the memory savings 🎉

integration/ngcc/test.sh Show resolved Hide resolved
kara
kara approved these changes Apr 17, 2020
Copy link
Contributor

@kara kara left a comment

LGTM for integration/

matsko added a commit that referenced this issue Apr 17, 2020
The cached file-system was implemented to speed up ngcc
processing, but in reality most files are not accessed many times
and there is no noticeable degradation in speed by removing it.

Benchmarking `ngcc -l debug` for AIO on a local machine
gave a range of 196-236 seconds with the cache and 197-224
seconds without the cache.

Moreover, when running in parallel mode, ngcc has a separate
file cache for each process. This results in excess memory usage.
Notably the master process, which only does analysis of entry-points
holds on to up to 500Mb for AIO when using the cache compared to
only around 30Mb when not using the cache.

Finally, the file-system cache being incorrectly primed with file
contents before being processed has been the cause of a number
of bugs. For example angular/angular-cli#16860 (comment).

PR Close #36687
matsko added a commit that referenced this issue Apr 17, 2020
This was only being used by ngcc but not any longer.

PR Close #36687
@matsko matsko closed this in 0c2ed4c Apr 17, 2020
matsko added a commit that referenced this issue Apr 17, 2020
This was only being used by ngcc but not any longer.

PR Close #36687
@petebacondarwin petebacondarwin deleted the ngcc-no-filecache branch Apr 18, 2020
peruukki added a commit to peruukki/angular that referenced this issue Apr 25, 2020
angular#36687)

Default to using DOMParser if it is available and fall back to
createDocument if needed. This is the approach used by DOMPurify and
suggested in the related Angular.js pull request
angular/angular.js#17013. It also safely
avoids using an inline style tag that causes CSP violation errors if inline
CSS is prohibited.

The related unit tests in `html_sanitizer_spec.ts`, "should not allow
JavaScript execution when creating inert document" and "should not allow
JavaScript hidden in badly formed HTML to get through sanitization (Firefox
bug)", are left untouched to assert that the behavior hasn't changed in
those scenarios.

Fixes angular#25214.
peruukki added a commit to peruukki/angular that referenced this issue Apr 25, 2020
…6687)

The `inertDocument` member is only needed when using the InertDocument
strategy. By separating the DOMParser and InertDocument strategies into
separate classes, we can easily avoid creating the inert document
unnecessarily when using DOMParser.
peruukki added a commit to peruukki/angular that referenced this issue Apr 25, 2020
…#36687)

Verify that HTML parsing is supported in addition to DOMParser existence.
This maybe wasn't as important before when DOMParser was used just as a
fallback on Firefox, but now that DOMParser is the default choice, we need
to be more accurate.
peruukki added a commit to peruukki/angular that referenced this issue Apr 25, 2020
…#36687)

Verify that HTML parsing is supported in addition to DOMParser existence.
This maybe wasn't as important before when DOMParser was used just as a
fallback on Firefox, but now that DOMParser is the default choice, we need
to be more accurate.
@angular-automatic-lock-bot
Copy link

@angular-automatic-lock-bot angular-automatic-lock-bot bot commented May 19, 2020

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 May 19, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants