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

test: basic setup for benchmarks #134

Closed
wants to merge 27 commits into from
Closed

test: basic setup for benchmarks #134

wants to merge 27 commits into from

Conversation

armano2
Copy link
Member

@armano2 armano2 commented Jan 23, 2019

update:

Single File: restrict-plus-operands

tslint x 2.42 ops/sec ±5.84% (11 runs sampled)
eslint x 2,177 ops/sec ±8.03% (78 runs sampled)
Fastest is eslint

Multi File: restrict-plus-operands

tslint x 2.76 ops/sec ±2.33% (12 runs sampled)
eslint x 896 ops/sec ±2.58% (85 runs sampled)
Fastest is eslint

Single File: no-empty-interface

tslint x 1,636 ops/sec ±0.79% (90 runs sampled)
eslint x 2,584 ops/sec ±3.82% (76 runs sampled)
Fastest is eslint

Multi File: no-empty-interface

tslint x 1,202 ops/sec ±1.29% (90 runs sampled)
eslint x 1,018 ops/sec ±2.79% (82 runs sampled)
Fastest is tslint

#130 (comment)

@aboyton @JamesHenry @uniqueiniquity @scottohara #55

@armano2 armano2 added the DO NOT MERGE PRs which should not be merged yet label Jan 23, 2019
@armano2 armano2 self-assigned this Jan 23, 2019
@armano2
Copy link
Member Author

armano2 commented Jan 24, 2019

small update:

i added additional test case when we try to process multiple files and looks like in this case tslint wins, we are not preserving Program, and parser has to do everythink from start:

// 1 file
tslint x 2.56 ops/sec ±9.62% (11 runs sampled)
eslint x 2.59 ops/sec ±6.59% (11 runs sampled)
Fastest is  [ 'eslint', 'tslint' ]

// 3 files
tslint x 2.41 ops/sec ±9.04% (11 runs sampled)
eslint x 0.83 ops/sec ±2.93% (7 runs sampled)
Fastest is  [ 'tslint' ]

more files we add to project, parser is creating more instances of tsc and difference its getting bigger

@uniqueiniquity
Copy link
Contributor

@armano2 That's definitely a bug on me, then. The intent was absolutely to preserve the Program... that's kinda the whole point. Thanks for finding that. I can definitely look into this, but probably won't have time until early next week.

@armano2 armano2 added the package: typescript-estree Issues related to @typescript-eslint/typescript-estree label Jan 24, 2019
@JamesHenry
Copy link
Member

@armano2 Is this marked as DO NOT MERGE because you are still working on it? Or do you not think we should merge it in general?

@JamesHenry
Copy link
Member

Seems like it might be good to have a consistent benchmark to iterate against when fixing the performance issue

@armano2
Copy link
Member Author

armano2 commented Jan 24, 2019

i'm still working on it, and i'm unsure if we should merge it :)

@j-f1
Copy link
Contributor

j-f1 commented Jan 24, 2019

Should we rename “DO NOT MERGE” to “WIP” and just close PRs we don’t plan to merge? This would also allow the wip bot to prevent us from merging WIP PRs.

@armano2 armano2 removed the package: typescript-estree Issues related to @typescript-eslint/typescript-estree label Jan 24, 2019
@armano2
Copy link
Member Author

armano2 commented Jan 25, 2019

i did some profiling on all steps, and running parser take alot of time

typescript-estree x 19,657 ops/sec ±1.79% (84 runs sampled)
parser x 8,640 ops/sec ±2.16% (81 runs sampled)

parser is way way slower than it should, i have some profiles and i will continue to investigate

@armano2 armano2 changed the title test: basic setup for benchmark of restrict-plus-operands test: basic setup for benchmarks Jan 25, 2019
@armano2 armano2 removed the DO NOT MERGE PRs which should not be merged yet label Jan 25, 2019
@JamesHenry
Copy link
Member

I will begin working on refactoring the TypeScript Program logic this evening - I will keep you in the loop @uniqueiniquity

@bradzacher
Copy link
Member

speeds are looking better!

@JamesHenry
Copy link
Member

Speaking with @uniqueiniquity we have some code in right now designed with the long running (e.g. in editor/IDE) process in mind.

It would be ideal if we could add some benchmark around "watch" performance as well, although I appreciate that is less straightforward

@bradzacher bradzacher added the tests anything to do with testing label Feb 12, 2019
@bradzacher bradzacher added the DO NOT MERGE PRs which should not be merged yet label Apr 11, 2019
@bradzacher
Copy link
Member

Closing this for housekeeping purposes.

@bradzacher bradzacher closed this Aug 9, 2019
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 21, 2020
@armano2 armano2 deleted the benchmark branch February 13, 2021 15:50
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
DO NOT MERGE PRs which should not be merged yet tests anything to do with testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants