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.

Sign up for GitHub

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

Jump to bottom

Improve benchmarks accuracy #148

Merged
merged 10 commits into from Nov 27, 2016
Merged

Improve benchmarks accuracy #148

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
827a835
Improve benchmarks accuracy
kangax Sep 5, 2016
4fa81c5
Fix test
kangax Sep 5, 2016
245306e
Merge master
kangax Nov 25, 2016
aee567a
Remove plugins from package.json
kangax Nov 25, 2016
a8c3740
Revert parts of benchamrk
kangax Nov 25, 2016
0449171
Add best size / best speed
kangax Nov 25, 2016
069ba34
Add an option to simplify
kangax Nov 25, 2016
4c4c10f
Fix linter
kangax Nov 25, 2016
d051c44
Remove multiPass
kangax Nov 27, 2016
4ee5860
Remove state arg
kangax Nov 27, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 6 additions & 1 deletion packages/babel-plugin-minify-simplify/src/index.js
View file
Open in desktop
Expand Up @@ -507,7 +507,12 @@ module.exports = ({ types: t }) => {
},

Function: {
exit(path) {
exit(path, state) {
const multiPass = state.opts.multiPass === false ? false : true;
Copy link
Member Author

Choose a reason for hiding this comment

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

This is true by default which probably makes sense most of the time.

Copy link
Member

Choose a reason for hiding this comment

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

exit(path, { opts: { multipass = true } = {} } = {}) {
}

?

Copy link
Member Author

Choose a reason for hiding this comment

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

A bit too cryptic for my taste :) What do you think?

Copy link
Member

@boopathi boopathi Sep 12, 2016 •
edited

Choose a reason for hiding this comment

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

For this (putting everything in one line and just one 2-level-deep option) it looks cryptic, actually using this is much clear sometimes, We could use default destructuring assignment when we have more options.

if (!multiPass) {
return;
}

earlyReturnTransform(path);

if (!path.node[shouldRevisit]) {
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't multipass affect this ?

It throws an error for me - Container is falsy. I suppose this happens when calling path.replaceWith on the path whose node is modified.

Copy link
Member

Choose a reason for hiding this comment

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

I tried the script from the readme -

./scripts/benchmark.js react@0.14.3 react/dist/react.js

Expand Down
46 changes: 39 additions & 7 deletions scripts/benchmark.js
View file
Open in desktop
Expand Up @@ -17,6 +17,8 @@ const compile = require("google-closure-compiler-js").compile;

let packagename, filename;

const NUM_TEST_RUNS = 3;

const script = new Command("benchmark.js")
.option("-o, --offline", "Only install package if not present; package not removed after testing")
.usage("[options] <package> [file]")
Expand All @@ -35,7 +37,7 @@ if (!packagename) {
const pathToScripts = __dirname;

const table = new Table({
head: ["", "raw", "raw win", "gzip", "gzip win", "parse time", "run"],
head: ["", "raw", "raw win", "gzip", "gzip win", "parse time", "run time (average)"],
chars: {
top: "",
"top-mid": "" ,
Expand Down Expand Up @@ -100,6 +102,7 @@ function checkFile() {
}

function test(name, callback) {

console.log("testing", name);

const start = Date.now();
Expand All @@ -116,23 +119,54 @@ function test(name, callback) {
const parseEnd = Date.now();
const parseNow = parseEnd - parseStart;

const runTimes = [run];

for (let i = 1; i < NUM_TEST_RUNS; i++) {
const start = Date.now();
callback(code);
runTimes.push(Date.now() - start);
}

const totalTime = runTimes.reduce((a, b) => a + b, 0);
const average = parseInt(totalTime / runTimes.length, 10);

results.push({
name: name,
raw: result.length,
gzip: gzipped.length,
parse: parseNow,
run: run,
run: average,
});
}

function testFile() {
code = fs.readFileSync(filename, "utf8");
gzippedCode = zlib.gzipSync(code);

test("babili", function (code) {
test("babili (best speed)", function (code) {
return babel.transform(code, {
sourceType: "script",
presets: [require("../packages/babel-preset-babili")],
presets: [
[require("../packages/babel-preset-babili"), {
simplify: {
multiPass: false
}
}]
],
comments: false,
}).code;
});

test("babili (best size)", function (code) {
return babel.transform(code, {
sourceType: "script",
presets: [
[require("../packages/babel-preset-babili"), {
simplify: {
multiPass: true
Copy link
Member Author

Choose a reason for hiding this comment

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

@boopathi this doesn't seem to show any difference in any of the files I'm testing...

Copy link
Member

Choose a reason for hiding this comment

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

there is no multipass option implemented in simplify.

Copy link
Member

Choose a reason for hiding this comment

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

You implemented it here - in this commit - 827a835#diff-05684bffcab704dd5d3842adbf921357 and your latest changes don't seem to have this one

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, I'm an idiot, was thinking that we already added it.

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 still doesn't seem to work though. Am I missing something else? Should I change anything in options-manager?

}
}]
],
comments: false,
}).code;
});
Expand Down Expand Up @@ -161,9 +195,7 @@ function testFile() {
}

function processResults() {
results = results.sort(function (a, b) {
return a.gzip > b.gzip;
});
results = results.sort((a, b) => a.gzip > b.gzip);

results.forEach(function (result, i) {
let row = [
Expand Down