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

Log bundle metrics #733

Merged
merged 17 commits into from
Feb 14, 2018
Merged

Log bundle metrics #733

merged 17 commits into from
Feb 14, 2018

Conversation

DeMoorJasper
Copy link
Member

@DeMoorJasper DeMoorJasper commented Feb 2, 2018

Feature requested in closes #415

Minimal report (default behaviour)
screen shot 2018-02-02 at 17 15 25
Detailed report (cli flag --detailed-report)
screen shot 2018-02-02 at 17 15 07

@codecov-io
Copy link

codecov-io commented Feb 2, 2018

Codecov Report

Merging #733 into master will increase coverage by 0.33%.
The diff coverage is 94.11%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #733      +/-   ##
==========================================
+ Coverage    94.1%   94.43%   +0.33%     
==========================================
  Files          64       66       +2     
  Lines        2036     2103      +67     
==========================================
+ Hits         1916     1986      +70     
+ Misses        120      117       -3
Impacted Files Coverage Δ
src/Bundler.js 91.66% <100%> (+0.18%) ⬆️
src/packagers/HTMLPackager.js 97.43% <100%> (+0.06%) ⬆️
src/Bundle.js 98.94% <100%> (+0.09%) ⬆️
src/packagers/SourceMapPackager.js 100% <100%> (ø) ⬆️
src/packagers/RawPackager.js 92.3% <100%> (+0.64%) ⬆️
src/utils/prettifyTime.js 100% <100%> (ø)
src/packagers/CSSPackager.js 100% <100%> (ø) ⬆️
src/utils/bundleReport.js 91.48% <91.48%> (ø)
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6d23efd...c83f611. Read the comment docs.

src/Bundle.js Outdated
@@ -18,6 +18,14 @@ class Bundle {
this.siblingBundles = new Set();
this.siblingBundlesMap = new Map();
this.offsets = new Map();
this.startTime = new Date().getTime();
Copy link
Contributor

Choose a reason for hiding this comment

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

Possibly a micro-optimization, but Date.now() is historically faster.

Copy link
Member Author

Choose a reason for hiding this comment

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

I will change it, performance boosts are always a good thing

@@ -23,6 +23,7 @@ class CSSPackager extends Packager {
}

await this.dest.write(css);
this.bundle.addAssetSize(asset, this.dest);
Copy link
Contributor

Choose a reason for hiding this comment

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

Any way for this to be in the base Packager without reading the file every time? Or maybe stat is efficient enough? Would be nice to not have to re-implement this for every Packager (and remember to do so in the future).

Copy link
Member Author

Choose a reason for hiding this comment

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

I can possibly move it to asset, but stat is an extra operation and it wouldn't keep in mind things like treeshaking and uglifying. So i guess this is the correct implementation, keeping in mind that some Packagers have a different way of writing files, for example sourcemap writes everything at the end and rawPackager copies the file (without a writestream)
I did my best to abstract it using bundle.addAssetSize

return `${size.toFixed(2)} ${sizeTypes[type]}`;
}

function prettifyTime(time) {
Copy link
Contributor

@brandon93s brandon93s Feb 3, 2018

Choose a reason for hiding this comment

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

Similar logic exists in Bundler.js and could be consolidated into a util:

parcel/src/Bundler.js

Lines 196 to 201 in 391e17f

let buildTime = Date.now() - startTime;
let time =
buildTime < 1000
? `${buildTime}ms`
: `${(buildTime / 1000).toFixed(2)}s`;
logger.status(emoji.success, `Built in ${time}.`, 'green');

Copy link
Member Author

Choose a reason for hiding this comment

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

Will change

size = size / 1024;
type++;
}
return `${size.toFixed(2)} ${sizeTypes[type]}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

While unlikely, you could overflow your array in which case you'd end up with 1.54 undefined. Should at least safe-guard against it and stop dividing by 1024.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ow yeah, thought it wouldn't be an issue as a bundle larger than 1024 GB would be insane for a web application (or any other application)
But i've fixed it anyway

@@ -0,0 +1,106 @@
const path = require('path');
const sizeTypes = ['B', 'KB', 'MB', 'GB'];
Copy link
Contributor

Choose a reason for hiding this comment

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

Should KB be kB?

Copy link
Member Author

Choose a reason for hiding this comment

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

It should be KB as it's 1024 not 1000
"1 KB" means 1024 bytes (as Windows would report it, traditional usage)
"1 kB" means 1000 bytes (as Mac OS would report it, IEC usage)
"1 KiB" means 1024 bytes (unambiguous, but perhaps unfamiliar terminology)

const path = require('path');
const sizeTypes = ['B', 'KB', 'MB', 'GB'];

function spaces(amount) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can use String.prototype.repeat() here:

function spaces(count) {
	return ' '.repeat(count);
}

It also correctly returns an empty string for 0.

There is also String.prototype.padEnd() in Node 8+ which may simplify some code.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll use the repeat as babel didn't catch padEnd when i tried to build

@brandon93s
Copy link
Contributor

Curious why the total time is so much greater (as a %) than the bundle time in both of the screenshots.

@DeMoorJasper
Copy link
Member Author

DeMoorJasper commented Feb 3, 2018

@brandon93s the bigger time is because the total time in the metrics is the total processing time, basically the time it would've taken if ran on a single thread/worker.
The total build time or the time parcel runs is the perceived time, not the actual processing time.
So in theory the metrics total time would be near total bundle time * threads on large projects

@@ -0,0 +1,95 @@
const path = require('path');
const prettifyTime = require('./prettifyTime');
const sizeTypes = ['B', 'KB', 'MB', 'GB'];
Copy link
Contributor

Choose a reason for hiding this comment

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

What about using KiB, MiB, GiB or changing the factor to 1000 instead of 1024, for correctness?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure, the current implementation seems like the accurate notation, as it's being used by a lot of OS's (I've changed it anyway so that it is 100% correct)

Copy link
Contributor

Choose a reason for hiding this comment

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

I know that Windows uses the wrong KB notation, and that most Linux distros use KiB notation. Don't know about Mac, Android, iOS and others.
(nice!)

@devongovett
Copy link
Member

Question: should we only do this on parcel build and not in development mode? Seems like bundle sizes aren't that useful in development, and will be wrong anyway since code isn't minified/optimized in dev mode anyway. Thoughts?

@devongovett
Copy link
Member

Ok I ended up reworking this a bit to make it a bit prettier. Here's what it looks like:

screen shot 2018-02-13 at 9 53 32 pm

When you want a detailed report it looks like this:

screen shot 2018-02-13 at 9 51 42 pm

As you can see, it's clearer which items are bundles, and which items are assets within bundles. Bundles are bold, and assets are dimmed and indented. Plus it's colorful! Directory names are dimmed and filenames highlighted. 🎨

It will also warn when you have a large bundle size with a warning triangle and yellow text:

screen shot 2018-02-13 at 9 52 38 pm

I moved the table rendering stuff to the logger class, and used the widely used filesize module to format file sizes. I also made it so each packager doesn't need to manually add each asset size - the bundler handles that as it is packaging. There is a default getSize function in the packager that returns the current output size of the file, and the base Packager implements the default version of this. Only RawPackager needed to override it.

Let me know what you think!

devongovett pushed a commit that referenced this pull request Oct 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🙋 Report of generated bundle size + breakdown of contents
5 participants