-
Notifications
You must be signed in to change notification settings - Fork 167
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
CViewSelect::PrintSelectionInfoMsg の高速化、及び行のEOL長を取得する処理の高速化 #993
CViewSelect::PrintSelectionInfoMsg の高速化、及び行のEOL長を取得する処理の高速化 #993
Conversation
現在のEOL長を取得する処理の高速化
✅ Build sakura 1.0.2134 completed (commit be5be12080 by @beru) |
改善の方向性としてGOな判断です。
サクラエディタのRelease版ビルド設定は、CMakeでいうRelWithDebInfo相当なので、ヘッダに移しただけだと(=inlineを付けないと)効果がないはずです。
似た配列を複製するのではなく、元配列をヘッダ側で参照できるようににしたいです。
事象確認のために使うファイルの行数が、常用外レベルに大きくなってる点についてはスルーします。
妥当な判断だと思います。 ちなみに、これらの関数の引数型 CLayoutInt というのは int ベースの独自型です。
ぼくの中では サクラエディタ > VS Code > Visual Studio な感じです。 っていうか、MinGWビルド直さないといかんですね・・・。 |
x64 Release ビルドで確認しましたが付けないでもインライン化されています。 https://stackoverflow.com/a/13687207/4699324
その場合は
確かに極端な例なんですよね。隠れている問題を表面化するのには役立ちますがもうちょっと現実的なユースケースでの改善を試みないといかんですね。。
リリースビルドでは、 #326 は久しぶりに見ました。懐かしいです。
サクラエディタは色々なんとかしたい点がありますが「絵文字対応」は個人的には優先度が低いところです。サロゲートペア文字の扱いとか大変そうだし、ブラウザで見ればいいし…。 |
C++11(=言語仕様)で既定されてるんですね。
ブロックごとヘッダ側にコピペで何か問題あるんでしたっけ? ちなみにこんな感じでやってみたらビルドは通りました。 ヘッダ側
struct SEolDefinition{
const TCHAR* m_szName;
const WCHAR* m_szDataW;
const ACHAR* m_szDataA;
int m_nLen;
bool StartsWith(const WCHAR* pData, int nLen) const{ return m_nLen<=nLen && 0==auto_memcmp(pData,m_szDataW,m_nLen); }
bool StartsWith(const ACHAR* pData, int nLen) const{ return m_nLen<=nLen && m_szDataA[0] != '\0' && 0==auto_memcmp(pData,m_szDataA,m_nLen); }
};
extern const SEolDefinition g_aEolTable[];
実装ファイル側
const SEolDefinition g_aEolTable[] = {
// ↑先頭のstatic外しただけ
レイアウト単位というコトバを、行を数える単位、桁を数える単位、画面上のピクセルを数える単位の3種類の意味に使っていて、何気にとり違いが起きてるっていう問題でした・・・。そのうちなんとかしたいですが、直接的には放置でOKです 😭 サクラエディタのなんとかしたい点 |
あ、
お、確認ありがとうございます。
過去に範囲選択領域の変更がピクセル単位になってしまって困ってましたが #738 で過去パッチを取り込んで解消したのを思い出しました。 今どういう問題が起きているか把握していませんが、知らぬが仏、という事でしょうか?
JIS第1水準以外の文字は憲法で禁止してほしいです。 |
✅ Build sakura 1.0.2136 completed (commit ecba3164b9 by @beru) |
/azp run |
↑rebaseしないと意味無いんだった...orz |
Azure Pipelines successfully started running 1 pipeline(s). |
桁位置取得のマクロ関数の挙動を、まだ直せてなかった気がします。
ロボット憲法第4条・・・ 今週中には PR できそうなんで書いてしまいました。 ほぼ関係ない話でした・・・ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
というわけで、問題ないと思います。
対応ありがとうございます。
@berryzplus さん レビューありがとうございました。問題が見つかったら別のPRで対処します。 |
…ctionInfoMsg CViewSelect::PrintSelectionInfoMsg の高速化、及び行のEOL長を取得する処理の高速化
PR の目的
選択範囲情報メッセージの表示処理を速くする事でレスポンスを上げるのが目的です。
ステータスバーを表示している場合は選択範囲の文字数がそこに表示されます。
CViewSelect::PrintSelectionInfoMsg
メソッドは選択範囲情報メッセージを作成してステータスバーに表示する処理ですが、そこの記述を更新しました。主に最適化したのは選択文字数をカウントする処理です。カテゴリ
PR の背景
ステータスバーを表示している場合の選択範囲操作のレスポンスが上がります。
2^24 (16777216) 行あるファイルを開いた後に、Ctrl + A を押して全選択した後に Shift キーを押しながらカーソルキーを押して選択範囲を変更すると結構操作のレスポンスが良くない事が確認出来ます。
実はこのPRの変更よりも #985 を適用した後の方が効果が高いです。それはレイアウト情報の確保をメモリプールを使って行っているので(メモリの利用効率が良くなった為に)キャッシュヒット率が向上している為と考えられます。
Mery というテキストエディタがありますが、このテキストエディタは様々な操作に掛かる時間がサクラエディタより大分高速です。
最近の一連のPRでパフォーマンスの改善を行う事で速度差を縮める事が出来ればと考えています。
このPRでの細かい変更内容について記載します。
従来実装では
CLayoutMgr::GetLineStr
メソッドが使われていましたが、CLayoutMgr::SearchLineByLayoutY
メソッドで置き換えました。取得した値のうちレイアウトデータしか使っていなかったので、
CLayoutMgr::GetLineStr
メソッドは使うまでも無いと判断したからです。CEol::GetLen
メソッドもアプリケーションの色々なところから呼び出されているのでインライン化されるようにヘッダ側に記述を移しました。行終端子の長さのテーブルもヘッダファイル
CEol.h
側に用意しました。CEol.cpp
ファイルにあるg_aEolTable
配列と一部内容が重複しているので保守の手間が増えますが処理時間を削減するには必要な事です。PR のメリット
ステータスバーを表示しているケースで、行数が多い文書を扱っている際の範囲選択操作のレスポンスを改善します。
PR のデメリット (トレードオフとかあれば)
CEol::GetLen
メソッドをインライン化する為にヘッダーファイルにテーブルを新規追加したので保守の手間が増えます。PR の影響範囲
CViewSelect::PrintSelectionInfoMsg
メソッド関連CEol::GetLen
メソッド関連関連チケット
#985