ファイル読み込み高速化(行データ読み出しのマルチスレッド対応) #2021

suconbu · 2025-03-15T15:32:35Z

PR対象

アプリ(サクラエディタ本体)

カテゴリ

改善

PR の背景

#1988 の第3弾対応です。

今回の変更対象は CReadManager::ReadFile_To_CDocLineMgr() で、この関数では主に以下の処理をしています。

ファイルからテキストデータを読み込む
それを行単位に分割
それを内部文字エンコーディング (UTF-16) へ変換 (CIoBridge::FileToImpl の実行)

これらの処理を yoshinrt/SakuraVz の 1578dc68, e9cbf1b0 の対応を参考に高速化します。

仕様・動作説明

対応内容：

CFileLoad のメモリマップドファイル対応
従来は固定長のバッファに対して逐次ファイル内容を読み込みながら処理をしていましたが、
メモリコピーの負荷を減らすことと、後述のマルチスレッド対応の効果を高める (＝マルチスレッド化ができない処理を減らす) ことを目的に、メモリマップドファイルを使ってデータを読み込むよう変更します。
CReadManager::ReadFile_To_CDocLineMgr のマルチスレッド対応
テキストデータを行単位へ分割する処理と文字コード変換の処理を、複数のスレッドで並列実行するようにします。
サイズが2GiB以上のファイルを開いた時に落ちる問題を修正 (参考元の対応にはない修正)
「2」の対応したことで起きるようになったため本PRに含めて対応します。
CEol::SetTypeByStringForFile() の引数 nDataLen がオーバーフローしていることが分かったため、型を int から size_t に変更します。(関連する変数/引数の型もあわせて直します)

各区間の処理時間 (計測条件などは #1988 を参照)：

区間	対応前 (`9f9948d`)	本PR対応後
A. CLoadAgent::OnLoad 開始から ReadFile_To_CDocLineMgr 完了まで	1458.327	502.304
B. そこから CLoadAgent::OnLoad 完了まで (レイアウトデータ化処理)	676.073	647.408
C. CLoadAgent::OnAfterLoad 開始から完了まで	16.088	89.502
合計	2150.488	1239.214 (-42%)

PR の影響範囲

メモリマップドファイルを使うようにした影響で、ファイル読み込み中の瞬間最大のメモリ使用量が増えます。
読み込んだファイルとほぼ同じサイズ分だけ増加します。

(例) 1GiB のファイルを読み込む時の最大メモリ使用量：

変更前 (`9f9948d`)	本PR対応後
2.4GB	3.3GB (+0.9GB)

テスト内容

test.zip の「test_1000lines.txt」(1000行のテキストファイル) を開き、各行の数字が行番号と一致していること。👈ファイル読み込み処理後のCDocLineMgrの集約が正しい順序でできていることの確認
test_4gb.zip の「test_4gb.txt」(4GBのテキストファイル) を開いた時、エラーのダイアログボックスが表示されないこと。👈対応内容の「3」の修正が効いていることの確認

参考資料

SetTypeByStringForFileの引数nDataLenがオーバーフローしていたため。関連する処理を含めてintをsize_tに変更する。

suconbu · 2025-03-15T15:41:12Z

sakura_core/io/CFileLoad.h

@@ -105,11 +103,14 @@ class CFileLoad
 	/* メンバオブジェクト */
 	const SEncodingConfig* m_pEencoding;

+	//! 文字コード自動検出のために読み込む最大サイズ(byte)
+	static constexpr LONGLONG m_nAutoDetectReadLen = 32768LL;


挙動が変化しないよう旧 gm_nBufSizeDef と同値としました。

suconbu · 2025-03-15T15:43:11Z

sakura_core/io/CFileLoad.cpp

-	バッファにデータを読み込む
-	@note エラー時は throw する
-*/
-void CFileLoad::Buffering( void )


メモリマップドファイル化により逐次の読み込みが不要になったため廃止

sonarqubecloud · 2025-03-15T15:57:18Z

Quality Gate passed

Issues
19 New issues
0 Accepted issues

Measures
0 Security Hotspots
81.5% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

AppVeyorBot · 2025-03-15T16:17:18Z

✅ Build sakura 1.0.4461 completed (commit 0a57687cf0 by @suconbu)

berryzplus

対応ありがとうございます。基本方針賛同です。

berryzplus · 2025-03-16T03:38:59Z

sakura_core/CEol.cpp

+	{ "\x0d\x00",			"\x00\x0d",				2U },
+	{ "\x85\x00",			"\x00\x85",				2U },
+	{ "\x28\x20",			"\x20\x28",				2U },
+	{ "\x29\x20",			"\x20\x29",				2U },


指摘ではないですが、変更する必要の「ある／なし」で言うと「なし」なので微妙です。

エンディアン変換をするための構造体だと思いますが、もう少しスマートな方法でやれないものか、とか思います。

berryzplus · 2025-03-16T03:47:42Z

sakura_core/CEol.h

-	bool StartsWith(const WCHAR* pData, int nLen) const{ return m_nLen<=nLen && 0==wmemcmp(pData,m_szDataW,m_nLen); }
-	bool StartsWith(const ACHAR* pData, int nLen) const{ return m_nLen<=nLen && m_szDataA[0] != '\0' && 0==memcmp(pData,m_szDataA,m_nLen); }
+	bool StartsWith(const WCHAR* pData, size_t nLen) const{ return m_nLen<=nLen && 0==wmemcmp(pData,m_szDataW,m_nLen); }
+	bool StartsWith(const ACHAR* pData, size_t nLen) const{ return m_nLen<=nLen && m_szDataA[0] != '\0' && 0==memcmp(pData,m_szDataA,m_nLen); }


指摘ではないですが、 util/string_ex.h に似たような定義がたくさんあるので、そちらで集中管理してもいいんじゃないかと思いました。

berryzplus · 2025-03-16T03:49:13Z

sakura_core/CReadManager.cpp

@@ -33,6 +33,8 @@
 #include "util/window.h"
 #include "CSelectLang.h"
 #include "String_define.h"
+#include <atomic>
+#include <future>


指摘ではないですが、標準c++のヘッダーファイルは StdAfx.h でまとめてインクルードする感じにできませんかね。

たぶん趣味の領域なのでどちらでもいいです。

berryzplus · 2025-03-16T03:51:00Z

sakura_core/CReadManager.cpp

@@ -84,7 +86,7 @@ EConvertResult CReadManager::ReadFile_To_CDocLineMgr(
 	EConvertResult eRet = RESULT_COMPLETE;

 	try{
-		CFileLoad cfl(type->m_encoding);
+		CFileLoad cFileLoad( type->m_encoding );


何故変数名を変える？　のありがち指摘を入れときます。

理由があるなら教えてください。
理由のない変更は（以下省略。）

berryzplus · 2025-03-16T03:59:34Z

sakura_core/CReadManager.cpp

 		pFileInfo->SetBomExist( bBom );

 		/* ファイル時刻の取得 */
 		FILETIME	FileTime;
-		if( cfl.GetFileTime( NULL, NULL, &FileTime ) ){
+		if( cFileLoad.GetFileTime( NULL, NULL, &FileTime ) ){


影響が大きいので黙ってましたが、 NULL を nullptr で re-define する変更を revert したいと思っています。
理由は「 gdiplus.h をインクルードしたときエラーになるから」です。
個人的には gdiplus 嫌いなんですけど、ダークモード対応で TortoiseGit のコードの一部を流用しようとしたときに問題になるので、いつか対応入れると思います。

このあたり踏まえて、触る行に含まれる NULL は nullptr に置換しときたいです。

berryzplus · 2025-03-16T04:03:20Z

sakura_core/CReadManager.cpp

+		std::vector<std::future<EConvertResult>> vecWorkerFutures;
+		std::atomic<bool> bCanceled = false;
+
+		size_t nOffsetBegin = cFileLoad.GetNextLineOffset( (size_t)cFileLoad.GetFileSize() );


指摘じゃないですが (size_t) に違和感。
キャストが必要なのは縮小変換する場合だけのはず。
cFileLoad.GetFileSize() の関数名で、返すものがファイルサイズじゃないとすると問題。

berryzplus · 2025-03-16T04:07:09Z

sakura_core/doc/logic/CDocLineMgr.cpp

+//! 元のCDocLineMgrの内容は空になる
+void CDocLineMgr::AppendAsMove( CDocLineMgr& other )
+{
+	if( other.GetDocLineTop() == nullptr ){ return; }


指摘じゃないです。ほほ趣味ですが、こう書いても同じ意味。

Suggested change

if( other.GetDocLineTop() == nullptr ){ return; }

if (!other.GetDocLineTop()) return; // otherが空なら抜ける

berryzplus · 2025-03-16T04:08:52Z

sakura_core/io/CFileLoad.h

 //	LPWSTR	m_pszFileName;	// ファイル名
 	HANDLE	m_hFile;		// ファイルハンドル
+	HANDLE	m_hFileMapping;	// メモリマップドファイルハンドル


Suggested change

HANDLE m_hFileMapping; // メモリマップドファイルハンドル

HANDLE m_hFileMapping = nullptr; // メモリマップドファイルハンドル

berryzplus · 2025-03-16T04:12:39Z

sakura_core/io/CFileLoad.h

+	const char*	m_pReadBufTop;	// 読み込みバッファの先頭を指すポインタ
+	size_t m_nReadBufOffsetBegin;
+	size_t m_nReadBufOffsetEnd;
+	size_t m_nReadBufOffsetCurrent;	// 読み込みバッファ中のオフセット(次の行頭位置)


指摘です。最近のC++ではクラスメンバーの宣言時に初期値を書いておくやり方が主流なので、想定する初期値があればここに書いて欲しいです。

berryzplus · 2025-03-16T04:14:11Z

sakura_core/io/CFileLoad.h

@@ -154,7 +154,7 @@ inline BOOL CFileLoad::GetFileTime( FILETIME* pftCreate, FILETIME* pftLastAccess
 inline int CFileLoad::Read( void* pBuf, size_t nSize )
 {
 	DWORD ReadSize;
-	if( !::ReadFile( m_hFile, pBuf, nSize, &ReadSize, NULL ) )
+	if( !::ReadFile( m_hFile, pBuf, (DWORD)nSize, &ReadSize, NULL ) )


指摘じゃないです。

メモリマップトファイルに変更したのに ReadFile 使うん？
（ちゃんとソースコード読めよ、のセルフ突っ込みしておきます。）

suconbu added 3 commits March 15, 2025 17:36

ファイル読み込み高速化(CFileLoadのメモリマップドファイル対応)

5ff14e2

ファイル読み込み高速化(行データ読み出しのマルチスレッド対応)

289a4bd

2GiB以上のファイルを開く時にエラーが出ることがあるのを修正

a8bf87b

SetTypeByStringForFileの引数nDataLenがオーバーフローしていたため。関連する処理を含めてintをsize_tに変更する。

suconbu commented Mar 15, 2025

View reviewed changes

suconbu changed the title ~~Feature/speedup fileopen multithread open~~ ファイル読み込み高速化(行データ読み出しのマルチスレッド対応) Mar 15, 2025

berryzplus reviewed Mar 16, 2025

View reviewed changes

berryzplus mentioned this pull request Mar 16, 2025

半角文字が全角文字扱いされる + 全角文字が半角文字扱いされる #2022

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ファイル読み込み高速化(行データ読み出しのマルチスレッド対応) #2021

ファイル読み込み高速化(行データ読み出しのマルチスレッド対応) #2021

suconbu commented Mar 15, 2025 •

edited

Loading

suconbu Mar 15, 2025

suconbu Mar 15, 2025

sonarqubecloud bot commented Mar 15, 2025

AppVeyorBot commented Mar 15, 2025

berryzplus left a comment

berryzplus Mar 16, 2025

berryzplus Mar 16, 2025

berryzplus Mar 16, 2025

berryzplus Mar 16, 2025

berryzplus Mar 16, 2025

berryzplus Mar 16, 2025

berryzplus Mar 16, 2025

berryzplus Mar 16, 2025

berryzplus Mar 16, 2025

berryzplus Mar 16, 2025

	if( other.GetDocLineTop() == nullptr ){ return; }
	if (!other.GetDocLineTop()) return; // otherが空なら抜ける

	HANDLE m_hFileMapping; // メモリマップドファイルハンドル
	HANDLE m_hFileMapping = nullptr; // メモリマップドファイルハンドル

ファイル読み込み高速化(行データ読み出しのマルチスレッド対応) #2021

Are you sure you want to change the base?

ファイル読み込み高速化(行データ読み出しのマルチスレッド対応) #2021

Conversation

suconbu commented Mar 15, 2025 • edited Loading

PR対象

カテゴリ

PR の背景

仕様・動作説明

PR の影響範囲

テスト内容

関連 issue, PR

参考資料

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Mar 15, 2025

Quality Gate passed

AppVeyorBot commented Mar 15, 2025

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

suconbu commented Mar 15, 2025 •

edited

Loading