SetScrollInfo の呼び出しを抑制 #453

beru · 2018-09-16T19:04:52Z

CEditView::AdjustScrollBars で呼び出している SetScrollInfo Windows API 関数の負荷が高いようなので、必要な場合にのみ呼び出しを行うように判定を追加しました。

m-tmatma · 2018-09-18T12:01:13Z

sakura_core/view/CEditView_Scroll.cpp

@@ -335,8 +339,12 @@ void CEditView::AdjustScrollBars()
 		si.nMax  = (Int)GetRightEdgeForScrollBar() - 1;		// 2009.08.28 nasukoji	スクロールバー制御用の右端座標を取得
 		si.nPage = (Int)GetTextArea().m_nViewColNum;			/* 表示域の桁数 */
 		si.nPos  = (Int)GetTextArea().GetViewLeftCol();		/* 表示域の一番左の桁(0開始) */
-		si.nTrackPos = 1;
-		::SetScrollInfo( m_hwndHScrollBar, SB_CTL, &si, TRUE );
+		si.nTrackPos = 0;


1 → 0 に変わっていますが、意図してますか?

意図したものです。tagSCROLLINFO の nTrackPos メンバーは、SetScrollInfo 関数では無視されます。そして、GetScrollInfo で取得した値と比較する為、このメンバーの値をあえて 0 に揃えています。

m-tmatma · 2018-09-18T12:14:15Z

sakura_core/view/CEditView_Scroll.cpp

-		::SetScrollInfo( m_hwndVScrollBar, SB_CTL, &si, TRUE );
+		SCROLLINFO siPrev = si;
+		::GetScrollInfo( m_hwndVScrollBar, SB_CTL, &siPrev );
+		siPrev.nTrackPos = 0;


GetScrollInfo した結果で siPrev.nTrackPos が 0 以外でそれ以外が
一致していた場合、元のコードと振る舞いが異なりますが、
意図していますか?

nTrackPos メンバーは取得のみで設定は出来ないので問題ないという認識です。
実際に操作して問題は無いように見えます。色んなケースを本当に確認できているのか？
という疑問は常に付きまといますが。

下のコメントに書きましたが、memcmp を使おうとするからややこしくなるのだと思います。

そう言われると記述量が多くなってもメンバー単位で比較した方が良いのかもしれないですね。

m-tmatma · 2018-09-18T12:16:19Z

sakura_core/view/CEditView_Scroll.cpp

+		SCROLLINFO siPrev = si;
+		::GetScrollInfo( m_hwndVScrollBar, SB_CTL, &siPrev );
+		siPrev.nTrackPos = 0;
+		if (memcmp(&si, &siPrev, sizeof(si)) != 0)


siPrev 全体と si を比較せずに、si.nPage や si.nPos など異なる可能性のあるメンバーを選んで比較した方がいいと思います。

c言語は構造体メンバのビット数をある程度任意に決められるので、慣習的にこういう比較方法をとることが多いように思います。たとえば0～7の数値は3bitあれば足りるので、構造体の変数定義をint型にして3bit割り付ける、というような細かい調整ができます。メモリサイズの制約が厳しかった時代の遺産なので、現代では考慮する必要のないことなのかも知らんです。

どっちかというとこれが標準なので、必要なメンバだけ比較って方式に変えるのは大変かも知れないです。ぶっちゃけると、ここで細かい調整をしても、あとで大枠ごと入れ替える可能性があるので、現段階ではあまりこだわらないのが吉であるように感じております。

ビットフィールドが今回のケースにどう関わってくるのか良く分かりませんが、 memcmp で比較するコードにしたのはメンバ単位での比較を記述するのが手間に感じたからです。しかしこうして色々やり取りが発生する事を考えると memcmp さんは存在自体が罪深いなとしみじみと感じます…。

構造体のフィールド間にパディングが生じる場合に、memcmp での比較はパディングまで比較対象にしてしまいます。パディングの値は基本的に不定です。

構造体のフィールドに割り当てるビット数の指定やアラインメントの指定はパディングの制御を可能にします。しかし SCROLLINFO を定義しているのは……。

memcmp で比較する場合は、両方のオペランドが memset により 0 埋め初期化されていることが必須ではないでしょうか。確かめたことがありませんが、不安になる＆前提の確認が面倒なので、memcmp を比較には使わないようにしています。

色々と解説ありがとうございます。

今回のケースで memcmp で実際に動作上問題があるのかどうかは各人のコメントを見ても良く分かりませんでしたが、横着せずにメンバーを選んで比較するように最初から書いた方が良かったですね。

願わくば、絡まれるのを避けるためだけの事なかれ主義によるものではなく、C 時代の化石じみた問題含みの手法であることを理解した結果であることを。(別の場所でうんざりさせてしまった自覚があるので書いています)

突き合わせしたいフィールド外の箇所の値に影響されてしまうというのは本当にそうですね。
それへの対処を別に入れるくらいなら使わない方が良い、というのもおっしゃる通りだと思います。
今回は楽できるかなという思い込みで書きましたが、色々と駄目でした。

m-tmatma · 2018-09-18T12:59:58Z

水平方向と垂直方向の処理で、コードの重複を避けるため関数化できませんか?

berryzplus · 2018-09-18T14:07:12Z

水平方向と垂直方向の処理で、コードの重複を避けるため関数化できませんか?

正論。

しかし難しかったようなうろ覚えの記憶があります。

テンプレじゃ無理なんで「やるならマクロ関数化？」みたいな修羅道だった気が。

m-tmatma · 2018-09-18T14:12:35Z

sakura/sakura_core/view/CEditView_Scroll.cpp

Lines 308 to 319 in 58d8bd9

    
           si.cbSize = sizeof( si ); 
        
           si.fMask = SIF_ALL | SIF_DISABLENOSCROLL; 
        
           si.nMin  = 0; 
        
           si.nMax  = (Int)nAllLines / nVScrollRate - 1;	/* 全行数 */ 
        
           si.nPage = (Int)GetTextArea().m_nViewRowNum / nVScrollRate;	/* 表示域の行数 */ 
        
           si.nPos  = (Int)GetTextArea().GetViewTopLine() / nVScrollRate;	/* 表示域の一番上の行(0開始) */ 
        
           si.nTrackPos = 0; 
        
           SCROLLINFO siPrev = si; 
        
           ::GetScrollInfo( m_hwndVScrollBar, SB_CTL, &siPrev ); 
        
           siPrev.nTrackPos = 0; 
        
           if (memcmp(&si, &siPrev, sizeof(si)) != 0) 
        
           	::SetScrollInfo( m_hwndVScrollBar, SB_CTL, &si, TRUE );

で si.nMax, si.nPage, si.nPos, m_hwndVScrollBar の値を引数に渡すような関数を作って実現できないですか?

sakura/sakura_core/view/CEditView_Scroll.cpp

Lines 311 to 313 in 58d8bd9

    
           si.nMax  = (Int)nAllLines / nVScrollRate - 1;	/* 全行数 */ 
        
           si.nPage = (Int)GetTextArea().m_nViewRowNum / nVScrollRate;	/* 表示域の行数 */ 
        
           si.nPos  = (Int)GetTextArea().GetViewTopLine() / nVScrollRate;	/* 表示域の一番上の行(0開始) */

sakura/sakura_core/view/CEditView_Scroll.cpp

Line 316 in 58d8bd9

::GetScrollInfo( m_hwndVScrollBar, SB_CTL, &siPrev );

m-tmatma · 2018-09-19T10:59:48Z

sakura_core/view/CEditView_Scroll.cpp

+		|| siPrev.nPos != nPos)
+	{
+		if (nMax + 1 < (int)nPage) {
+			nPage = (UINT)(nMax + 1);


max に +1 しているのはなぜですか?

動作確認した結果入れました。

ユーザー定義関数の引数 nPage に来る値が、nMax より結構大きかったりするケースが状況によってはあるのですが、その値をそのまま構造体に入れて SetScrollInfo で設定しても、GetScrollInfo で取った値ではクリッピングされているので、比較する時に一致しません。今回の変更の目的は呼び出しコストの大きい SetScrollInfo をなるべく呼ばなくする事なので対処を入れています。

このコメントを書く際に公式のドキュメントを再度調べてみたら書かれてました。

https://docs.microsoft.com/en-ca/windows/desktop/api/winuser/nf-winuser-setscrollinfo#remarks

The SetScrollInfo function performs range checking on the values specified by the nPage and nPos members of the SCROLLINFO structure. The nPage member must specify a value from 0 to nMax - nMin +1. The nPos member must specify a value between nMin and nMax - max( nPage– 1, 0). If either value is beyond its range, the function sets it to a value that is just within the range.

sakura/sakura_core/view/CEditView_Scroll.cpp

Lines 289 to 291 in b178891

if (nMax + 1 < (int)nPage) {

nPage = (UINT)(nMax + 1);

}

は以下のような感じにしたらいかがでしょうか?

// nPage を 0 から nMax - nMin + 1 の間に収まるようにする // https://docs.microsoft.com/en-ca/windows/desktop/api/winuser/nf-winuser-setscrollinfo#remarks const int nMin = 0; if ((nMax - nMin)+ 1 < (int)nPage) { nPage = (UINT)((nMax - nMin)+ 1); }

↑ (nMax - nMin) の括弧は不要でした。

そのように直しておきます。

元のコードからある問題なのですが、nMax に負の値を指定された場合に UINT へのキャストでとても大きい値になってしまうのがちょっと心配ですね。

あと nPos はクリッピングしなくて良いのか、とかも疑問があります。

まぁとりあえず動いているから良いかな。。

m-tmatma · 2018-09-19T11:01:48Z

この PR の効果を調べるにはどうやってテストしたらわかりますか?

beru · 2018-09-19T17:39:52Z

この PR の効果を調べるにはどうやってテストしたらわかりますか?

下記の手順で自分は確認しました。

Visual Studio で Release ビルドします。
Menu の Debug の Performance Profiler を選んだ後に Start ボタンを押して起動します。
起動後に何かファイルを開きます。例えば sakura_core\CBackupAgent.cpp
開いた後にスクロールバーをマウスのドラッグ操作で上下に往復して動かします。出来るだけ一定のペースで10往復程度。
終了するとプロファイルのレポートが表示されるので、CEditView::AdjustScrollBars のCPU使用率を確認します。

master の場合と比べてこのPRのビルドで数値が大きく減少する事が確認出来れば効果が有りとみなせると思います。

m-tmatma · 2018-09-19T22:00:09Z

下記の手順で自分は確認しました。

ありがとうございます。今夜 or 明日動作確認してみようと思います。

m-tmatma · 2018-09-20T12:46:57Z

14.76 % → 9.4 % になりました。

master (f411bb3)

この PR (494e1cf)

m-tmatma · 2018-09-20T13:00:42Z

以下のようなコードで setScrollInfoIfNeeded が呼ばれた回数に対して SetScrollInfo が呼ばれた回数を調べてみました。→ g_total = 970, g_called = 470 でした。SetScrollInfo 呼び出しを半分程度にできるようです。

diff --git a/sakura_core/dlg/CDlgAbout.cpp b/sakura_core/dlg/CDlgAbout.cpp
index f68b289f..c77afac1 100644
--- a/sakura_core/dlg/CDlgAbout.cpp
+++ b/sakura_core/dlg/CDlgAbout.cpp
@@ -138,6 +138,9 @@ int CDlgAbout::DoModal( HINSTANCE hInstance, HWND hwndParent )
 	return (int)CDialog::DoModal( hInstance, hwndParent, IDD_ABOUT, (LPARAM)NULL );
 }
 
+extern int g_called;
+extern int g_total;
+
 /*! 初期化処理
 	@date 2008.05.05 novice GetModuleHandle(NULL)→NULLに変更
 	@date 2011.04.10 nasukoji	各国語メッセージリソース対応
@@ -205,6 +208,8 @@ BOOL CDlgAbout::OnInitDialog( HWND hwndDlg, WPARAM wParam, LPARAM lParam )
 	// 段落区切り
 	cmemMsg.AppendString( _T("\r\n") );
 
+	cmemMsg.AppendStringF(_T("call %d/%d\r\n"), g_called, g_total);
+
 	// コンパイル情報
 	cmemMsg.AppendString( _T("      Compile Info: ") );
 	cmemMsg.AppendStringF(
diff --git a/sakura_core/view/CEditView_Scroll.cpp b/sakura_core/view/CEditView_Scroll.cpp
index dcefb80f..a65a1196 100644
--- a/sakura_core/view/CEditView_Scroll.cpp
+++ b/sakura_core/view/CEditView_Scroll.cpp
@@ -30,6 +30,9 @@
 #include "types/CTypeSupport.h"
 #include <limits.h>
 
+int g_total = 0;
+int g_called = 0;
+
 /*! スクロールバー作成
 	@date 2006.12.19 ryoji 新規作成（CEditView::Createから分離）
 */
@@ -273,6 +276,7 @@ CLayoutInt CEditView::OnHScroll( int nScrollCode, int nPos )
 	return nScrollVal;
 }
 
+
 static void setScrollInfoIfNeeded(HWND hWndScrollBar, int nMax, UINT nPage, int nPos)
 {
 	SCROLLINFO siPrev;
@@ -294,6 +298,7 @@ static void setScrollInfoIfNeeded(HWND hWndScrollBar, int nMax, UINT nPage, int
 	if (nPos > nMax) {
 		nPos = nMax;
 	}
+	g_total++;
 	if (siPrev.nMin != 0
 		|| siPrev.nMax != nMax
 		|| siPrev.nPage != nPage
@@ -307,6 +312,7 @@ static void setScrollInfoIfNeeded(HWND hWndScrollBar, int nMax, UINT nPage, int
 		si.nPage = nPage;
 		si.nPos  = nPos;
 		::SetScrollInfo( hWndScrollBar, SB_CTL, &si, TRUE );
+		g_called++;
 	}
 }

m-tmatma

ありがとうございます。

beru · 2018-09-20T13:45:26Z

レビューありがとうございます。Merge します。
もし問題が見つかった場合には別 PR を作成して対処する事にしましょう。

SetScrollInfo の呼び出しを抑制

beru added the 🚅 speed up 🚀 高速化 label Sep 17, 2018

m-tmatma reviewed Sep 18, 2018

View reviewed changes

m-tmatma reviewed Sep 19, 2018

View reviewed changes

beru added 3 commits September 20, 2018 02:18

SetScrollInfo の呼び出しを抑制

2bfcc04

レビュー指摘対応

f24ebf2

改行コード修正

b178891

レビュー指摘対応

494e1cf

m-tmatma approved these changes Sep 20, 2018

View reviewed changes

beru merged commit 498b921 into sakura-editor:master Sep 20, 2018

beru deleted the AdjustScrollBars branch September 20, 2018 13:45

m-tmatma added this to the next release milestone Oct 21, 2018

HoppingTappy pushed a commit to HoppingTappy/sakura that referenced this pull request Jun 11, 2019

Merge pull request sakura-editor#453 from beru/AdjustScrollBars

d4d735e

SetScrollInfo の呼び出しを抑制

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SetScrollInfo の呼び出しを抑制 #453

SetScrollInfo の呼び出しを抑制 #453

beru commented Sep 16, 2018

m-tmatma Sep 18, 2018

beru Sep 18, 2018

m-tmatma Sep 18, 2018

beru Sep 18, 2018

m-tmatma Sep 18, 2018

beru Sep 18, 2018

m-tmatma Sep 18, 2018

berryzplus Sep 18, 2018

beru Sep 18, 2018

ds14050 Sep 19, 2018 •

edited

Loading

beru Sep 19, 2018

ds14050 Sep 19, 2018 •

edited

Loading

beru Sep 19, 2018

m-tmatma commented Sep 18, 2018

berryzplus commented Sep 18, 2018

m-tmatma commented Sep 18, 2018

m-tmatma Sep 19, 2018

beru Sep 19, 2018 •

edited

Loading

m-tmatma Sep 19, 2018

m-tmatma Sep 19, 2018

beru Sep 20, 2018

m-tmatma commented Sep 19, 2018

beru commented Sep 19, 2018

m-tmatma commented Sep 19, 2018

m-tmatma commented Sep 20, 2018

m-tmatma commented Sep 20, 2018

m-tmatma left a comment

beru commented Sep 20, 2018

SetScrollInfo の呼び出しを抑制 #453

SetScrollInfo の呼び出しを抑制 #453

Conversation

beru commented Sep 16, 2018

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

Choose a reason for hiding this comment

ds14050 Sep 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ds14050 Sep 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m-tmatma commented Sep 18, 2018

berryzplus commented Sep 18, 2018

m-tmatma commented Sep 18, 2018

Choose a reason for hiding this comment

beru Sep 19, 2018 • 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

m-tmatma commented Sep 19, 2018

beru commented Sep 19, 2018

m-tmatma commented Sep 19, 2018

m-tmatma commented Sep 20, 2018

m-tmatma commented Sep 20, 2018

m-tmatma left a comment

Choose a reason for hiding this comment

beru commented Sep 20, 2018

ds14050 Sep 19, 2018 •

edited

Loading

ds14050 Sep 19, 2018 •

edited

Loading

beru Sep 19, 2018 •

edited

Loading