ルーラー描画の高速化 #1067

beru · 2019-10-06T03:35:06Z

PR の目的

ルーラー描画の高速化が目的です。同等の処理をより少ない負担で行う事で効率化を図ります。

カテゴリ

速度向上

PR の背景

ルーラーの背景描画で目盛り線を一本ずつ描くより、PolyPolyline 関数を使用してまとめて描くことで処理時間の短縮が出来るのではないかと考え変更しました。

Performance Profiler で確認する限りでは、DrawRulerBg に 3% 程度掛かっていたのが 1% 程度に減りました。

フォントのハンドルや PolyPolyline 関数のパラメータとして使用する動的配列はローカル変数ではなく CRuler クラスのメンバー変数にして再利用する事で、呼び出すたびに毎回作成と破棄される事が無いようにしました。

PR のメリット

メリットとしては描画処理が幾分か軽くなります。

PR のデメリット (トレードオフとかあれば)

実装が少し複雑になります。

PR の影響範囲

ルーラーの描画処理

AppVeyorBot · 2019-10-06T03:50:30Z

✅ Build sakura 1.0.2293 completed (commit 9af5f1fbd9 by @)

berryzplus · 2019-10-06T05:44:49Z

ついに来ましたね、この変更がｗ
まだ内容みてないですが、いつかはやりたいとずっと考えていた変更っす。

beru · 2019-10-06T06:12:30Z

sakura_core/view/CRuler.cpp

 		}
 		//10目盛おきの区切り(大)と数字
 		else if( 0 == keta % 10 ){
 			wchar_t szColumn[32];
-			::MoveToEx( gr, nX, nY, NULL );
-			::LineTo( gr, nX, 0 );
+			apt[idx * 2 + 1] = POINT{nX, 0};
 			_itow( ((Int)keta) / 10, szColumn, 10 );
 			::TextOut( gr, nX + 2 + 0, -1 + 0, szColumn, wcslen( szColumn ) );


ここって TextOut を複数回呼び出す代わりに PolyTextOut で一括で行うようにしたら処理負荷って下がるんでしょうか？ちょっと実験してみます。

そればっかりはやって見ないと分からん感じです。
Poly系の関数はデバイスが該当描画処理を標準装備している場合に効果を発揮するはずです。
複数の線分はイケる可能性が高いですが、テキストはどうか分からんですw

berryzplus

内容確認しました。とくに問題ないと思います。

軽微な修正依頼が2件ありますので対応をお願いしたいです。
感想がいっぱいありますが、対応は必須でないです。

sakura_core/view/CRuler.cpp

berryzplus · 2019-10-06T05:52:39Z

sakura_core/view/CRuler.cpp

+		lf.lfClipPrecision	= 2;
+		lf.lfQuality		= 1;
+		lf.lfPitchAndFamily	= 34;
+		wcscpy( lf.lfFaceName, L"Arial" );


これは別件です。

フォントセット Arial が入ってないバージョンのWindowsは存在しないと思いますが、属性を指定して適切なフォントをwindowsに選択させる方式のが好ましい気がしています。

どういう属性にするの？とか横展開対象になるフォントってどれ？とか課題が多いので別件です。

人によっては漢数字で表示してほしいかもしれないですね。その前に縦書き表示や右書きにも対応するべきかもしれませんが（自分は必要を感じていないので）誰か違う人が対処してくれると思います。

berryzplus · 2019-10-06T05:59:26Z

sakura_core/view/CRuler.cpp

+		m_hFont = ::CreateFontIndirect( &lf );
+		m_nRulerHeight = pCommon->m_sWindow.m_nRulerHeight;
+	}
+	assert(m_hFont != NULL);


このassertがfailするときの挙動を、いつか真面目に考えないといかんと思っとります。
たぶん、ガチスルーでよいはず。

基本的には起こりえないエラーについては Debug ビルドで軽くみておくぐらいで良いんじゃないかなと思います。フリーソフトのテキストエディタにミッションクリティカルを求めるのも変な話なので。

ガチスルーの意図

if (!m_hFont) return; //後続の処理を実行しない

です。

ガチスルーとはそういう事でしたか。話題自体をスルーする事かと思ってました。

フォント作成が失敗するような状況だと return 後の処理が正常に動く保証も無いような気がしますが、エラーチェックを入れないより大分良い気がしてきました。

sakura_core/view/CRuler.cpp

berryzplus · 2019-10-06T06:21:26Z

sakura_core/view/CRuler.cpp

+#else
+	const int nWidth = m_pEditView->GetTextArea().GetRightCol() - i;
+#endif
+	const size_t nLinesToDraw = 1 + std::min<int>((nWidth + 1 + 1 + oneColumn - 1) / oneColumn, nMaxLineKetas - keta + 1);


この式の確からしさを検証できるドキュメントが欲しいです。

左辺の変数名は「ドキュメントが要らないコード」の要件を満たしている気がします。
右辺の数式がほぼほぼ意味不明なので判断がつかんとです。

while 文が回る回数を計算してます。

ええと、左辺の変数名から利用方法は理解していて、わからんのはその数式が正しいといえる根拠です。

const size_t nLinesToDraw = 1 + std::min<int>( (nWidth + 1 + 1 + oneColumn - 1) / oneColumn, nMaxLineKetas - keta + 1);

nLinesToDraw (縦線を引く本数＝ループの回数)　＝　1 + 以下A,B のうち小さいほうの値

A = （nWidth(描画範囲の桁数) + 1 + 1 + oneColumn(1桁あたりのピクセル数)）÷　oneColumn(1桁あたりのピクセル数)

B = nMaxLineKetas(折り返し桁数) - keta(テキストエリアの左端桁数を1桁あたりのピクセル数で割ったもの) + 1

日本語にして見ると なんでやねん！ の嵐です。

とりあえず、「既存と同じです」なら「ま、いっか」となるんですけど、なんで？のもっともらしい理由づけが欲しいです。

元の実装の記述の意味合いや厳密性については考えていませんが、while 文での扱いと同じ結果になるように合わせた式にしています。

std::min の左側の 1 + は、下線 (ルーラーと本文の境界) の分です。

もちょっと細かく解説した方が良いでしょうか？

もちょっと細かく解説した方が良いでしょうか？

冗談でなく、本当に理解できてないです。

改めてコードを眺めた感じの見解が以下です。

nWidth + 1　＝　描画領域の右端桁位置　－　描画領域の左端桁位置　＝　描画範囲の桁数

+ 1 は見切れ分を考慮した保険的な増分だと思っています

+ 1(2個目のやつ)　＝　ルーラーの区切り線（コメント中は「下線」と表現されている）を引くために必要な値。

oneColumn - 1　＝　謎の値。動作を見る限り、oneColumn == 1 っぽいので無視してよさげ。

/ oneColumn　＝　謎の値。動作を見る限り、oneColumn == 1 っぽいので無視してよさげ。

この見解で合ってるとすると、スジが通ってると思います。
ただ、oneColumn - 1と/ oneColumnは不要なので分かりやすさを担保するために削除したいっす。

ただ、oneColumn - 1と/ oneColumnは不要なので分かりやすさを担保するために削除したいっす。

このコメント取消。
害はないと思うので CI ビルドの完了を待って approve 予定です。

const size_t nLinesToDraw = 1 + std::min<int>((nWidth + 1 + 1 + oneColumn - 1) / oneColumn, nMaxLineKetas - keta + 1);

は

nLinesToDraw（何本描画するか）= 1（下線(ルーラーと本文の境界)の1本分） + 後述する A と B のうちの小さい値

で、A と B から小さい値を選ぶ理由は while 文の条件式で && が有って両方の条件を満たさないとループが中断されるからです。

A

nWidth = m_pEditView->GetTextArea().GetRightCol() - i

なので、

A = (m_pEditView->GetTextArea().GetRightCol() - i + 1 + 1 + oneColumn - 1) / oneColumn

ですが、nWidth を別変数にしたのは CLayoutInt を int に暗黙的な変換が出来なくてコンパイルエラーが起きたためです（両方とも Int にキャストすれば回避出来るのを知らなかった）。

さて、while 文の条件式は下記のようになっていて、

i <= m_pEditView->GetTextArea().GetRightCol() + 1

ループ内では

i += oneColumn

となっています。なので変数 i に oneColumn を何回足したら m_pEditView->GetTextArea().GetRightCol() + 1 を超えた値になるか、を考えます。

この場合に除算が使えます。余りが出た場合に +1 する為に 割る数 - 1 を足してから割るという慣用的な記述を使っています。

+ 1 が何故2つ有るのかについては、元々の条件式にある + 1 の分と 2個目の + 1 については元々の条件式で <= が使われているからです。

元々の条件式に +1 が有るのは何かの保険の為でしょうか？そこについては理解してません。

B

B = 折り返し桁数 - 目盛りを描く最初の桁数 + 1

+ 1 するのは、while 文の条件式で <= としているからです。

sakura_core/view/CRuler.h

berryzplus · 2019-10-06T06:31:58Z

sakura_core/view/CRuler.cpp

+		m_nRulerHeight = pCommon->m_sWindow.m_nRulerHeight;
+	}
+	assert(m_hFont != NULL);
+	HFONT hFontOld = (HFONT)::SelectObject( gr, m_hFont );


感想です。

gr.PushMyFont(m_hFont) としても良いような気がします。

メソッド名に My が入っているのが嫌です。

sakura_core/view/CRuler.cpp

berryzplus · 2019-10-06T06:47:45Z

sakura_core/view/CRuler.cpp


 		nX += dx;
 		i  += oneColumn;
 		keta++;
 	}
+	::PolyPolyline(gr, &apt[0], &asz[0], (DWORD)nLinesToDraw);


感想です。

趣味ですが、最後の引数で渡す値は asz.size() と一致していたほうが分かりやすい気がします。
std::vector は確保済み配列サイズと有効データ個数を別に管理できます。

reserveで確保サイズを決定し、
処理の冒頭で resize(0) して push_back していく構成にすれば、累積した｛点，点｝の追加を取り違えるリスクを排除できます。

たぶん、PolyPolyLineの呼出直前に線分の数を計算するのが有効なはず。

asz.assign(nLinesToDraw, 2);

で、nLinesToDraw の実態は apt.size() / 2 だと思うので、こうなるはず。

asz.assign(apt.size() / 2, 2); ::PolyPolyline(gr, apt.data(), asz.data(), static_cast<DWORD>(asz.size()));

その方法は安全で間違いが起きないかもしれませんが、途中でメモリの再確保が行われるかもしれません。事前計算出来る場合にはメモリを一括で確保していくやり方が良いと思います。

出たとこ勝負で push_back する方式のメリットは、
処理前に正確なメモリ使用量の見積もりを行う必要がないことにあると思います。

ルーラーで表示できる桁目盛りの数なんて高が知れてるので、全部動的に追加確保しても問題ないような気もします。まぁ、このコメントは感想なので、具体的になにか対応してほしいわけでもなく、指摘として言ってみてるだけなんで気にしないでください 😄

人によっては1行80桁の制限を頑なに守り続けるかもしれませんが、将来的にモニタの解像度が異常に上がって頭髪の平均密度を越す勢いで~~ルーター~~ルーラーの桁目盛りが描画される時代が来るかもしれません。

AppVeyorBot · 2019-10-06T07:56:06Z

✅ Build sakura 1.0.2295 completed (commit e2562f2ce4 by @beru)

berryzplus

たまってたコメントを放流・・・。

berryzplus · 2019-10-06T07:34:42Z

sakura_core/view/CRuler.cpp

 		}
 		//10目盛おきの区切り(大)と数字
 		else if( 0 == keta % 10 ){
 			wchar_t szColumn[32];
-			::MoveToEx( gr, nX, nY, NULL );
-			::LineTo( gr, nX, 0 );
+			apt[idx * 2 + 1] = POINT{nX, 0};
 			_itow( ((Int)keta) / 10, szColumn, 10 );
 			::TextOut( gr, nX + 2 + 0, -1 + 0, szColumn, wcslen( szColumn ) );


そればっかりはやって見ないと分からん感じです。
Poly系の関数はデバイスが該当描画処理を標準装備している場合に効果を発揮するはずです。
複数の線分はイケる可能性が高いですが、テキストはどうか分からんですw

sakura_core/view/CRuler.h

berryzplus · 2019-10-06T08:08:18Z

sakura_core/view/CRuler.cpp

+#else
+	const int nWidth = m_pEditView->GetTextArea().GetRightCol() - i;
+#endif
+	const size_t nLinesToDraw = 1 + std::min<int>((nWidth + 1 + 1 + oneColumn - 1) / oneColumn, nMaxLineKetas - keta + 1);


ええと、左辺の変数名から利用方法は理解していて、わからんのはその数式が正しいといえる根拠です。

const size_t nLinesToDraw = 1 + std::min<int>( (nWidth + 1 + 1 + oneColumn - 1) / oneColumn, nMaxLineKetas - keta + 1);

nLinesToDraw (縦線を引く本数＝ループの回数)　＝　1 + 以下A,B のうち小さいほうの値

A = （nWidth(描画範囲の桁数) + 1 + 1 + oneColumn(1桁あたりのピクセル数)）÷　oneColumn(1桁あたりのピクセル数)

B = nMaxLineKetas(折り返し桁数) - keta(テキストエリアの左端桁数を1桁あたりのピクセル数で割ったもの) + 1

日本語にして見ると なんでやねん！ の嵐です。

とりあえず、「既存と同じです」なら「ま、いっか」となるんですけど、なんで？のもっともらしい理由づけが欲しいです。

AppVeyorBot · 2019-10-06T12:30:32Z

✅ Build sakura 1.0.2296 completed (commit d28c692b9d by @beru)

berryzplus

とくに問題ないと思うので review を更新します。

beru · 2019-10-06T14:39:51Z

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

ルーラー描画の高速化

ルーラー描画の高速化

410e842

beru added the 🚅 speed up 🚀 高速化 label Oct 6, 2019

beru commented Oct 6, 2019

View reviewed changes

berryzplus reviewed Oct 6, 2019

View reviewed changes

berryzplus mentioned this pull request Oct 6, 2019

ノート線描画を少し分かりやすくする #1066

Merged

レビュー指摘対応

10e8913

berryzplus reviewed Oct 6, 2019

View reviewed changes

レビュー指摘対応

1a8fb17

berryzplus approved these changes Oct 6, 2019

View reviewed changes

beru merged commit 0af0286 into sakura-editor:master Oct 6, 2019

beru deleted the DrawRulerBg branch October 6, 2019 14:40

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

Merge pull request sakura-editor#1067 from beru/DrawRulerBg

465ba37

ルーラー描画の高速化

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ルーラー描画の高速化 #1067

ルーラー描画の高速化 #1067

beru commented Oct 6, 2019

AppVeyorBot commented Oct 6, 2019

berryzplus commented Oct 6, 2019

beru Oct 6, 2019

berryzplus Oct 6, 2019

berryzplus left a comment

berryzplus Oct 6, 2019

beru Oct 6, 2019

berryzplus Oct 6, 2019

beru Oct 6, 2019

berryzplus Oct 6, 2019

beru Oct 6, 2019

berryzplus Oct 6, 2019

beru Oct 6, 2019 •

edited

Loading

berryzplus Oct 6, 2019

beru Oct 6, 2019

beru Oct 6, 2019 •

edited

Loading

beru Oct 6, 2019

berryzplus Oct 6, 2019

berryzplus Oct 6, 2019

beru Oct 6, 2019

berryzplus Oct 6, 2019

beru Oct 6, 2019

berryzplus Oct 6, 2019

beru Oct 6, 2019

berryzplus Oct 6, 2019

beru Oct 6, 2019 •

edited

Loading

AppVeyorBot commented Oct 6, 2019

berryzplus left a comment

berryzplus Oct 6, 2019

berryzplus Oct 6, 2019

AppVeyorBot commented Oct 6, 2019

berryzplus left a comment

beru commented Oct 6, 2019

ルーラー描画の高速化 #1067

ルーラー描画の高速化 #1067

Conversation

beru commented Oct 6, 2019

PR の目的

カテゴリ

PR の背景

PR のメリット

PR のデメリット (トレードオフとかあれば)

PR の影響範囲

AppVeyorBot commented Oct 6, 2019

berryzplus commented Oct 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

berryzplus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beru Oct 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beru Oct 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

A

B

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beru Oct 6, 2019 • edited Loading

Choose a reason for hiding this comment

AppVeyorBot commented Oct 6, 2019

berryzplus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AppVeyorBot commented Oct 6, 2019

berryzplus left a comment

Choose a reason for hiding this comment

beru commented Oct 6, 2019

beru Oct 6, 2019 •

edited

Loading

beru Oct 6, 2019 •

edited

Loading

beru Oct 6, 2019 •

edited

Loading