fix(ngcc): do not use cached file-system #36687
Closed
Conversation
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).
JoostK
reviewed
Apr 17, 2020
There's a remaining comment in integration/ngcc/test.sh that refers to CachedFileSystem
packages/compiler-cli/ngcc/src/locking/lock_file_with_child_process/index.ts
Show resolved
Hide resolved
This was only being used by ngcc but not any longer.
JoostK
approved these changes
Apr 17, 2020
gkalpak
approved these changes
Apr 17, 2020
Awesome! Very excited about the memory savings
kara
approved these changes
Apr 17, 2020
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
added a commit
that referenced
this issue
Apr 17, 2020
This was only being used by ngcc but not any longer. PR Close #36687
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.
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
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 debugfor AIO on a local machinegave 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?
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information
The text was updated successfully, but these errors were encountered: