feat: implement `--sdk-path` CLI option to use when running as compiled executable #430

roman-petrov · 2021-08-20T06:36:35Z

roman-petrov · 2021-08-20T08:00:22Z

The implementation seems to be working just fine: now I am able to run dart code metrics as windows executable. I did some measurements again to check if compilation to exe improves performance.

I created two batch files:

measure.bat:

@echo %time%
@call dart dart-code-metrics/bin/metrics.dart analyze dart-code-metrics/lib --sdk-path D:\SDK\dart-sdk >nul
@echo %time%

measure_exe.bat:

@echo %time%
@call metrics.exe analyze --sdk-path D:\SDK\dart-sdk dart-code-metrics/lib >nul
@echo %time%

The results I see:

measure.bat: ~8 seconds

measure.exe.bat: ~ 2 seconds

As you can see, there is an "amazing" performance improvement, but I think this is too good to be true ;) So I think we need more measurements/comparisons to find out if we get real performance benefits when compiling to exe like:

Do we get performance improvement if we compare exe performance with aot snapshot performance when dart code metrics is activated globally from pub? I was unable to quickly test this because the current version is not published yet, and there is no precompilation when activating from path AnalysisSession getResolvedUnit2 is slower than AnalysisServer unit resolution dart-lang/sdk#46914 (comment)
Can you provide some measurements from your side? @dkrutskikh
Maybe it would be good to ask the Telegram community, what they think about this, do they see performance benefit.

So let's have further discussions on this if you don't mind. I will finish this PR if we will decide to move forward with this approach.

codecov · 2021-08-20T08:38:03Z

Codecov Report

Merging #430 (1f5702d) into master (6bbf52a) will increase coverage by 0.18%.
The diff coverage is 81.57%.

❗ Current head 1f5702d differs from pull request most recent head 9edcfbc. Consider uploading reports for the commit 9edcfbc to get more accurate results

@@            Coverage Diff             @@
##           master     #430      +/-   ##
==========================================
+ Coverage   84.74%   84.92%   +0.18%     
==========================================
  Files         217      218       +1     
  Lines        4439     4459      +20     
==========================================
+ Hits         3762     3787      +25     
+ Misses        677      672       -5

Impacted Files	Coverage Δ
lib/src/utils/analyzer_utils.dart	`90.00% <ø> (ø)`
lib/src/cli/commands/analyze.dart	`45.83% <50.00%> (+0.50%)`	⬆️
lib/src/cli/commands/check_unused_files.dart	`53.84% <50.00%> (+2.12%)`	⬆️
lib/src/cli/commands/check_unused_l10n.dart	`59.37% <50.00%> (+2.23%)`	⬆️
lib/src/cli/commands/base_command.dart	`68.51% <80.95%> (+17.00%)`	⬆️
lib/src/analyzers/lint_analyzer/lint_analyzer.dart	`92.74% <100.00%> (ø)`
...s/unused_files_analyzer/unused_files_analyzer.dart	`89.18% <100.00%> (ø)`
...ers/unused_l10n_analyzer/unused_l10n_analyzer.dart	`86.41% <100.00%> (ø)`
lib/src/cli/utils/detect_sdk_path.dart	`100.00% <100.00%> (ø)`

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 6bbf52a...9edcfbc. Read the comment docs.

incendial · 2021-08-22T12:36:58Z

I think the result is really good and it correlates with what I measured locally (~8 seconds for compilation). Thank you for your effort!

Do we get performance improvement if we compare exe performance with aot snapshot performance when dart code metrics is activated globally from pub?

Yeah, that's a good question, I think, the answer will not surprise us 😄. We published 4.2.0 today, could you please measure the snapshot version too?

Maybe it would be good to ask the Telegram community, what they think about this, do they see performance benefit.

Please show me a person who'd say that slower is better 😄😄😄. I think that it's a right direction no matter what.

The only suggestion I have is to check how dart analyze handles SDK detection and maybe we can find a solution without a separate option. Could you please research it too?

incendial · 2021-08-22T12:40:48Z

I really like the trend in your tests: 12 secs -> 8 secs -> 2 secs. It's such a magnificent improvement!!!

roman-petrov · 2021-08-22T14:24:24Z

@incendial, I like the trend too :) But there is still something we have to do before we get this ready to be used...

Tomorrow I will measure speed of running dart code metrics as precompiled AOT snapshot and publish the result here. We can continue discussing if running as EXE will still be significantly faster, if not - I think we can just stop here.

About Dart SDK path detection: I think, this is actually not possible. Users can have multiple Dart SDK versions installed (like I do). dart analyze runs inside Dart process so I assume it is easy to detect Dart SDK path from process handle / dart.exe path.

But when we compile to Windows EXE this information is lost and there is no any association between our EXE file and Dart SDK.

So I do not see many options at the moment:

Use CLI option for SDK path
Bundle required Dart SDK files as CLI application assets. But even in this case, I am not sure if Dart compiler will bundle these assets into compiled EXE file.

Actually, second scenario as I understand is not quite easy to implement. So as you see I am not quite optimistic here, but I will be back tomorrow with AOT snapshot measurements :)

roman-petrov · 2021-08-23T10:42:47Z

Today I performed three performance measurements. All measurements were done on this branch. Here are three batch files and the results:

measure_local.bat

@echo %time%
@call dart dart-code-metrics/bin/metrics.dart analyze dart-code-metrics/lib --sdk-path D:\SDK\dart-sdk >nul
@echo %time%

measure_aot.bat (this uses globally activated dart code metrics 4.2.0 and I ignored first launch while measuring because there is time to precompile aot)

@echo %time%
@call C:\Users\roman.petrov\AppData\Local\Pub\Cache\bin\metrics.bat  analyze dart-code-metrics/lib >nul
@echo %time%

measure_exe.bat

@echo %time%
@call metrics.exe analyze --sdk-path D:\SDK\dart-sdk dart-code-metrics/lib >nul
@echo %time%

The results are:

measure_local.bat: ~8 seconds

measure_aot.bat: ~4,5 seconds

measure_exe.bat: ~2,5 seconds

So exe still seems to be fastest method from my measurements. @dkrutskikh - can you please repeat these measurements on Windows to validate my results?..

If we decide to provide some fast dart code metrics workflow with exe compilation we need to figure out first how this can be done :) Current PR with CLI option might be just a starting point.

roman-petrov · 2021-08-25T06:13:23Z

@incendial,

A couple of days ago @dkrutskikh proposed an idea: we can try to detect SDK path from system PATH environment variable. I think this might be quite a good approach. The logic can be as follows:

for each <directory> in PATH try to find dart.exe
if found - use parent directory of this <directory> as SDK path

The possible drawback: some users (like I do) can have multiple Dart SDK versions installed and SDK we found might be outdated. In this case, users will just get unpredictable runtime errors when running compiled exe. But I still think that this approach is a good trade-off between complexity and functionality.

roman-petrov · 2021-08-25T06:24:44Z

But the most important question for me is: will Windows users actually use compiled exe? We know, compiling to exe gives a significant performance improvement. But it involves non standard Dart workflow:

Standard workflow: to use/upgrade dart package it is required only to run dart pub global activate <package_name> and that's all.
EXE workflow: users will have to clone the repository or activate the package globally first to download the package source code. After that, they should compile it using dart compile exe. The upgrade process requires repeating these steps.

So I am completely unsure if users will like the second workflow and use it. This complex workflow might provide major benefits only for large codebases. Personally, I am quite satisfied with CLI performance after #428.

@incendial , @dkrutskikh - what do you think, should we continue with this?

incendial · 2021-08-25T08:13:56Z

The possible drawback: some users (like I do) can have multiple Dart SDK versions installed and SDK we found might be outdated. In this case, users will just get unpredictable runtime errors when running compiled exe. But I still think that this approach is a good trade-off between complexity and functionality.

This exactly why we need to combine those approaches: we will provide auto-detection based on PATH, but if it'll not work for the user in a specific case (like having multiple SDK version) we should also provide a cli option in order to let the user get a working exe and not be blocked.

EXE workflow: users will have to clone the repository or activate the package globally first to download the package source code. After that, they should compile it using dart compile exe. The upgrade process requires repeating these steps.

I think we can choose an approach with package managers and distribute precompiled version like fvm does, for example. This approach will not require any action from the user, but to download the precompiled executable and call it.

incendial · 2021-08-25T08:15:41Z

But we can't move to this step until we have no doubts about the package working correctly and without SDK detection it won't work, so I think we should definitely finish this improvement and then consider a distribution through package managers.

roman-petrov · 2021-08-25T09:15:09Z

So:

we will use the --sdk-path option that I already implemented.
I will update this PR and implement sdk path detection from PATH environment variable: this should work if the application process started on Windows as .exe file
.exe distribution will be not part of this PR and should be discussed/implemented in another issue?

@incendial, is that correct?..

incendial · 2021-08-25T09:27:22Z

Yeah, that's how I imagined it. Completing the first two steps will unblock use-cases with local compilation (like you have) and let you speed up the pipeline. Then we'll move forward to improving the DX with precompiled distribution.

Will this work?

roman-petrov · 2021-08-25T09:45:06Z

I think this would be ok and I like the result we got. I will continue my work on this PR and implement sdk path detection from PATH.

BTW, I will have a two-week vacation in a few days, so I think this PR will be completed only after that. Please let me know if it's ok or just feel free to take ownership of this branch if you like to deliver it faster.

incendial · 2021-08-25T12:50:56Z

BTW, I will have a two-week vacation in a few days, so I think this PR will be completed only after that.

Sure, no problem.

roman-petrov · 2021-09-16T10:29:47Z

@incendial I completed Dart SDK detection functionality here. I tested EXE compilation on my machine and the compiled application seems to be working just fine without --sdk-path argument.

Can you please take a look if the detection logic fits our needs?

roman-petrov · 2021-09-16T13:01:15Z

We had some discussion in Telegram today and I've got just one more question here: should we support non Windows platforms somehow when compiling to native executables?...

@incendial , @dkrutskikh

roman-petrov · 2021-09-23T13:32:22Z

We had some discussion in Telegram today and I've got just one more question here: should we support non Windows platforms somehow when compiling to native executables?...

@incendial , @dkrutskikh

Today I installed Dart on Ubuntu (I have almost no experience in *nix) development. I thought that dart compile on the non-Windows platform is able to build native executables. When I ran dart compile I see

So if I understand correctly, exe is the only native executable format and it is not possible to build a native executable for *nix platforms (even running dart on these platforms)? Do you have any knowledge in this area? (as I said, I am too inexperienced in *nix)

incendial · 2021-09-26T08:57:12Z

@roman-petrov the docs says that exe can be run on different platforms https://dart.dev/tools/dart-compile#exe and I've tested it on macos and it looks fine. So, If there is nothing else you want to add/change in the current implementation, I think we can merge it.

roman-petrov · 2021-09-26T10:37:11Z

@incendial - it's a good news. Do you have any code comments for current implementation?

I see that codecov found coverage regression, I will try to add some tests before we merge.

incendial

Let's not mention Windows anyhow, I think it should work properly on any platform. Also, could you add the docs link https://dart.dev/tools/dart-compile#exe somewhere?

Everything else is cool, I think we can merge it after you add tests for validateSdkPath and findSdkPath.

roman-petrov · 2021-09-27T07:42:23Z

@incendial , sure, I will add the link to the https://dart.dev/tools/dart-compile#exe somewhere, if I understand correctly, do we need it in user docs?

incendial · 2021-09-27T14:54:00Z

@roman-petrov I think adding it into the description of the argument should be enough https://github.com/dart-code-checker/dart-code-metrics/pull/430/files#diff-34a9cc97d58062ac8d9ab552da6d66e78e8e3d41a0d1859c7561c8115d5d2366R47

roman-petrov · 2021-09-29T06:42:23Z

@incendial - thank you, I will add the link. In a few days I will update this PR and add tests too.

I still have a question about non-Windows platforms. At the moment as you can see in the code Dart SDK detection is explicitly enabled only for Windows platform.

You said you tested this PR on macOS and it works fine too. So I am a bit stuck here, how my implementation can affect macOS... I assume that for non-Windows platforms this PR does nothing more than adding the --sdk-path CLI parameter, Dart SDK detection is disabled. Did you perform the test on macOS using this new parameter, can you please clarify?...

incendial · 2021-10-03T08:48:15Z

@roman-petrov okay, now I got your point. Yeah, I've tested compilation without the new parameter and the idea "it should work properly on any platform" was about the compilation too.

So, yeah, let's keep mentioning Windows for now and when we'll return to distributing compiled version we'll update the parameter if needed.

roman-petrov · 2021-10-11T17:41:36Z

@incendial - sorry for late response. I was ill and still recovering, hope to get well and finish this this or next week.

incendial · 2021-10-11T18:13:02Z

No problem, I hope you'll get better soon!

roman-petrov · 2021-10-19T13:04:27Z

I am finally very excited to get well and get back to work:) I finished working on this, PTAL @incendial , @dkrutskikh

I addressed review comments, added tests and also moved some duplicated code into BaseCommand class. Still see some duplication between three commands we have, but this seems to be out of my pull request scope.

I will make follow up PR into website repository once this gets merged.

incendial · 2021-10-22T07:44:05Z

@roman-petrov sorry, but I'm a bit overloaded right now, I'll try to reply to all your comments and PR's in a few days.

roman-petrov · 2021-10-22T08:00:17Z

@incendial - no problem, take your time.

incendial

A small comment on validateCommand changes, everything else looks great.

lib/src/cli/commands/base_command.dart

CHANGELOG.md

README.md

lib/src/analyzers/lint_analyzer/lint_analyzer.dart

lib/src/analyzers/unused_files_analyzer/unused_files_analyzer.dart

lib/src/analyzers/unused_l10n_analyzer/unused_l10n_analyzer.dart

roman-petrov changed the title ~~[WIP] feat: implement --sdk-path CLI option to use when running as Window…~~ [WIP] feat: implement --sdk-path CLI option to use when running as Windows EXE Aug 20, 2021

roman-petrov changed the title ~~[WIP] feat: implement --sdk-path CLI option to use when running as Windows EXE~~ feat: implement --sdk-path CLI option to use when running as Windows EXE Sep 16, 2021

incendial self-requested a review September 26, 2021 11:14

incendial added the area-core label Sep 26, 2021

incendial added this to the 4.4.0 milestone Sep 26, 2021

incendial reviewed Sep 26, 2021

View reviewed changes

dkrutskikh removed this from the 4.4.0 milestone Oct 4, 2021

dkrutskikh modified the milestones: 4.5.0, 4.6.0 Oct 22, 2021

incendial suggested changes Oct 24, 2021

View reviewed changes

lib/src/cli/commands/base_command.dart Show resolved Hide resolved

Roman Petrov added 12 commits October 25, 2021 08:56

feat: implement --sdk-path CLI option to use when running as Windows EXE

087dcbe

docs: add link to official docs

6bed958

fix: add "sdk-path" option to "CheckUnusedL10nCommand"

8b8d65e

test: add tests for BaseCommand.validateSdkPath and detectSdkPath

fe2f203

chore: fix analyzer issue

ee1ccc8

fix: use sdk-path in check_unused_l10n.dart

cbd5612

refactor: remove code duplication

3a14c99

fix: remove unneeded override

597bc7f

refactor: address review comment

860eedb

test: add test for 'findSdkPath`

fa8b39b

test: add a test case for "detectSdkPath"

3798b1b

test: fix existing and add new test

dbe9db4

dkrutskikh suggested changes Oct 25, 2021

View reviewed changes

Roman Petrov added 3 commits October 25, 2021 15:04

docs: don't mention Windows platform

8b3c45e

docs: fix changelog entry

f553a99

refactor: convert "sdkPath" to named argument

b6dde60

dkrutskikh approved these changes Oct 25, 2021

View reviewed changes

roman-petrov changed the title ~~feat: implement --sdk-path CLI option to use when running as Windows EXE~~ feat: implement --sdk-path CLI option to use when running as compiled executable Oct 25, 2021

docs: don't mention Windows in CHANGELOG.md

9edcfbc

incendial approved these changes Oct 25, 2021

View reviewed changes

incendial merged commit 5c4d739 into dart-code-checker:master Oct 25, 2021

roman-petrov deleted the sdk-path-option branch October 26, 2021 06:13

feat: implement --sdk-path CLI option to use when running as compiled executable #430

feat: implement --sdk-path CLI option to use when running as compiled executable #430

Uh oh!

Conversation

roman-petrov commented Aug 20, 2021 • edited by incendial Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

roman-petrov commented Aug 20, 2021

Uh oh!

codecov bot commented Aug 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

incendial commented Aug 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

incendial commented Aug 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

roman-petrov commented Aug 22, 2021

Uh oh!

roman-petrov commented Aug 23, 2021

Uh oh!

roman-petrov commented Aug 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

roman-petrov commented Aug 25, 2021

Uh oh!

incendial commented Aug 25, 2021

Uh oh!

incendial commented Aug 25, 2021

Uh oh!

roman-petrov commented Aug 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

incendial commented Aug 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

roman-petrov commented Aug 25, 2021

Uh oh!

incendial commented Aug 25, 2021

Uh oh!

roman-petrov commented Sep 16, 2021

Uh oh!

roman-petrov commented Sep 16, 2021

Uh oh!

roman-petrov commented Sep 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

incendial commented Sep 26, 2021

Uh oh!

roman-petrov commented Sep 26, 2021

Uh oh!

incendial left a comment

Choose a reason for hiding this comment

Uh oh!

roman-petrov commented Sep 27, 2021

Uh oh!

incendial commented Sep 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

roman-petrov commented Sep 29, 2021

Uh oh!

incendial commented Oct 3, 2021

Uh oh!

roman-petrov commented Oct 11, 2021

Uh oh!

incendial commented Oct 11, 2021

Uh oh!

roman-petrov commented Oct 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

incendial commented Oct 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

roman-petrov commented Oct 22, 2021

Uh oh!

incendial left a comment

Choose a reason for hiding this comment

Uh oh!

feat: implement `--sdk-path` CLI option to use when running as compiled executable #430

feat: implement `--sdk-path` CLI option to use when running as compiled executable #430

roman-petrov commented Aug 20, 2021 •

edited by incendial

Loading

codecov bot commented Aug 20, 2021 •

edited

Loading

incendial commented Aug 22, 2021 •

edited

Loading

incendial commented Aug 22, 2021 •

edited

Loading

roman-petrov commented Aug 25, 2021 •

edited

Loading

roman-petrov commented Aug 25, 2021 •

edited

Loading

incendial commented Aug 25, 2021 •

edited

Loading

roman-petrov commented Sep 23, 2021 •

edited

Loading

incendial commented Sep 27, 2021 •

edited

Loading

roman-petrov commented Oct 19, 2021 •

edited

Loading

incendial commented Oct 22, 2021 •

edited

Loading