メモリプールを使ってファイル読み込み時のメモリ確保を高速化 #312

beru · 2018-07-28T18:16:10Z

下記のメモリプールライブラリを使ってファイル読み込みを高速化しました。
https://github.com/cacay/MemoryPool

a\r\n という行が 16777216行連続している 48MB のファイルを読み込む時間が 7.5 秒から 5.8 秒になりました。

beru · 2018-07-28T18:18:08Z

テストに使ったファイル : test.zip

m-tmatma · 2018-07-28T21:58:26Z

PR でライセンスに関する言及がないですね。

beru · 2018-07-28T23:05:34Z

MITライセンスだそうです。
https://github.com/cacay/MemoryPool#license

kobake · 2018-08-09T21:30:03Z

外部ライブラリの置き場はそれがサードパーティであることが分かる場所にしたいです。
sakura_core/mem 配下は現状サクラエディタオリジナルのソースコードの置き場になっているので、それ以外のフォルダを作るのが好ましいと思いますがどうでしょう。

sakura_core 自体をサクラエディタオリジナルコードを収める場所と捉えて（現状そうでないものも混じっているのですが、それは今後整理したい）、プロジェクト直下に lib みたいなフォルダを作ってそこに外部ライブラリ系はファイルコピーまたは submodule 参照で入れるのが良いのではないかな、というのが自分のアイデアです。

あと、細かいところですが MemoryPool.h が memorypool.h という名前でコミットされているのは意図的ですか？

また、MemoryPool.tcc はこの PR では使われてないですね。使われていないファイルも含めてコピーするか否かの判断についてはどう考えましょうか？＞ALL

個人的にはケースバイケースだと思いますが今回のようなケースでは（ファイルコピーの策をとる場合には）使うファイルだけコピー、およびライセンスファイルがあればそれも含める、が良いと思います。

beru · 2018-08-11T17:01:14Z

@kobake レビューありがとうございます。

外部ライブラリの置き場はそれがサードパーティであることが分かる場所にしたいです。
sakura_core/mem 配下は現状サクラエディタオリジナルのソースコードの置き場になっているので、それ以外のフォルダを作るのが好ましいと思いますがどうでしょう。

そうですね。当初 sakura_core/extmodule が良いのかなとも思いましたが sakura_core/mem の方が目的に一致していて良いかなと思ってそこに置きました。

外部ライブラリの置き場をどこか決めてもらえればそこから #include するように変更します。

あと、細かいところですが MemoryPool.h が memorypool.h という名前でコミットされているのは意図的ですか？

意図はしていなかった筈です。何故か小文字に変ってしまってますね…。

また、MemoryPool.tcc はこの PR では使われてないですね。使われていないファイルも含めてコピーするか否かの判断についてはどう考えましょうか？＞ALL

MemoryPool.tcc は memorypool.h の 97行目で #include されています。

個人的にはケースバイケースだと思いますが今回のようなケースでは（ファイルコピーの策をとる場合には）使うファイルだけコピー、およびライセンスファイルがあればそれも含める、が良いと思います。

それで良いと思います。なお取得元のリポジトリにライセンスファイルは無いです。

sakura-editor/management-forum#20 でまだ結論が出ていませんが、外部ライブラリを使わない方針になったら同等のメモリプール処理を別途記述するようにします。

kobake · 2018-08-12T03:36:41Z

MemoryPool.tcc は memorypool.h の 97行目で #include されています。

あ、ほんとですね。把握です。失礼しました。

外部ライブラリの置き場をどこか決めてもらえればそこから #include するように変更します。

個人的には lib がシンプルで良いかなと思いますが、extmodule でも何でもお好みがあれば何でも良いです。名前決めは早い者勝ちでいいんじゃないですかね。必要であれば後から変更もできますし。

だいたい以下のようなイメージを抱いています。

sakura/
  sakura_core/
  lib/
    MemoryPool/
      MemoryPool.h
      MemoryPool.tcc
      README.md … なんとなくこれも置いておくと親切かも。

なお取得元のリポジトリにライセンスファイルは無いです。

みたいですね……。まぁ今回は置くとしたら README あたりで。

sakura-editor/management-forum#20

これの結論は待っててもしゃあないので待たずに進めちゃっていいです。
方針決まったら方針に会わせて後から対応しましょう。（その手戻りが嫌ってことでしたら「待ち」もありです）

kobake · 2018-08-12T03:48:00Z

ちなみに自分は VC++ プロジェクトで外部ライブラリ利用するときは最近は NuGet 経由で取り込むことが多いのですが、NuGet パッケージが提供されていないライブラリについては自分のほうで勝手に代理パッケージングしちゃったりしてます。

https://www.nuget.org/profiles/kobake

MemoryPool についても将来的には NuGet パッケージングしちゃおうかな～と思っていたり。（今後気が向いたときにでも）

berryzplus · 2018-08-12T03:50:44Z

取り込む方向で話が進んでいるようでｗ

https://github.com/cacay/MemoryPool

これgithubで公開されているものだからサブモジュール化できますけど、
サブモジュールにしなくていいですか？

kobake · 2018-08-12T04:00:32Z

まぁ、取り込みについてはまだ結論出て無くてグレーってことで目をつぶってくださいな ;)

たぶん一番楽なのは、

まずはコピーで対応しちゃう（最もハマりにくい）
動作問題なければ一旦マージしちゃう
その後、もっとクールな取り込み方法を検討して別件 PR で対応する

っていう流れがやりやすいかと。一発の PR で一気に理想にもっていこうとすると難航するので段階を踏むのが良いです。

berryzplus

外部ライブラリ取り込みOKで進める感じなので内容をチェックしました。
全体で問題ないと思います。
このあとフォルダ構成の変更が入ると思うので、変更箇所周辺で気になった点にコメントします。

berryzplus · 2018-08-12T04:02:43Z

sakura_core/doc/logic/CDocLineMgr.h

@@ -90,6 +91,7 @@ class CDocLineMgr{
 	CDocLine*	m_pDocLineTop;		//!< 最初の行
 	CDocLine*	m_pDocLineBot;		//!< 最後の行(※1行しかない場合はm_pDocLineTopと等しくなる)
 	CLogicInt	m_nLines;			//!< 全行数
+	MemoryPool<CDocLine> m_memoryPool;

 public:
 	//$$ kobake注: 以下、絶対に切り離したい（最低切り離せなくても、変数の意味をコメントで明確に記すべき）変数群


昔から気になっていたんですがこういうコメントどうします？ To: @kobake さん
「優先度を付けて対応」だと仕事っぽくてイヤですよね・・・
完全に別件なんですがどなたか issue 立てていただけると嬉しいです。

んまぁなんでもかんでも Issue にする必要はないかなぁ、と。
気が向いた人（自分も含む）が気づいたときに気が向いたときに対応、で良いんじゃないですかね。美学の話まで Issue に持ち込むと Issue 多くなりすぎて疲れそうです。

berryzplus · 2018-08-12T04:08:09Z

sakura_core/doc/layout/CLayoutMgr.cpp

@@ -612,7 +612,7 @@ CLayout* CLayoutMgr::DeleteLayoutAsLogical(
 			DEBUG_TRACE( _T("バグバグ\n") );


おそらく assert_warning(false && _T("バグバグ\n")); を意図するコードと思われます。
こういうのは気付いた時点で対応を検討していくのがベターだと思うんですがどうでしょう？
これまた完全に別件なのでどなたか issue を立てていただけると嬉しいです。

僕の場合だと「気づいた時点で気が向いたら Issue すっ飛ばして対応しちゃって PR 飛ばし」ますね。

では今回は気が向かないのでスルーでｗ

「気が向いた人がやる」、それがOSSの文化！

kobake · 2018-08-12T04:49:47Z

当初 sakura_core/extmodule が良いのかなとも思いましたが sakura_core/mem の方が目的に一致していて良いかなと思ってそこに置きました。

あ、sakura_core/extmodule って既存ディレクトリだったんですね。今気づきました。そっちでも良いです。

ただ、既存コードの置かれ方が extmodule 直下への cpp, h の直置きになってますが、README や LICENSE 配置の都合を考えると extmodule (に置くとしたら) 配下にサブフォルダ作って、そこに諸々置くのが好ましいと思います。

m-tmatma · 2018-08-13T03:44:19Z

これgithubで公開されているものだからサブモジュール化できますけど、
サブモジュールにしなくていいですか？

本家で更新かあったときの追従が楽、および外部ソースというのが明確になるという点で
考えると submodule にするといいと思います。

beru · 2018-08-13T06:50:04Z

これgithubで公開されているものだからサブモジュール化できますけど、
サブモジュールにしなくていいですか？

本家で更新かあったときの追従が楽、および外部ソースというのが明確になるという点で
考えると submodule にするといいと思います。

https://github.com/cacay/MemoryPool#this-repository-is-no-longer-maintained

という事なので submodule にしないで良いと思います。

https://github.com/cacay/MemoryPool

beru · 2018-09-17T09:12:40Z

このPRは風化しつつあるので Close します。

berryzplus mentioned this pull request Jul 29, 2018

外部プロジェクトのソースコード取り込みについて考える sakura-editor/management-forum#20

Open

beru changed the title ~~メモリプールを使ってファイル読み込み時のメモリ確保を高速化~~ [WIP] メモリプールを使ってファイル読み込み時のメモリ確保を高速化 Aug 10, 2018

berryzplus reviewed Aug 12, 2018

View reviewed changes

beru added 2 commits August 16, 2018 08:39

メモリプールを使ってファイル読み込み時のメモリ確保を高速化

0fa1afa

https://github.com/cacay/MemoryPool

レビュー指摘点対応

9b451d8

beru changed the title ~~[WIP] メモリプールを使ってファイル読み込み時のメモリ確保を高速化~~ メモリプールを使ってファイル読み込み時のメモリ確保を高速化 Aug 15, 2018

KENCHjp added the 🚅 speed up 🚀 高速化 label Sep 2, 2018

beru closed this Sep 17, 2018

ds14050 added the 🚅 speed up 🚀 高速化 label Sep 18, 2018

beru mentioned this pull request Aug 3, 2019

行データ管理クラスでメモリプールを使う事でメモリ確保と解放処理を高速化 #980

Closed

beru mentioned this pull request Aug 10, 2019

メモリプールを使う事でメモリ確保と解放を高速化 #985

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

メモリプールを使ってファイル読み込み時のメモリ確保を高速化 #312

メモリプールを使ってファイル読み込み時のメモリ確保を高速化 #312

beru commented Jul 28, 2018

beru commented Jul 28, 2018

m-tmatma commented Jul 28, 2018

beru commented Jul 28, 2018

kobake commented Aug 9, 2018

beru commented Aug 11, 2018

kobake commented Aug 12, 2018

kobake commented Aug 12, 2018

berryzplus commented Aug 12, 2018

kobake commented Aug 12, 2018

berryzplus left a comment

berryzplus Aug 12, 2018

kobake Aug 12, 2018 •

edited

Loading

berryzplus Aug 12, 2018

kobake Aug 12, 2018

berryzplus Aug 12, 2018

kobake Aug 12, 2018

kobake commented Aug 12, 2018

m-tmatma commented Aug 13, 2018

beru commented Aug 13, 2018

beru commented Sep 17, 2018

		@@ -612,7 +612,7 @@ CLayout* CLayoutMgr::DeleteLayoutAsLogical(
		DEBUG_TRACE( _T("バグバグ\n") );

メモリプールを使ってファイル読み込み時のメモリ確保を高速化 #312

メモリプールを使ってファイル読み込み時のメモリ確保を高速化 #312

Conversation

beru commented Jul 28, 2018

beru commented Jul 28, 2018

m-tmatma commented Jul 28, 2018

beru commented Jul 28, 2018

kobake commented Aug 9, 2018

beru commented Aug 11, 2018

kobake commented Aug 12, 2018

kobake commented Aug 12, 2018

berryzplus commented Aug 12, 2018

kobake commented Aug 12, 2018

berryzplus left a comment

Choose a reason for hiding this comment

berryzplus Aug 12, 2018

Choose a reason for hiding this comment

kobake Aug 12, 2018 • edited Loading

Choose a reason for hiding this comment

berryzplus Aug 12, 2018

Choose a reason for hiding this comment

kobake Aug 12, 2018

Choose a reason for hiding this comment

berryzplus Aug 12, 2018

Choose a reason for hiding this comment

kobake Aug 12, 2018

Choose a reason for hiding this comment

kobake commented Aug 12, 2018

m-tmatma commented Aug 13, 2018

beru commented Aug 13, 2018

beru commented Sep 17, 2018

kobake Aug 12, 2018 •

edited

Loading