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

Enhance the benchmark #209

Merged
merged 20 commits into from
Jan 19, 2018
Merged

Enhance the benchmark #209

merged 20 commits into from
Jan 19, 2018

Conversation

nlepage
Copy link
Member

@nlepage nlepage commented Jan 9, 2018

No description provided.

@nlepage nlepage added this to the 1.0 milestone Jan 9, 2018
@nlepage nlepage self-assigned this Jan 9, 2018
@codecov-io
Copy link

codecov-io commented Jan 10, 2018

Codecov Report

Merging #209 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #209   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          93     93           
  Lines         267    267           
=====================================
  Hits          267    267

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 2267229...6f8baf8. Read the comment docs.

@nlepage nlepage force-pushed the benchmark/enhance branch 2 times, most recently from 0d928f6 to 455857a Compare January 16, 2018 21:45
@nlepage nlepage removed the 🚧 WIP label Jan 16, 2018
export function createBenchmark(title, testResult, pMaxTime = 30, pMaxOperations = 1000) {

const fast = Boolean(process.env.FAST)
const maxTime = fast ? pMaxTime / 3 : pMaxTime, maxOperations = fast ? Math.round(pMaxOperations / 3) : pMaxOperations
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe on two lines so it would be clearer with the ternary operator ?


it('ES2015', () => {
benchmark('es2015', 'ES2015 destructuring', () => {
const start = randomStart(), end = start + modifySize
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 more readable than the previous one with ternary operators but i found it hard to view each variables at one glance.


it('ES2015', () => {
benchmark('es2015', 'ES2015 destructuring', () => {
const start = randomStart(), end = start + modifySize
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we could create a function getTestBounds which return an array with start and end value ?


const runs = []

function run(key, opTitle, operation) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe you could add more comments to explain more your approach ? Or a pointer on the method you followed ?

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 document in a later PR, created an issue #223

frinyvonnick
frinyvonnick previously approved these changes Jan 19, 2018
Copy link
Contributor

@frinyvonnick frinyvonnick left a comment

Choose a reason for hiding this comment

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

Nice work 👍

})
}

// Prepare immutalbe state
Copy link
Contributor

Choose a reason for hiding this comment

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

typo "immutable"

@nlepage nlepage merged commit 4d31d8c into master Jan 19, 2018
@nlepage nlepage deleted the benchmark/enhance branch January 19, 2018 09:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants