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

Improve performance of ngc in bazel / blaze #19581

Closed
wants to merge 2 commits into from

Conversation

tbosch
Copy link
Contributor

@tbosch tbosch commented Oct 5, 2017

No description provided.

@tbosch tbosch added target: major This PR is targeted for the next major release area: core Issues related to the framework runtime labels Oct 5, 2017
@mary-poppins
Copy link

You can preview 10a15b1 at https://pr19581-10a15b1.ngbuilds.io/.

@mary-poppins
Copy link

You can preview dfaee55 at https://pr19581-dfaee55.ngbuilds.io/.

This helps hazel as it does not check libraries (e.g. the default lib) which are
not input files, but still checks `.d.ts` files that are inputs.
@mary-poppins
Copy link

You can preview b021793 at https://pr19581-b021793.ngbuilds.io/.

@tbosch tbosch force-pushed the perf_g3 branch 2 times, most recently from aa23164 to 7cb1c0c Compare October 5, 2017 23:33
@mary-poppins
Copy link

You can preview 7cb1c0c at https://pr19581-7cb1c0c.ngbuilds.io/.

@@ -20,6 +20,8 @@ const NGC_GEN_FILES = /^(.*?)\.(ngfactory|ngsummary|ngstyle|shim\.ngstyle)(.*)$/
// knows about them
const NGC_ASSETS = /\.(css|html|ngsummary\.json)$/;

const BLAZE_BIN = /\bblaze-out\b.*?\bbin\b/;
Copy link
Contributor

Choose a reason for hiding this comment

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

It's surprising to see this here - would it be hard to implement this in the first-party ngc-wrapped instead?

@@ -160,6 +162,15 @@ export function compile({allowNonHermeticReads, compilerOpts, tsHost, bazelOpts,
relativeToRootDirs(importedFilePath, compilerOpts.rootDirs).replace(EXT, '');
ngHost.toSummaryFileName = (fileName: string, referringSrcFileName: string) =>
ngHost.fileNameToModuleName(fileName, referringSrcFileName);
if (blazeBin) {
Copy link
Contributor

Choose a reason for hiding this comment

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

eg. you could introduce
bazelOpts.restrictSummaryResolution: string
and set it to the blaze-bin rootDirs[x] value in first-party
then just use the presence of that bazelOpt here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Introduced an argument allDepsCompiledWithBazel for now, which is true inside of G3 but false outside.

For hazel, we have a specific way of writing summaries,
which we can leverage to make the deserialization faster.
@mary-poppins
Copy link

You can preview 0bef4b8 at https://pr19581-0bef4b8.ngbuilds.io/.

@tbosch tbosch added the action: merge The PR is ready for merge by the caretaker label Oct 6, 2017
@tbosch tbosch modified the milestone: Merge-queue Oct 6, 2017
chuckjaz pushed a commit to chuckjaz/angular that referenced this pull request Oct 6, 2017
For hazel, we have a specific way of writing summaries,
which we can leverage to make the deserialization faster.

PR Close angular#19581
@chuckjaz chuckjaz closed this in 0b06ea1 Oct 6, 2017
@tbosch tbosch deleted the perf_g3 branch October 10, 2017 15:16
@angular-automatic-lock-bot
Copy link

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 Sep 12, 2019
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 cla: yes target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants