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

「複数プロセッサによるコンパイル」を有効にする #103

Merged
merged 1 commit into from
Jun 12, 2018

Conversation

yoshinrt
Copy link
Contributor

  • Release, Debug ビルドの,コンパイラオプションの「複数プロセッサによるコンパイル」を有効にしました.
  • 同時に Debug ビルドの「最小リビルド」を無効にしています.(上記オプションと同時指定できないので)

ヘッダファイルを修正すると殆どのファイルが再コンパイルされるので,結構時間短縮になると思います.

@m-tmatma
Copy link
Member

appveyor でのビルド

https://ci.appveyor.com/project/sakuraeditor/sakura/build/1.0.153 → 4分38秒 (#103 対応前)
https://ci.appveyor.com/project/sakuraeditor/sakura/build/1.0.150 → 4分40秒 (#103 対応後)

appveyor の環境
https://www.appveyor.com/docs/build-environment/#build-vm-configurations

以下バッチファイルでのローカルビルド

set SLN_FILE=sakura.sln
set PLATFORM=Win32
set CONFIGURATION=Release

set MSBUILD_EXE="C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\MSBuild\15.0\Bin\MSBuild.exe"

echo %time% > log.txt
echo %MSBUILD_EXE% %SLN_FILE% /p:Platform=%PLATFORM% /p:Configuration=%CONFIGURATION%      /t:"Build"
     %MSBUILD_EXE% %SLN_FILE% /p:Platform=%PLATFORM% /p:Configuration=%CONFIGURATION%      /t:"Build"
echo %time% >> log.txt

e44fc1a (対応前) → 1分03秒
dd7ccff (対応後) → 42 秒

ローカルビルドに使用した環境

Operating System: Windows 10 Pro 64-bit (10.0, Build 17134) (17134.rs4_release.180410-1804)
    Processor: Intel(R) Core(TM) i7-6500U CPU @ 2.50GHz (4 CPUs), ~2.6GHz
       Memory: 12288MB RAM

@m-tmatma
Copy link
Member

yoshinrt:master から PR を作成しておられますが
ブランチを作って対応したほうがいいです。

@yoshinrt
Copy link
Contributor Author

すみません,ブランチ切ったつもりだったのですが,pull req 出したあとで気づきました.
一旦取り下げたほうがいいのでしょうか?

@kobake
Copy link
Member

kobake commented Jun 11, 2018

PR ありがとうございます!
ブランチ名は本当は意味のあるもののほうが好ましいですがコミットログで意味伝わるので今回はそのままでも良いと思います

@m-tmatma
Copy link
Member

m-tmatma commented Jun 11, 2018

一旦取り下げたほうがいいのでしょうか?

これ自体は問題ではありません。

ただ、この PR が取り込まれるまでは @yoshinrt さんが master に本件以外の修正を
入れにくくなります。

@yoshinrt さんの master を編集すると自動的にこの PR に修正が積まれてしまうからです。

ただこの PR に関してどれぐらい速くなるのか、定量的なデータがほしいです。

@m-tmatma m-tmatma added the enhancement ■機能追加 label Jun 11, 2018
@m-tmatma
Copy link
Member

/MP に互換性のない機能として、/Yc が挙げられている。
https://msdn.microsoft.com/ja-jp/library/bb385193.aspx

StdAfx.cpp で /Yc を使ってるが、問題ないのだろうか?

@yoshinrt
Copy link
Contributor Author

yoshinrt:master から PR の件は,このままにさせていただきます.アドバイスありがとうございます.

ただこの PR に関してどれぐらい速くなるのか、定量的なデータがほしいです。

私の環境ですと (WIn10, AMD-A8 7670K Mem:8GB),以下のようになりました.
各 5回実行時の平均 (秒)

Debug Release
PR 適用前 86 79
PR 適用後 42 46

StdAfx.cpp で /Yc を使ってるが、問題ないのだろうか?

リンク先を見ますと,互換性のないオプションの組み合わせを用いると,警告を出して,停止または単に無視すると読めます.
少なくとも不正なコンパイルを行ってビルド完了するようなことはないと思いますが,いかがでしょうか.

@berryzplus
Copy link
Contributor

berryzplus commented Jun 11, 2018

PRありがとうございます。

/Yc はプリコンパイル済みヘッダーを作るためのオプションです。ビルドの最初に処理される工程で、ここを並列化するのは不可能です。pchが作られた後にだけ並列化できるという意味で矛盾するとなってるのかと思います。

並列化で速くなるのは主にファイル数が多い場合な認識です。サクラエディタはソースファイル(.cpp)が多いので有効だと思います。手元では実測してないですがデータで見ると倍近く速くなってますよね。ここまでとは思いませんでした 😄

@yoshinrt yoshinrt changed the title 複数プロセッサによるコンパイル」を有効にする 「複数プロセッサによるコンパイル」を有効にする Jun 11, 2018
@kobake
Copy link
Member

kobake commented Jun 12, 2018

かなり速くなったと思います!

  • Env: Windows 10 Home, Visual Studio Commuinty 2017
  • CPU: Intel(R) Core(TM) i7-6600U CPU @ 2.60GHz 2.81GHz
  • Memory: 16.0 GB

master Debug

3>Task Performance Summary:
3>      111 ms  MSBuild                                    4 calls
3>      353 ms  Exec                                       2 calls
3>      421 ms  RC                                         1 calls
3>    14055 ms  Link                                       1 calls
3>    88181 ms  CL                                         2 calls

master Release

3>Task Performance Summary:
3>       93 ms  MSBuild                                    4 calls
3>      208 ms  RC                                         1 calls
3>      491 ms  Exec                                       2 calls
3>     8375 ms  Link                                       1 calls
3>    58802 ms  CL                                         2 calls

yoshinrt/master Debug

3>Task Performance Summary:
3>      134 ms  MSBuild                                    4 calls
3>      249 ms  RC                                         1 calls
3>      533 ms  Exec                                       2 calls
3>    11391 ms  Link                                       1 calls
3>    42904 ms  CL                                         2 calls

yoshinrt/master Release

3>Task Performance Summary:
3>       35 ms  MSBuild                                    4 calls
3>      238 ms  RC                                         1 calls
3>      363 ms  Exec                                       2 calls
3>    20645 ms  Link                                       1 calls
3>    30777 ms  CL                                         2 calls

Copy link
Contributor

@berryzplus berryzplus left a comment

Choose a reason for hiding this comment

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

LGTMです。

@kobake kobake merged commit c14829c into sakura-editor:master Jun 12, 2018
@kobake
Copy link
Member

kobake commented Jun 12, 2018

ご対応ありがとうございました!

@m-tmatma
Copy link
Member

#108 を登録しました。

@yoshinrt
Copy link
Contributor Author

人生初 PR が無事終わってホッとしています.ご対応ありがとうございました.

@m-tmatma m-tmatma added this to the next release milestone Jun 12, 2018
@m-tmatma m-tmatma added the 🚅 speed up 🚀 高速化 label Jun 17, 2018
@ds14050 ds14050 added enhancement ■機能追加 🚅 speed up 🚀 高速化 labels Sep 18, 2018
@KENCHjp KENCHjp added the CI appveyor など CI 関連 【ChangeLog除外】 label Dec 5, 2018
HoppingTappy pushed a commit to HoppingTappy/sakura that referenced this pull request Jun 11, 2019
「複数プロセッサによるコンパイル」を有効にする
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🚅 speed up 🚀 高速化 CI appveyor など CI 関連 【ChangeLog除外】 enhancement ■機能追加
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants