fix: restore stats output target functionality. #3030

splitinfinities · 2021-08-26T20:06:04Z

Pull request checklist

Please check if your PR fulfills the following requirements:

Tests for the changes have been added (for bug fixes / features)
Docs have been reviewed and added / updated if needed (for bug fixes / features)
Build (npm run build) was run locally and any changes were pushed
Unit tests (npm test) were run locally and passed
E2E Tests (npm run test.karma.prod) were run locally and passed
Prettier (npm run format) was run locally and passed

Pull request type

Please check the type of change your PR introduces:

What is the current behavior?

When the stats flag, or the stats output target was added to a consumers repo, the stencil-stats.json file was not generated.

GitHub Issue Number: #2926
Jira: STENCIL-43

What is the new behavior?

This file, when generated, provides a json file that folks can use in order to pull in what files were generated and how large those files were, as well as some of the configuration options passed to the compiler when it was invoked.

Does this introduce a breaking change?

Yes
No

Testing

Go check this app out: https://github.com/splitinfinities/stencil-build-stats

As well as this branch. Link them, and build. You'll see the stencil-stats.json file produce changes.

Other information

Stencil Site related PR: https://github.com/ionic-team/stencil-site/pull/773/files

src/compiler/build/build-finish.ts

src/compiler/build/build-stats.ts

splitinfinities · 2021-08-26T21:19:54Z

src/compiler/output-targets/dist-lazy/generate-system.ts

@@ -26,7 +26,7 @@ export const generateSystem = async (
    const results = await generateRollupOutput(rollupBuild, esmOpts, config, buildCtx.entryModules);
    if (results != null) {
      const destinations = systemOutputs.map((o) => o.esmDir);
-      await generateLazyModules(
+      buildCtx.systemComponentBundle = await generateLazyModules(


Similar scenario to src/compiler/output-targets/dist-lazy/generate-cjs.ts

splitinfinities · 2021-08-26T21:21:13Z

src/compiler/output-targets/dist-lazy/lazy-output.ts

@@ -60,15 +60,13 @@ export const outputLazy = async (config: d.Config, compilerCtx: d.CompilerCtx, b

    const rollupBuild = await bundleOutput(config, compilerCtx, buildCtx, bundleOpts);
    if (rollupBuild != null) {
-      const [componentBundle] = await Promise.all([


Within this promise, the return signature in generateEsmBrowser was used and the others returned undefined. Refactoring to reset the buildCtx where the function does internal modifications felt like a better fit.

This will effectively serializes these outputs - do we have any measurements on how that impacts performance of the lazy build? If not, can we generate some?

Lars generated some, but I'll write a node script to run a build 10 times to gather and report data.

src/declarations/stencil-private.ts

splitinfinities · 2021-08-26T22:08:19Z

src/declarations/stencil-private.ts

@@ -260,7 +266,8 @@ export type BuildTask = any;

 export type BuildStatus = 'pending' | 'error' | 'disabled' | 'default';

-export interface BuildStats {
+export interface CompilerBuildStats {


I updated this name because it would match the names of other typings. Also, it wasn't used anywhere.

src/declarations/stencil-private.ts

ltm · 2021-09-02T18:11:34Z

src/compiler/build/build-stats.ts

+ */
+export function generateBuildStats(config: d.Config, buildCtx: d.BuildCtx) {
+  const buildResults = buildCtx.buildResults;
+  let jsonData: any;


I'd like to avoid adding any types if we can. This variable and the return of this function could be CompilerBuildStats | { diagnostics: d.Diagnostic[] }. You could add a type for the union and/or { diagnostics: d.Diagnostic[] }.

ltm · 2021-09-02T19:12:18Z

src/declarations/stencil-private.ts

+  esmBrowserComponentBundle: BundleModule[];
+  esmComponentBundle: BundleModule[];
+  es5ComponentBundle: BundleModule[];
+  systemComponentBundle: BundleModule[];
+  commonJsComponentBundle: BundleModule[];


I was worried that adding these to the build context would increase the memory usage significantly. Luckily that is not the case. I ran 10 builds of ionic-framework using master and this PR. The increase is relatively minor:

Build # master #3030

1 MEM: 682.1MB MEM: 658.6MB

2 MEM: 743.8MB MEM: 677.3MB

3 MEM: 546.5MB MEM: 757.6MB

4 MEM: 727.2MB MEM: 697.9MB

5 MEM: 731.8MB MEM: 658.1MB

6 MEM: 730.2MB MEM: 656.2MB

7 MEM: 738.5MB MEM: 749.1MB

8 MEM: 726.2MB MEM: 655.7MB

9 MEM: 709.2MB MEM: 684.9MB

10 MEM: 663.2MB MEM: 617.7MB

Average MEM: 681.3MB MEM: 699.9MB

@ltm is there a process you used for this that we could doc/reproduce in the future? I could see this also being useful for build times as well

splitinfinities · 2021-09-09T17:48:47Z

src/compiler/build/build-stats.ts

+      imports: bundle.rollupResult.imports,
+      // code: bundle.rollupResult.code, // (use this to debug)
+      // Currently, this number is inaccurate vs what seems to be on disk.
+      byteSize: byteSize(bundle.rollupResult.code),


@rwaskiewicz or @Lars, any recs about how to get this number to be more accurate without reading the file from disk? Should I omit for the time being?

How inaccurate are we talking? Like an order of magnitude? or a few bytes?

Oh, duh. It's because the files are compressed. It's a symptom of uglifying the files being applied after the build. Ironically, there's also some larger discrepancy between esmBrowser and esm as a result of esm not being minified. Since esm is not compressed right now (there's another bug somewhere filed about this...) the esm file's byte sizes produced at this point are accurate.

I think I'm going to rename this to "originalByteSize" instead. We should in the future figure out how to add that. Feels like an ordering problem. I'd hate to read files, as generating the stats may be much slower.

splitinfinities · 2021-09-09T18:42:00Z

Note: I'm not quite sure how best to test this exclusively within Jest, considering I don't know how to mock components to be compiled as a part of the compilerCtx/buildCtx during the Jest process. If you have any ideas don't hesitate to point me in the right direction!

rwaskiewicz

Do we have any plans to doc this as well? I noticed the checkbox in the PR summary was unchecked ATM

rwaskiewicz · 2021-09-09T18:42:44Z

src/compiler/build/build-stats.ts

+import type * as d from '../../declarations';
+
+/**
+ *


Minor, the JSDocs in this file just seem to be stubbed out

src/compiler/build/build-stats.ts

rwaskiewicz · 2021-09-09T18:56:13Z

src/compiler/output-targets/dist-lazy/generate-cjs.ts

@@ -25,7 +25,8 @@ export const generateCjs = async (
    const results = await generateRollupOutput(rollupBuild, esmOpts, config, buildCtx.entryModules);
    if (results != null) {
      const destinations = cjsOutputs.map((o) => o.cjsDir);
-      await generateLazyModules(
+
+      buildCtx.commonJsComponentBundle = await generateLazyModules(


Can we update the signature of the function to reflect we return something please? (and in the other files where we generate other module outputs for lazy)?

rwaskiewicz · 2021-09-09T18:59:36Z

src/compiler/output-targets/dist-lazy/lazy-output.ts

@@ -60,15 +60,13 @@ export const outputLazy = async (config: d.Config, compilerCtx: d.CompilerCtx, b

    const rollupBuild = await bundleOutput(config, compilerCtx, buildCtx, bundleOpts);
    if (rollupBuild != null) {
-      const [componentBundle] = await Promise.all([


This will effectively serializes these outputs - do we have any measurements on how that impacts performance of the lazy build? If not, can we generate some?

rwaskiewicz · 2021-09-09T19:03:27Z

src/declarations/stencil-private.ts

+  esmBrowserComponentBundle: BundleModule[];
+  esmComponentBundle: BundleModule[];
+  es5ComponentBundle: BundleModule[];
+  systemComponentBundle: BundleModule[];
+  commonJsComponentBundle: BundleModule[];


@ltm is there a process you used for this that we could doc/reproduce in the future? I could see this also being useful for build times as well

src/compiler/build/build-stats.ts

rwaskiewicz · 2021-09-09T19:26:41Z

src/compiler/build/build-stats.ts

+      imports: bundle.rollupResult.imports,
+      // code: bundle.rollupResult.code, // (use this to debug)
+      // Currently, this number is inaccurate vs what seems to be on disk.
+      byteSize: byteSize(bundle.rollupResult.code),


How inaccurate are we talking? Like an order of magnitude? or a few bytes?

splitinfinities · 2021-09-10T18:10:46Z

@rwaskiewicz Added your feedback! Had a couple conversation starters in your comments. Also, re: docs, yes I'd love to add them, but I don't think they should yet block this from being merged. I'd like to collaborate with Anthony on the content of the page. Something for next sprint I imagine.

rwaskiewicz · 2021-09-14T18:39:26Z

src/compiler/build/build-stats.ts

+      const result = await config.sys.writeFile(outputTarget.file, JSON.stringify(data, null, 2));
+
+      if (result.error) {
+        config.logger.warn([`Stats failed to write file to ${outputTarget.file}: ${result.error}`]);


Have we verified this won't print

Stats failed to write file to [Object object]: [Object object]

?

file should work fine, I removed the result.error so we can guard against that.

rwaskiewicz · 2021-09-14T18:39:52Z

src/compiler/build/build-stats.ts

- * @param compilerCtx
- * @param buildCtx
- * @returns
+ * @param config the project build configuration


Minor, the summary is missing in these JSDocs

rwaskiewicz · 2021-09-15T12:18:40Z

@splitinfinities regarding doc: I don't think we can merge this without docs. I think that needs to be a part of our definition of done

src/declarations/stencil-private.ts

rwaskiewicz · 2021-09-22T17:14:41Z

src/compiler/build/test/build-stats.spec.ts

+    if (result.hasOwnProperty('compiler') && result.compiler.hasOwnProperty('version')) {
+      delete result.compiler.version;
+    }


What value was this producing that we had to delete it and remove it from the assertion below?

The version that's built at the moment of the test's build (it appended the date and time to the version)

Hmmm, what was it that we changed here that made this suddenly fail?

It had been failing all along, actually, I just hadn't realized it because I didn't look into the build task on Github. My local build version wasn't failing because the timestamp hadn't changed.

I guess more concretely, what did we change in this PR to cause this to fail? Or are you saying it was failing since we turned tests back on in CI?

I mean quite literally that all the tests in all of my previous commits failed on CI because the built compiler's version includes a timestamp, so I deleted the version that includes the timestamp so I don't have to test the object against that.

Gotcha. Arguably, we should mock timestamp out, but I think that may be out of scope for this PR. Would you mind creating a ticket and adding it to the backlog?

STENCIL-137

splitinfinities · 2021-09-22T19:07:07Z

Stencil Site's related PR: https://github.com/ionic-team/stencil-site/pull/773/files

rwaskiewicz · 2021-09-24T14:44:05Z

src/compiler/output-targets/dist-lazy/lazy-output.ts

+          buildCtx.buildResults = result.buildCtx.buildResults;
+          buildCtx.components = result.buildCtx.components;


I'm not sure why we're doing this/what's going on here - can you explain this to me please?

Yeah! The components and buildResults are passed on to the buildCtx within this file from the resultant builds. These needed to be set on the buildCtx here because the promises don't update the buildCtx any more, and the original stats code needs to understand the state of the updated buildCtx after the promises all run. I wrote it this way so that we could keep the parallelization of the builds.

Does this affect the memory usage of Stencil in a negative way? I mean, I'm sure there's some cost associated with it, but do you have an idea of the order of magnitude?

I suspect this comment still holds true, and may likely improve since I brought the parallelization back.

I was worried that adding these to the build context would increase the memory usage significantly. Luckily that is not the case. I ran 10 builds of ionic-framework using master and this PR. The increase is relatively minor:

Build # master #3030
1 MEM: 682.1MB MEM: 658.6MB
2 MEM: 743.8MB MEM: 677.3MB
3 MEM: 546.5MB MEM: 757.6MB
4 MEM: 727.2MB MEM: 697.9MB
5 MEM: 731.8MB MEM: 658.1MB
6 MEM: 730.2MB MEM: 656.2MB
7 MEM: 738.5MB MEM: 749.1MB
8 MEM: 726.2MB MEM: 655.7MB
9 MEM: 709.2MB MEM: 684.9MB
10 MEM: 663.2MB MEM: 617.7MB
Average MEM: 681.3MB MEM: 699.9MB

splitinfinities self-assigned this Aug 26, 2021

splitinfinities added the work-in-progress: do not review label Aug 26, 2021

splitinfinities commented Aug 26, 2021

View reviewed changes

src/compiler/build/build-finish.ts Outdated Show resolved Hide resolved

splitinfinities commented Aug 26, 2021

View reviewed changes

This was referenced Aug 26, 2021

CLI Flag --stats does nothing #2926

Closed

Stats output target not producing anything #2633

Closed

ltm reviewed Sep 2, 2021

View reviewed changes

splitinfinities commented Sep 9, 2021

View reviewed changes

splitinfinities removed the work-in-progress: do not review label Sep 9, 2021

splitinfinities marked this pull request as ready for review September 9, 2021 18:39

splitinfinities requested a review from a team September 9, 2021 18:39

splitinfinities added 4 commits September 9, 2021 13:39

fix: restore stats output target functionality.

aa7fd72

Feedback from my own review

f05890b

prettier

da2e392

Adding stats and feedback from Lars

a58bc3c

splitinfinities force-pushed the STENCIL-43-fix-stats-flag branch from 5eac36e to a58bc3c Compare September 9, 2021 18:39

Expand out typing

d0f7be8

rwaskiewicz reviewed Sep 9, 2021

View reviewed changes

Adding feedback from Ryan, improving typings

907f105

rwaskiewicz reviewed Sep 14, 2021

View reviewed changes

splitinfinities added Resolution: Refine This PR is marked for Jira refinement. We're not working on it - we're talking it through. Resolution: Backlog and removed Resolution: Refine This PR is marked for Jira refinement. We're not working on it - we're talking it through. labels Sep 16, 2021

splitinfinities added 3 commits September 22, 2021 11:18

Switch back to parallel builds

e8e662a

prettier

4ff5749

Fix test

d1e73fe

splitinfinities force-pushed the STENCIL-43-fix-stats-flag branch from ff607c0 to d1e73fe Compare September 22, 2021 16:35

rwaskiewicz reviewed Sep 22, 2021

View reviewed changes

splitinfinities added 2 commits September 22, 2021 12:38

Update the components number

807caa9

Updating name of new return type

d5bc274

splitinfinities requested review from ltm and rwaskiewicz September 22, 2021 19:06

rwaskiewicz reviewed Sep 24, 2021

View reviewed changes

rwaskiewicz approved these changes Sep 24, 2021

View reviewed changes

Merge branch 'master' into STENCIL-43-fix-stats-flag

303af48

rwaskiewicz merged commit c76dca7 into master Sep 24, 2021

rwaskiewicz deleted the STENCIL-43-fix-stats-flag branch September 24, 2021 18:29

Build #	`master`	#3030
1	MEM: 682.1MB	MEM: 658.6MB
2	MEM: 743.8MB	MEM: 677.3MB
3	MEM: 546.5MB	MEM: 757.6MB
4	MEM: 727.2MB	MEM: 697.9MB
5	MEM: 731.8MB	MEM: 658.1MB
6	MEM: 730.2MB	MEM: 656.2MB
7	MEM: 738.5MB	MEM: 749.1MB
8	MEM: 726.2MB	MEM: 655.7MB
9	MEM: 709.2MB	MEM: 684.9MB
10	MEM: 663.2MB	MEM: 617.7MB
Average	MEM: 681.3MB	MEM: 699.9MB

		buildCtx.buildResults = result.buildCtx.buildResults;
		buildCtx.components = result.buildCtx.components;

fix: restore stats output target functionality. #3030

fix: restore stats output target functionality. #3030

Conversation

splitinfinities commented Aug 26, 2021 • edited Loading

Pull request checklist

Pull request type

What is the current behavior?

What is the new behavior?

Does this introduce a breaking change?

Testing

Other information

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

splitinfinities commented Sep 9, 2021

rwaskiewicz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

splitinfinities commented Sep 10, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwaskiewicz commented Sep 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

splitinfinities Sep 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

splitinfinities Sep 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

splitinfinities commented Sep 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

splitinfinities commented Aug 26, 2021 •

edited

Loading

splitinfinities Sep 22, 2021 •

edited

Loading

splitinfinities Sep 24, 2021 •

edited

Loading