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

CViewSelect::PrintSelectionInfoMsg の高速化、及び行のEOL長を取得する処理の高速化 #993

Merged
merged 2 commits into from
Aug 15, 2019
Merged

CViewSelect::PrintSelectionInfoMsg の高速化、及び行のEOL長を取得する処理の高速化 #993

merged 2 commits into from
Aug 15, 2019

Conversation

beru
Copy link
Contributor

@beru beru commented Aug 14, 2019

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

現在のEOL長を取得する処理の高速化
@beru beru added the 🚅 speed up 🚀 高速化 label Aug 14, 2019
@beru beru changed the title CViewSelect::PrintSelectionInfoMsg の高速化、及び現在のEOL長を取得する処理の高速化 CViewSelect::PrintSelectionInfoMsg の高速化、及び行のEOL長を取得する処理の高速化 Aug 14, 2019
@AppVeyorBot
Copy link

Build sakura 1.0.2134 completed (commit be5be12080 by @beru)

@berryzplus
Copy link
Contributor

改善の方向性としてGOな判断です。

CEol::GetLen メソッドもアプリケーションの色々なところから呼び出されているのでインライン化されるようにヘッダ側に記述を移しました。

サクラエディタのRelease版ビルド設定は、CMakeでいうRelWithDebInfo相当なので、ヘッダに移しただけだと(=inlineを付けないと)効果がないはずです。

行終端子の長さのテーブルもヘッダファイル CEol.h 側に用意しました。CEol.cpp ファイルにある g_aEolTable 配列と一部内容が重複しているので保守の手間が増えますが処理時間を削減するには必要な事です。

似た配列を複製するのではなく、元配列をヘッダ側で参照できるようににしたいです。
extern struct XXX g_aEolTable[8]; にしたらいいんじゃないかと言ってます。型、数字テキトー。

2^24 (16777216) 行あるファイルを開いた後に、Ctrl + A を押して全選択した後に Shift キーを押しながらカーソルキーを押して選択範囲を変更すると結構操作のレスポンスが良くない事が確認出来ます。

事象確認のために使うファイルの行数が、常用外レベルに大きくなってる点についてはスルーします。

従来実装では CLayoutMgr::GetLineStr メソッドが使われていましたが、CLayoutMgr::SearchLineByLayoutY メソッドで置き換えました。
取得した値のうちレイアウトデータしか使っていなかったので、CLayoutMgr::GetLineStr メソッドは使うまでも無いと判断したからです。

妥当な判断だと思います。

ちなみに、これらの関数の引数型 CLayoutInt というのは int ベースの独自型です。
一年くらい前にissue登場済みの、やや問題ありな型です。
#326 マクロ関数の選択開始桁と選択終了桁で取得できる数値が桁数でない

Mery というテキストエディタがありますが、このテキストエディタは様々な操作に掛かる時間がサクラエディタより大分高速です。

ぼくの中では サクラエディタ > VS Code > Visual Studio な感じです。
最近は Java 触らなくなったので他のエディタはほぼ使わなくて済んでいます。
Meryへのライバル意識の発露のさせ方としては「絵文字対応」なんてのもあります。

っていうか、MinGWビルド直さないといかんですね・・・。

@beru
Copy link
Contributor Author

beru commented Aug 14, 2019

サクラエディタのRelease版ビルド設定は、CMakeでいうRelWithDebInfo相当なので、ヘッダに移しただけだと(=inlineを付けないと)効果がないはずです。

x64 Release ビルドで確認しましたが付けないでもインライン化されています。
クラス定義内のメソッドはデフォルトでインライン化されるからみたいです。

https://stackoverflow.com/a/13687207/4699324

似た配列を複製するのではなく、元配列をヘッダ側で参照できるようににしたいです。
extern struct XXX g_aEolTable[8]; にしたらいいんじゃないかと言ってます。型、数字テキトー。

その場合は SEolDefinitionForUniFile 構造体の定義をヘッダ側に移す必要があると思います。

事象確認のために使うファイルの行数が、常用外レベルに大きくなってる点についてはスルーします。

確かに極端な例なんですよね。隠れている問題を表面化するのには役立ちますがもうちょっと現実的なユースケースでの改善を試みないといかんですね。。

ちなみに、これらの関数の引数型 CLayoutInt というのは int ベースの独自型です。
一年くらい前にissue登場済みの、やや問題ありな型です。
#326 マクロ関数の選択開始桁と選択終了桁で取得できる数値が桁数でない

リリースビルドでは、#define USE_STRICT_INT されないので通常の int になるので問題ないのではないでしょうか?(訳:問題無いアル)

#326 は久しぶりに見ました。懐かしいです。

ぼくの中では サクラエディタ > VS Code > Visual Studio な感じです。
最近は Java 触らなくなったので他のエディタはほぼ使わなくて済んでいます。
Meryへのライバル意識の発露のさせ方としては「絵文字対応」なんてのもあります。

サクラエディタは色々なんとかしたい点がありますが「絵文字対応」は個人的には優先度が低いところです。サロゲートペア文字の扱いとか大変そうだし、ブラウザで見ればいいし…。

@berryzplus
Copy link
Contributor

サクラエディタのRelease版ビルド設定は、CMakeでいうRelWithDebInfo相当なので、ヘッダに移しただけだと(=inlineを付けないと)効果がないはずです。

x64 Release ビルドで確認しましたが付けないでもインライン化されています。
クラス定義内のメソッドはデフォルトでインライン化されるからみたいです。
https://stackoverflow.com/a/13687207/4699324

C++11(=言語仕様)で既定されてるんですね。
マニュアルも更新されてる感じです(/Ob1)。
(ms docsに誤植見つけたので指摘してきました 😄)

似た配列を複製するのではなく、元配列をヘッダ側で参照できるようににしたいです。
※ extern struct XXX g_aEolTable[8]; にしたらいいんじゃないかと言ってます。型、数字テキトー。

その場合は SEolDefinitionForUniFile 構造体の定義をヘッダ側に移す必要があると思います。

ブロックごとヘッダ側にコピペで何か問題あるんでしたっけ?
ってあれ?ForUniFileのほうでしたっけ?
g_aEolLengths に設定された定数を見た感じ、Uniなし版がベースのような。

ちなみにこんな感じでやってみたらビルドは通りました。

ヘッダ側
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です 😭

サクラエディタのなんとかしたい点
実は Shift_JIS の特定カテゴリの文字を表示できない、って不具合があったりします。

@beru
Copy link
Contributor Author

beru commented Aug 15, 2019

その場合は SEolDefinitionForUniFile 構造体の定義をヘッダ側に移す必要があると思います。

ブロックごとヘッダ側にコピペで何か問題あるんでしたっけ?
ってあれ?ForUniFileのほうでしたっけ?
g_aEolLengths に設定された定数を見た感じ、Uniなし版がベースのような。

あ、SEolDefinitionForUniFile ではなくて SEolDefinition の方でした。構造体定義もヘッダに移しても多分問題は起きないと思います。

ちなみにこんな感じでやってみたらビルドは通りました。

お、確認ありがとうございます。

レイアウト単位というコトバを、行を数える単位、桁を数える単位、画面上のピクセルを数える単位の3種類の意味に使っていて、何気にとり違いが起きてるっていう問題でした・・・。そのうちなんとかしたいですが、直接的には放置でOKです 😭

過去に範囲選択領域の変更がピクセル単位になってしまって困ってましたが #738 で過去パッチを取り込んで解消したのを思い出しました。

今どういう問題が起きているか把握していませんが、知らぬが仏、という事でしょうか?

サクラエディタのなんとかしたい点
実は Shift_JIS の特定カテゴリの文字を表示できない、って不具合があったりします。

JIS第1水準以外の文字は憲法で禁止してほしいです。

@AppVeyorBot
Copy link

Build sakura 1.0.2136 completed (commit ecba3164b9 by @beru)

@berryzplus
Copy link
Contributor

/azp run

@berryzplus
Copy link
Contributor

↑rebaseしないと意味無いんだった...orz

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@berryzplus
Copy link
Contributor

レイアウト単位というコトバを、行を数える単位、桁を数える単位、画面上のピクセルを数える単位の3種類の意味に使っていて、何気にとり違いが起きてるっていう問題でした・・・。そのうちなんとかしたいですが、直接的には放置でOKです 😭

過去に範囲選択領域の変更がピクセル単位になってしまって困ってましたが #738 で過去パッチを取り込んで解消したのを思い出しました。

今どういう問題が起きているか把握していませんが、知らぬが仏、という事でしょうか?

桁位置取得のマクロ関数の挙動を、まだ直せてなかった気がします。
他には、アウトライン解析の内部処理で行桁の取り違いが発生していて、おそらく潜在バグなんだけど「報告もあがってないので放置」という状態が続いております:smile:

サクラエディタのなんとかしたい点
実は Shift_JIS の特定カテゴリの文字を表示できない、って不具合があったりします。

JIS第1水準以外の文字は憲法で禁止してほしいです。

ロボット憲法第4条・・・

今週中には PR できそうなんで書いてしまいました。
サクラエディタでは元ドキュメントを破壊しないために、SJIS⇒UNICODE変換ができてもUNICODE⇒SJIS変換で元に戻らない文字をバイナリ扱いにしています。この処理でバイナリ扱いになった文字(本来はUnicode変換できる)を文字として表示する機構がないので、表示上は文字化けしたように見えてしまうわけです。PR出すときに詳細出しますが、いわゆるOEM拡張文字の一部が表示できない状態になっとるわけです。(IBM拡張かNEC拡張かどっちだったか忘れました)

ほぼ関係ない話でした・・・

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.

というわけで、問題ないと思います。
対応ありがとうございます。

@beru beru merged commit f6ad684 into sakura-editor:master Aug 15, 2019
@beru
Copy link
Contributor Author

beru commented Aug 15, 2019

@berryzplus さん

レビューありがとうございました。問題が見つかったら別のPRで対処します。

@beru beru deleted the CViewSelect__PrintSelectionInfoMsg branch August 15, 2019 06:09
@m-tmatma m-tmatma added this to the v2.4.0 milestone Dec 29, 2019
HoppingTappy pushed a commit to HoppingTappy/sakura that referenced this pull request Jun 16, 2020
…ctionInfoMsg

CViewSelect::PrintSelectionInfoMsg の高速化、及び行のEOL長を取得する処理の高速化
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants