Skip to content
This repository has been archived by the owner on Jul 16, 2023. It is now read-only.

feat: add byte store to improve performance #428

Merged
merged 6 commits into from Aug 19, 2021
Merged

Conversation

incendial
Copy link
Member

What is the purpose of this pull request? (put an "X" next to an item)

[ ] Documentation update
[ ] Bug fix
[ ] New rule
[ ] Changes an existing rule
[ ] Add autofixing to a rule
[ ] Add a CLI option
[X] Add something to the core
[ ] Other, please explain:

@incendial
Copy link
Member Author

@roman-petrov could you run this fix on your codebase just to see how much is the difference now? It should now run faster after first run (actually similarly to dart analyze).

@codecov-commenter
Copy link

codecov-commenter commented Aug 17, 2021

Codecov Report

Merging #428 (8482354) into master (f6739aa) will decrease coverage by 0.27%.
The diff coverage is 91.66%.

❗ Current head 8482354 differs from pull request most recent head 361443f. Consider uploading reports for the commit 361443f to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #428      +/-   ##
==========================================
- Coverage   81.64%   81.36%   -0.28%     
==========================================
  Files         178      177       -1     
  Lines        3749     3811      +62     
==========================================
+ Hits         3061     3101      +40     
- Misses        688      710      +22     
Impacted Files Coverage Δ
lib/src/utils/analyzer_utils.dart 90.00% <90.00%> (ø)
lib/src/analyzers/lint_analyzer/lint_analyzer.dart 94.77% <100.00%> (ø)
...s/unused_files_analyzer/unused_files_analyzer.dart 89.18% <100.00%> (-0.56%) ⬇️
..._list/code_climate/lint_code_climate_reporter.dart 84.78% <0.00%> (-12.66%) ⬇️
lib/src/cli/commands/analyze.dart 46.26% <0.00%> (ø)
lib/src/analyzer_plugin/analyzer_plugin.dart 0.00% <0.00%> (ø)
...zers/lint_analyzer/reporters/reporter_factory.dart 100.00% <0.00%> (ø)
...analyzer/reporters/models/file_metrics_report.dart 100.00% <0.00%> (ø)
...de_climate/models/code_climate_issue_category.dart
...de_climate/models/code_climate_issue_severity.dart
... and 7 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 f6739aa...361443f. Read the comment docs.

@roman-petrov
Copy link
Contributor

@incendial , thank you for all your efforts trying to improve dart code metrics performance!

I just finished testing this branch to see if it has better analysis performance compared to master. Unfortunately, on my codebase, I don't see any notable difference (please keep in mind - we use only Windows machines, this might make sense).

After that, I decided to compare dart code metrics performance on the dart code metrics project itself. To make this semi-automatic I created a simple batch script that measures execution time:

@echo %time%
@call metrics analyze dart-code-metrics/lib >nul
@echo %time%

Before running each test I switched to git branch I need, deactivated dart code metrics globally, and then activated it again from path using dart pub global activate -spath D:\Projects\GitHub\dart-code-metrics.

Results are as follows:

Without optimization, master branch (average time ~14 seconds):
without_optimization

With optimization, add-bytestore branch (average time ~24 seconds):
with_optimization

Actually, I performed these tests twice, but the results are the same... So unfortunately I observe serious performance regression. not sure if I doing something wrong. Maybe, this is related to Windows somehow.

Maybe, the good next step would be if you repeat my test on Windows machine if you have one?...

@incendial
Copy link
Member Author

@roman-petrov thank you for your help, I think performance is very important so we definitely should address issues like that.

Without optimization, master branch (average time ~14 seconds):
With optimization, add-bytestore branch (average time ~24 seconds):

Interesting... I see exactly the opposite (mac os, 24 for master and 16 for this branch) which is very strange.

Could you try to find .dartServer/.analysis-driver and .dartServer/.dart-code-metrics folders and try to remove them and measure it again (removing first one should slow down dart analyze and second one works the same way for our cli)?

I not sure about cache mechanism working differently for Windows, but let's try to find out.

@incendial
Copy link
Member Author

I used dart bin/metrics.dart analyze lib to call our cli, maybe global activation works differently, I'll check that too.

@roman-petrov
Copy link
Contributor

I followed your advice and removed .dartServer/.analysis-driver and .dartServer/.dart-code-metrics folders. This not affected master branch analysis performance, so I will post what I see on add-bytestore branch:

image

As you see, first run takes more than 4 minutes, timings for subsequent runs are not changed (still ~23 seconds). So I still not see performance improvements on my machine.

I also tried using dart dart-code-metrics/bin/metrics.dart analyze instead of globally activated packages and didn't see any difference in timings.

@incendial
Copy link
Member Author

As you see, first run takes more than 4 minutes, timings for subsequent runs are not changed (still ~23 seconds). So I still not see performance improvements on my machine.

So, you mean, that master still runs for 14 secs?

@incendial
Copy link
Member Author

And what about dart analyze?

@roman-petrov
Copy link
Contributor

@incendial I have a small idea: maybe we can ask Telegram community in dart code metrics channel to help us to check if this PR improves analysis performance?

@incendial
Copy link
Member Author

incendial commented Aug 18, 2021

@incendial I have a small idea: maybe we can ask Telegram community in dart code metrics channel to help us to check if this PR improves analysis performance?

Let's measure it on Windows first (@dkrutskikh will do it today) and then we'll maybe go this way, if he'll get different results than you did. I guess, two reports for Windows regression will be enough.

@roman-petrov
Copy link
Contributor

So, you mean, that master still runs for 14 secs?

Exactly. When I delete dart code metrics and analyzer plugin caches timings on master not changed. Even on a first run I get ~13-14 seconds.

And what about dart analyze?

I created similar batch script to measure time:

@echo %time%
@call dart analyze dart-code-metrics >nul
@echo %time%

First run, I deleted analysis driver cache (~17 seconds):
image

Second run, without deleting cache(~11 seconds):
image

So when running dart analyze filled cache improves performance. But (that's quite strange!) caching on this branch negatively affects performance on my machine and first run takes extremely long time (4 mins) 🤔

@incendial
Copy link
Member Author

So when running dart analyze filled cache improves performance. But (that's quite strange!) caching on this branch negatively affects performance on my machine and first run takes extremely long time (4 mins) 🤔

That's very strange since it's available only in cli mode and even if we assume the server launches the plugin too, it still shouldn't affect it at all...

@roman-petrov
Copy link
Contributor

That's very strange since it's available only in cli mode and even if we assume the server launches the plugin too, it still shouldn't affect it at all...

Let's wait for results from @dkrutskikh.. I think we should figure out if the problem is Windows-specific or it happens only on my machine.

@dkrutskikh
Copy link
Member

@incendial @roman-petrov here are results I've got:

test.cmd

@echo %time%
@call dart run bin/metrics.dart analyze lib > nul
@echo %time%

master

PS C:\Users\dmitry\Development\dart-code-metrics> ./test.cmd
 8:07:09,44
 8:07:42,18 (32.74 sec)
PS C:\Users\dmitry\Development\dart-code-metrics> ./test.cmd
 8:07:44,98
 8:08:13,66 (28.68 sec)
PS C:\Users\dmitry\Development\dart-code-metrics> ./test.cmd
 8:08:38,37
 8:09:05,28 (26.91 sec)

add-bytestore

 PS C:\Users\dmitry\Development\dart-code-metrics> ./test.cmd      
  8:11:07,24
  8:11:36,11 (28.87 sec)
 PS C:\Users\dmitry\Development\dart-code-metrics> ./test.cmd
  8:11:48,99
  8:12:02,91 (13.92 sec)
 PS C:\Users\dmitry\Development\dart-code-metrics> ./test.cmd
  8:12:05,15
  8:12:19,12 (13.97 sec)

@roman-petrov
Copy link
Contributor

I decided to repeat the test on my machine. That's strange, but I got different results. Maybe, yesterday I had some Windows environment-related problems like running anti-virus or something like that...

So today results are:

master: ~12 seconds

master

add-bytestore: ~8 seconds

add_bytestore

So finally I see performance improvement too. Thank you for all your work!

@incendial
Copy link
Member Author

Finally 🎉

@dkrutskikh dkrutskikh merged commit e81eabb into master Aug 19, 2021
@dkrutskikh dkrutskikh deleted the add-bytestore branch August 19, 2021 14:00
@roman-petrov
Copy link
Contributor

@incendial, @dkrutskikh

Maybe, it would be interesting for you: I compared dart_code_metrics 4.1.0 with 4.2.0 on our monorepo and the results are great, please take a look:

4.1.0

4_1_0

4.2.0

4 2 0

So as you see, there is a 3X (115s -> 42s) performance improvement! Thank you for your great work!

PS. Screenshots are from my workflow tool that we will use within our company for Flutter / Dart projects.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants