メモリプールを使う事でメモリ確保と解放を高速化 #985
Conversation
|
C++17 以降では 単体テストについては #980 ではリクエストが有ったので追加しましたが、結局のところ役に立っていないと判断してこのPRでは追加していません。レビューアーの方々の考えとして、「単体テストが実装されていないコードなんてちゃんと動く保証が無いので Approve は出来ない（むしろ Reject したい）、動作確認するのは面倒なのでやりたくない」という事であれば、なんかもう代わりに改良してくれよ、って感じです。 |
|
✅ Build sakura 1.0.2114 completed (commit f0f6203e88 by @) |
|
リトライありがとうございます:smile:
c++の準拠規格が17に変更されていて、CLayoutMgrでstd::pmr::unsynchronized_pool_resourceが使われているように見えました。使うの使わないのどっち？な感じなので、可能なら確認しといて欲しいです。もちろんぼくもあとで見とこうと思っていますが :smile: 未定義の場合の準拠規格はc++14相当なので、準拠規格の変更にあたると思います。何が違うのかメリデメを説明するのがめんどくさいので、これに関しては「ついでに入れちゃえ！」でもいいです。後追いでMinGW側のメイクファイルとかCMakelists.txtにも横展開することになるのでいずれにしろ変更レビューが入りますし。 |
同じ内容の PR を何度も作成して手間をかけてしまいすみません。 レビューアーの方々、特に @m-tmatma さんの労力を無駄にしてしまい本当に申し訳ないと思います。指摘については出来るだけ対応しますが、小改良に関して綿密にコメントをやり取りするのはコスト掛け過ぎな気もするのでもうちょっとレビューの基準を緩めてほしいです。
最新のコミットでは、
致命的な問題が無ければ C++17 に変更してしまって良いと思います。MinGW側の対応は後で別のPRにさせてもらえると楽で助かります。 |
There was a problem hiding this comment.
先に c++17 の導入提案をしましょう 😄
この PR で提案されているメモリ管理の手法は c++17 の導入された考え方を前提にしています。既に、azure pipelinesのMinGWビルドがコケていて（＝悪い影響がでている）、あまり良くない感じになっています。導入提案を単発で行うこと自体にもメリットがあると思うので、先に済ませておきたいです。
導入提案で「c++17でできること」を共有しておけば、メソッド引数の属性マーキングに「これはなんですか？mdに記載してください。」みたいなアフォな指摘が付く事態も防げる気がします。（実際にあるかどうかは別にして容易に想定できる事態です 😭
現状で細かい指摘を入れてもMinGWビルドがコケて微妙な感じになるのは明白なので、導入提案が通ってrebaseができるまで指摘は保留します。（しょーもない指摘は先行で書いてしまうかも知れません。
c++導入提案のPRですが、可能ならお願いしたいです。
出なかったとしてもc++17導入のハラは決まったので、盆休み中にはPRを出したい感じです。
たぶん、メリデメ書くには詳しい人のほうがいいと思うので。
（ぼくがレビュアーに回ると「評価がザル」っていうメリットもあるよ！...orz
PRを分ける事で実質的に何が得られるのか自分には良く分かりませんが分けておきました。#986
そもそもなんで導入提案で「c++17でできること」の解説をわざわざしないといけないんでしょうか？興味がある人なら自分で調べるだろうし、興味は無いけど良く分からないから反対だけするという人がいれば意味もなく保守的だなぁ…って思います。 ターゲットがPCのアプリの開発で開発環境で新しい言語規格を使えるなら気兼ねなく使っても問題無いと思うんですが…。業務の都合上、古いバージョンのVisualStudioを使う必要があるわけではないし。 今現在サクラエディタの開発をしてる人でこれこれこういう理由で使いたくないんだ、っていうのがあれば聞いてみたいところです。 |
デメリットがある（＝既存コードを書き換えないといけない）からです。
@beru さんのレベルなら「たくさんの変更」を１つのPRに含めても、どのファイルのどの修正が何を変更するものなのかを適切に判断できると思います。PRを分けることのメリットは、ある目的を果たすための変更が正しく行われているか否かの判断を容易に行えるようになることです。レビュー参加にあたって求められるレベルを下げることができます。2つ以上の目的を含むPRは、全部の目的が承認(or 容認)されないとマージできませんが、目的が1つならば1つの容認を取り付ければマージできます。目的を1つにすることにより、話の争点がブレにくくなると思います。 |
既存コードの書き替えについては VC++ だと書き換えなくてもビルドが通りましたが、MinGW だとコンパイルエラーが生じていました。 あと「c++17でできること」って色々あり過ぎませんか？調べればいくらでも詳しい情報が見つかるのに、PRの導入文で何でそんな事を書かなきゃいけないのかもうわけがわかりません。。まぁ話の流れの中で適当に書いた事なのかもしれませんが…。。
なぜ別けた方が良いのかについての丁寧な解説ありがとうございます。別けた方が明確になるのはその通りだと思います。でも別けないと何やってるのか分からないくらい複雑なPRでも無いと思いますし、細かい事はどうでもいいじゃないかって思うんですけど…。 |
std::pmr::unsynchronized_pool_resource だとメモリ使用量が大きい為、自前実装を用意
|
Azure Pipelines で mingw のビルドが失敗しています precompiled header のファイルをビルド前に消さないといけないとか書いてるページがありました。 そもそもなぜ mingw のビルドまで抱えないといけないのだろう… zzz |
|
✅ Build sakura 1.0.2127 completed (commit e8b5621d50 by @) |
|
/AzurePipelines run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
MinGWビルドの結果は一旦スルーでいいです。 |
|
MinGW もうなくせばいいのにーとか思うんですけどここに書く話じゃないですね。 |
|
README.md には MinGW の対応必須ですよとは書かれてませんが対応したい人の判断では他の人のPRでもちゃんと対応せにゃあかん的な事なのかと… きっと昔はついでに見ておくか的なものだったんでしょうけど… |
| : m_getIndentOffset( &CLayoutMgr::getIndentOffset_Normal ) // Oct. 1, 2002 genta // Nov. 16, 2002 メンバー関数ポインタにはクラス名が必要 | ||
| : m_getIndentOffset( &CLayoutMgr::getIndentOffset_Normal ), // Oct. 1, 2002 genta // Nov. 16, 2002 メンバー関数ポインタにはクラス名が必要 | ||
| m_layoutMemRes(new CPoolResource<CLayout>()) | ||
| //m_layoutMemRes(new std::pmr::unsynchronized_pool_resource()) // メモリ使用量が大きい為に使用しないで、CPoolResource を代わりに使う |
There was a problem hiding this comment.
追加のメンバ初期化宣言を「, m_layoutMemRes」のように書けば、今後メンバ初期化を追加するときに迷う要素を減らせる気がします。よくよく考えてみたら、それほどしょーもない指摘でもなかった・・・。
例：
CLayoutMgr::CLayoutMgr()
: m_getIndentOffset( &CLayoutMgr::getIndentOffset_Normal ) // Oct. 1, 2002 genta // Nov. 16, 2002 メンバー関数ポインタにはクラス名が必要
, m_layoutMemRes(new CPoolResource<CLayout>())
There was a problem hiding this comment.
記述スタイルは癖とか好みの問題なんで自分は周りに合わせた方が良いと感じた時は合わせてました。
今回は berryzplus さんの指摘に従う事にします。
| @@ -95,7 +98,8 @@ void CLayoutMgr::_Empty() | |||
| CLayout* pLayout = m_pLayoutTop; | |||
| while( pLayout ){ | |||
| CLayout* pLayoutNext = pLayout->GetNextLayout(); | |||
| delete pLayout; | |||
| pLayout->~CLayout(); | |||
| m_layoutMemRes->deallocate(pLayout, sizeof(CLayout), alignof(CLayout)); | |||
There was a problem hiding this comment.
m_layoutMemRes が指すポインタ型で、配置delを呼んでメモリ解放を行うメソッドを提供すれば1行で書けると思います。おそらく、1行で書けるメソッドを新設するほうがコードの意図が分かりやすくなる気がします。
There was a problem hiding this comment.
m_layoutMemRes の型は std::unique_ptr<std::pmr::memory_resource> です。std::pmr::memory_resource にはそういうメソッドが無いのでそれを実現するのは厳しくないでしょうか？
https://en.cppreference.com/w/cpp/memory/memory_resource
std::pmr::polymorphic_allocator には construct メソッドがありますが、destroy メソッドはVS2017だと使えませんでした（追記 VS2019 16.3.0 Preview 1.0 でも無い事確認）。new_object と delete_object メソッドは C++20 からのようです。当初のコミットでは std::pmr::polymorphic_allocator 経由のコードでしたが、メンバーの数が増えちゃうので使わないようにしました。
ちなみに何故抽象I/F型の std::pmr::memory_resource を使っているかというと差し替えられるようにする為にです。直接独自クラスを使うようにすれば berryzplus さんが提案するメソッドも追加して利用出来るし仮想関数呼び出しのオーバーヘッドも無いのでそうしたい気持ちも少し有ります。本来は（色々とレビューがややこしい事になりがちな）自作クラスを使わずに std::pmr::unsynchronized_pool_resource で済ませたかったんですがメモリ使用量が激増してしまったので諦めました。
There was a problem hiding this comment.
テキトーに行間読んでくださいね。こういうことです。
こういうことです。コードの行間はテキトーに補ってください。
template<typename T>
struct IMemoryPool
{
virtual T* allocate() = 0;
virtual void deallocate(T* p) = 0;
}
template<typename T>
class CPoolResource : public IMemoryPool<T>, public std::pmr::memory_resource
{
}
class CLayoutMgr
{
std::unique_ptr<IMemoryPool> m_layoutMemRes;
}
CLayoutMgr::CLayoutMgr()
: m_layoutMemRes(new CPoolResource<CLayout>()) {}
There was a problem hiding this comment.
いやーそこまで行間読むことは出来ませんでした。
その書き方にしたら将来的に std::pmr::unsynchronized_pool_resource への差し替えが行いにくくなるしどうかなぁと思います。それにそう書くくらいなら CPoolResource 直接使った方が良いのでは？
There was a problem hiding this comment.
テキトーに行間読んでくださいね。こういうことです。
いやーそこまで行間読むことは出来ませんでした。
ニュアンスが変に伝わったように感じたので訂正。
誤）テキトーに行間読んでくださいね。こういうことです。
正）こういうことです。コードの行間はテキトーに補ってください。
前者の書きっぷりだと「ちゃんと行間読み取ってもらわないと困ります。」の意味に取れないこともない気がしました。ぼくそんな高慢な発想しないんですが、そう読めないこともない 😄
There was a problem hiding this comment.
#980 では CMemPool::Construct メソッド（Variadic template 使用）がメモリ確保とコンストラクタ呼び出しをまとめて行うようになっていて、CMemPool::Destruct はデストラクタ呼び出しとメモリ解放をまとめて行うようになってました。このPRでは抽象I/F型の std::pmr::memory_resource ベース前提で利用側の手続きが増えちゃってるので詳細を隠蔽したいマインドと相反するとは思います。
C++20 まで待てば std::pmr::polymorphic_allocator に new_object と delete_object メソッドが追加されるみたいで型安全志向の人もにんまり、となるかもしれませんがなんちゃって銀の弾を使っているだけだと狼人間に食べられてしまいそうです。
There was a problem hiding this comment.
テキトーに行間読んでくださいね。こういうことです。
いやーそこまで行間読むことは出来ませんでした。
ニュアンスが変に伝わったように感じたので訂正。
誤）
テキトーに行間読んでくださいね。こういうことです。
正）こういうことです。コードの行間はテキトーに補ってください。前者の書きっぷりだと「ちゃんと行間読み取ってもらわないと困ります。」の意味に取れないこともない気がしました。ぼくそんな高慢な発想しないんですが、そう読めないこともない 😄
冷静に考えてみると、自然言語の数行の行間から20行程度の疑似コードを読み取ってくれ、っていうのも発想として無理がありますね（コンテキストから補間してそれが可能なケースもありそうですけど圧縮率がやけに高い…）
std::pmr::memory_resource ベースにしたせいで色々弊害というか記述がすっきりしない点が出てますね。#980 では cacay/MemoryPool を参考にした自作メモリプールだけで済ませたらその実装内容の詳細問い詰めコースに入ってしまいましたが、このPRでは今後どうなる事やら ❔
| @@ -381,7 +385,7 @@ CLayout* CLayoutMgr::CreateLayout( | |||
| CLayoutColorInfo* colorInfo | |||
| ) | |||
| { | |||
| CLayout* pLayout = new CLayout( | |||
| CLayout* pLayout = new (m_layoutMemRes->allocate(sizeof(CLayout))) CLayout( | |||
There was a problem hiding this comment.
m_layoutMemRes が指すポインタ型で、メモリ確保を行って配置newを呼び出すメソッドを提供すれば1行で書けると思います。おそらく、1行で書けるメソッドを新設するほうがコードの意図が分かりやすくなる気がします。
There was a problem hiding this comment.
#985 (comment) で書いたのと同じ理由ですが、抽象I/F型である std::pmr::memory_resource にはそういうメソッドは無いので難しいと思います。std::pmr::polymorphic_allocator::new_object がそういうメソッドですが C++20 以降のもので使えません。
自分が加えた変更が少し読みにくいのは否定出来ませんが我慢してほしいです。
There was a problem hiding this comment.
この指摘、対応不要っす。
同じ系統ぜんぶ(=xxx が指すポインタ型でry)。
| @@ -589,7 +593,8 @@ CLayout* CLayoutMgr::DeleteLayoutAsLogical( | |||
| DEBUG_TRACE( _T("バグバグ\n") ); | |||
| } | |||
|
|
|||
| delete pLayout; | |||
| pLayout->~CLayout(); | |||
| m_layoutMemRes->deallocate(pLayout, sizeof(CLayout), alignof(CLayout)); | |||
There was a problem hiding this comment.
m_layoutMemRes が指すポインタ型で(ry
There was a problem hiding this comment.
#985 (comment) で回答しました。
sakura_core/doc/layout/CLayoutMgr.h
Outdated
| @@ -37,7 +38,6 @@ | |||
| #include "util/design_template.h" | |||
|
|
|||
| class CBregexp;// 2002/2/10 aroka | |||
| class CLayout;// 2002/2/10 aroka | |||
There was a problem hiding this comment.
何故削る？削らなくても害がなさそうなのに削っているのが気になりました。
There was a problem hiding this comment.
削らないでも害は確かに無いのかもしれませんが削ってもビルドが通るという事は不要な記述だろうと判断しました。
#include "mem/CMemoryIterator.h"があってそこで
#include "doc/layout/CLayout.h"しているので前方参照が不要という認識です。
「心のオアシスなんだから削るな！」と主張される場合は戻しておきます。
There was a problem hiding this comment.
削る必要がないのであれば残しておいてもいい気がします。
論点がブレる要因になりうるので 😄
「削るべきじゃない」とまでは思わないので、他の気になるところが片付いたらスルーする予定です。
There was a problem hiding this comment.
当初はヘッダファイル CLayoutMgr.h に std::pmr::polymorphic_allocator<CLayout> 型のメンバーを追加していて、その為に型のレイアウトをヘッダ側で知る必要があって前方宣言じゃ駄目なので削ったりしました。でも削った後に #include "doc/layout/CLayout.h" を追加しないでもビルドが通るので不思議に思って調べてみたら #include している別のヘッダファイルが #include してました。まぁつまり全て void* だけで頑張るべきという事でしょうか…。
There was a problem hiding this comment.
単純に削ると CLayout には依存しなくなった の意図に見えます。
実態は 前方宣言じゃ不十分なくらい強く依存している で、前方宣言の代わりに #include "CLayout.h" を追加する必要があった だと思います。
削る必要はないけど削る、を是とするなら、
#include "CLayout.h" もできれば入れたいです。
前方宣言じゃ不十分で、includeしないといけない、が削った理由なのだから。
There was a problem hiding this comment.
でも他のヘッダファイルが既に #include しているわけで、ポーズにしかならない気がします。
There was a problem hiding this comment.
b67852b で前方宣言は削らないで元に戻しておきました。ここに関してはこだわる必要は無いですね。
| : | ||
| m_docLineMemRes(new CPoolResource<CDocLine>()) | ||
| //m_docLineMemRes(new std::pmr::unsynchronized_pool_resource()) // メモリ使用量が大きい為に使用しないで、CPoolResource を代わりに使う | ||
|
|
There was a problem hiding this comment.
同じことなんで、例示だけ書きます。
CDocLineMgr::CDocLineMgr()
: m_docLineMemRes(new CPoolResource<CDocLine>())
{
中括弧の前の空行は要らない気がします。
There was a problem hiding this comment.
独立した行に配置する事で CPoolResource と std::pmr::unsynchronized_pool_resource のどちらかをコメントアウトさせての切り替えを行いやすくするためにそう書きました。まぁどうせ切り替えやすくする事が目的なら下記のように書いた方が良いかも知れないですが。
CDocLineMgr::CDocLineMgr()
:
#if 1
m_docLineMemRes(new CPoolResource<CDocLine>())
#else
m_docLineMemRes(new std::pmr::unsynchronized_pool_resource()) // メモリ使用量が大きい為に使用しないで、CPoolResource を代わりに使う
#endif今回は指摘に従って下のように書こうかと思います。
CDocLineMgr::CDocLineMgr()
: m_docLineMemRes(new CPoolResource<CDocLine>())
// : m_docLineMemRes(new std::pmr::unsynchronized_pool_resource()) // メモリ使用量が大きい為に使用しないで、CPoolResource を代わりに使う| @@ -68,15 +73,15 @@ CDocLineMgr::~CDocLineMgr() | |||
| //! pPosの直前に新しい行を挿入 | |||
| CDocLine* CDocLineMgr::InsertNewLine(CDocLine* pPos) | |||
| { | |||
| CDocLine* pcDocLineNew = new CDocLine; | |||
| CDocLine* pcDocLineNew = new (m_docLineMemRes->allocate(sizeof(CDocLine))) CDocLine(); | |||
There was a problem hiding this comment.
前の指摘と同じです。 m_docLineMemRes に専用のメソッドを作って「 m_docLineMemRes で allocate する」とはどういうことか？の詳細を実装したメソッドを用意すべきだと思います。
There was a problem hiding this comment.
#985 (comment) と同じ回答になりますが、m_docLineMemRes の型は std::unique_ptr<std::pmr::memory_resource> です。std::pmr::memory_resource は標準ライブラリの抽象I/F型で、派生クラスを直接使用しているわけではないので指摘に対応するのが難しいのではないかと考えています。
| #include <memory_resource> | ||
| #include <Windows.h> | ||
|
|
||
| // std::pmr::unsynchronized_pool_resource だとメモリ使用量が大きい為、自前実装を用意 |
There was a problem hiding this comment.
正確な数字は要らんですが、どれくらい大きいかの情報があると嬉しいです。
たとえば「128MiB程度なら誤差なのでTS使ったほうがいいと思います」な考え方もありだと思います。
There was a problem hiding this comment.
x64 Release の場合で、a\r\n というだけの行が 16777216 行連続するファイルを開いた場合のメモリ使用量（タスクマネージャーで確認）を記載します。
| 方法 | メモリ使用量（単位 MB） |
|---|---|
new |
2578.5 |
CPoolResource |
2195.9 |
std::pmr::unsynchronized_pool_resource |
3335.2 |
#985 (comment) にも記載しましたが、std::pmr::pool_options の値を調整しても std::pmr::unsynchronized_pool_resource を使った場合のメモリ使用量を減らす事が出来ませんでした。どうしてこんなにメモリ使用量が増えてしまうのか実装内容をちゃんと把握出来ていませんが、これだけ増えてしまうのでは使うのが厳しいなと判断して自前実装のメモリプールを使っています。
There was a problem hiding this comment.
1GiB余分に食ってしまう結果になったわけですね・・・。
検証にx64版を使っているのは、メモリプールだけで2GBを超えてしまうから、と・・・。
PRに含まれるコミットとこのあたりの資料を参考に、手元で少し検証してみようと思っています。
https://cpprefjp.github.io/reference/memory_resource/pool_resource.html
なんとなく「上流のメモリリソース」がキーワードな気がしています。
- 上流のメモリリソース （確保単位ガバガバ）
- CLayoutMgr のメモリリソース （確保単位＝sizeof(CLayout)）
技術的な詳細を把握しないままテキトー言ってるだけなので、試してみてやっぱり駄目かもしらんです。できるなら理想的な使い方をしたいのもありますし、できないならそれは暫定策を許容する理由になる気がします。
There was a problem hiding this comment.
std::pmr::unsynchronized_pool_resource の実装を確認したところ、_Find_pool メソッドで2の累乗サイズに切り上げしていました。
C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\VC\Tools\MSVC\14.16.27023\include\memory_resource の L639 ～
現時点で x64 Release ビルドでは ←間違い、Debugビルドで確認してました。sizeof(CDocLine) が 64 でそれに sizeof(void *) を足した値が 72 になり、切り上げた2の累乗値が 128 なので、1つのインスタンス毎に 128 バイトずつ使ってしまうのかもしれないです。
今こんな気分です。
l⌒Yl ｌY⌒ｌ ＼メモリプールでメモリ節約！／
{ ´┴｀} { ´┴｀} (＾ν＾)
（ |￣￣| ） /（ ）＼
| |￣￣￣￣￣| | |
は？
l⌒Yl ｌY⌒ｌ
{ ´┴｀} { ´┴｀} (＾ν＾) ・・・
（ |￣￣| ） /（ ）＼
| |￣￣￣￣￣| | |
l⌒Yl ｌY⌒ｌ ＼C++17 ﾏﾝｾｰ！！／
{ ´┴｀} { ´┴｀} (＾ν＾)
（ |￣￣| ） /（ ）＼
| |￣￣￣￣￣| | |
＿＿＿
/ ||￣￣||
|.....||＿＿|| （ν＾ ） ＜ｹﾞｲｼ組はアスペ 標準ライブラリ実装が豚の餌
|￣￣＼三⊂/￣￣￣/
| | ( ./ /
There was a problem hiding this comment.
std::pmr::unsynchronized_pool_resource ですが手持ちのプールの容量が足りなくなった時に _Increase_capacity メソッドで upstream から追加で Chunk を確保しますが、そのサイズ（_Next_capacity）を倍々で増やしています。これがメモリを浪費している原因じゃないでしょうか。
There was a problem hiding this comment.
std::pmr::unsynchronized_pool_resourceを削る前のコードって、「上流のメモリプール」を使ってなくないですか？
std::pmr::unsynchronized_pool_resource のコンストラクタには上流リソースを指定していません（デフォルトコンストラクタを使用）。その場合は std::pmr::get_default_resource() が使われる仕様と実装になっていますね。
アラインメントに指定できる値の要求仕様は「2の累乗であること」らしいです。
std::pmr::unsynchronized_pool_resource の実装だとアライメントだけじゃなくメモリプール内の個々の要素であるブロックのサイズも2の累乗にしているのでメモリを無駄遣いしてるように思えます。
There was a problem hiding this comment.
テスト条件 16777216 行連続する からして、
確保サイズが1バイト無駄になるごとに 17MiB の領域が無駄になるということですよね。
アラインメントの関係で無駄になるサイズが100バイト近いとすれば
17MiB × 100倍 で 1GB が浪費される事態にも納得がいきます。
（そういう理屈の浪費なら1GB程度は許容範囲な気もします）
#988 CDocLine の容量削減 のような対応が必要なのかも知れません。
確保単位が2の累乗に切り上げられるということは、32bytesの上は64bytesになります。
現状の sizeof(CDocLine) が56bytesになるそうなので、
64 - 56 で1行あたり、8バイト無駄になる計算です。
ただ小さくすればいいという話ではなく、
CDocLineのサイズを2の累乗になるように調整する(無駄な確保を無くす)
という方向にすべきなんでしょうね。
std::pmr::get_default_resource() の戻り値は事前に set しない限り未定義、という仕様があるっぽいです。
ぼくが認識する限り、VirtualAllocは確保サイズに制約のある使い勝手の悪いメモリ確保関数です。裏をかえせば、通常よりも大きな単位で確保を行う「上流メモリリソース」の実装向きなんじゃないかと思います。
っていうか、このPRをマージできるようにするための「落としどころ」がどの辺にありそうか、よく分らなくなってきてるのはぼくだけですかね？
早い話、一旦妥協してもいいんじゃね？と思い始めています。
There was a problem hiding this comment.
テスト条件
16777216 行連続するからして、
確保サイズが1バイト無駄になるごとに 17MiB の領域が無駄になるということですよね。アラインメントの関係で無駄になるサイズが100バイト近いとすれば
17MiB × 100倍 で 1GB が浪費される事態にも納得がいきます。
（そういう理屈の浪費なら1GB程度は許容範囲な気もします）
1GBもメモリ使用量が増えてしまうのは許容したくないです。2^24 行とかのファイルを開く事は通常は無いっちゃ無いですが。。
#988 CDocLine の容量削減 のような対応が必要なのかも知れません。
#988 では簡単に出来る対処で容量を減らしました。
32bit から 64bit になってポインタのサイズが 4バイトから 8バイトに増えたので、ポインタを多用してると結構使用メモリ量が増えてしまいます。
確保単位が2の累乗に切り上げられるということは、32bytesの上は64bytesになります。
現状のsizeof(CDocLine)が56bytesになるそうなので、
64 - 56で1行あたり、8バイト無駄になる計算です。ただ小さくすればいいという話ではなく、
CDocLineのサイズを2の累乗になるように調整する(無駄な確保を無くす)
という方向にすべきなんでしょうね。
インスタンス１つにつき 8バイト無駄になるだけでは、1GB のメモリ使用量の差は説明できません。現在のVC++付属の std::pmr::unsynchronized_pool_resource を使うとメモリ使用量が大幅に増えてしまう主要因は、追加のメモリ確保を行う方法にあるという理解です。
デバッグしやすいように Debug ビルドで、CLayoutMgr.cpp で CPoolResource の代わりに std::pmr::unsynchronized_pool_resource を使うように記述を切り替えてからCLayoutMgr::CreateLayout メソッドの先頭にブレークポイントを置いて下さい。
ビルドして起動した後に行追加操作を行うと先ほど追加したブレークポイントに止まるので、Step Into 操作を行ったりして memory_resource ヘッダのコードがデバッグ出来ます。
std::pmr::unsynchronized_pool_resource::_Increase_capacity メソッドの上から3行目がメモリ確保を行っている場所です。memory_resource ファイルの行番号でいうと、597 行目です。
その呼び出しのサイズを追ってみると下記のようになります。
| n回目 | _Size | 行数 |
|---|---|---|
| 1 | 568 | 1 |
| 2 | 1080 | 5 |
| 3 | 2104 | 13 |
| 4 | 4152 | 29 |
| 5 | 8248 | 61 |
| 6 | 16440 | 125 |
| 7 | 32824 | 253 |
2倍2倍で確保する容量を増やしていく方式な事が確認出来ます。行数がやたらと多いファイルを開くと、途中で今まで確保した容量の合計以上の容量を一気に追加確保するのでそれでメモリ使用量が大きく増えてしまっているという事だと思います。
std::pmr::get_default_resource() の戻り値は事前に set しない限り未定義、という仕様があるっぽいです。
それは一体どこに書かれていた情報でしょうか？
https://en.cppreference.com/w/cpp/memory/get_default_resource
ここの説明を見る限りでは、std::pmr::new_delete_resource と同じみたいです。
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2017/n4713.pdf
の P.584 の先頭には下記の記載があります。
The default memory resource pointer is a pointer to a memory resource that is used by certain facilities when an explicit memory resource is not supplied through the interface. Its initial value is the return value of
new_delete_resource().
ぼくが認識する限り、VirtualAllocは確保サイズに制約のある使い勝手の悪いメモリ確保関数です。裏をかえせば、通常よりも大きな単位で確保を行う「上流メモリリソース」の実装向きなんじゃないかと思います。
性格的にメモリープールの実装に使うには丁度良いと思います。ヒープマネージャーがメモリ確保する為に使うAPIとしては、HeapAlloc か VirtualAlloc だと思いますが、余計なマネージメントをしてほしくない場合には VirtualAlloc が良いですね。
っていうか、このPRをマージできるようにするための「落としどころ」がどの辺にありそうか、よく分らなくなってきてるのはぼくだけですかね？
早い話、一旦妥協してもいいんじゃね？と思い始めています。
内部実装について考えるのが飽きてきたらブラックボックステストをするのはどうでしょうか？
- 色々な動作確認をして問題のない挙動をしているか？
- ファイル読み込みに掛かる時間は減っているか？
- ファイル読み込み後のメモリ使用量は減っているか？
等で妥当かどうかの判断が出来ると思います。え？そんなのかったるいからPR作成者が徹底的にユニットテストを書けって？:sweat_smile:
There was a problem hiding this comment.
std::pmr::get_default_resource() の戻り値は事前に set しない限り未定義、という仕様があるっぽいです。
それは一体どこに書かれていた情報でしょうか？
https://en.cppreference.com/w/cpp/memory/get_default_resource
ここの説明を見る限りでは、std::pmr::new_delete_resource と同じみたいです。
どっかの日本語サイトだと思いますが、覚えていないです。
未定義時はstd::pmr::new_delete_resourceの戻り値が返る、で間違いなさそうっすね。
There was a problem hiding this comment.
どっかの日本語サイトだと思いますが、覚えていないです。
😦
|
|
||
| Node* m_unassignedNode = nullptr; // 未割当領域の先頭 | ||
| Node* m_currentBlock = nullptr; // 現在のブロック | ||
| Node* m_currentNode = nullptr; // 要素確保処理時に現在のブロックの中から切り出すNodeを指すポインタ、メモリ確保時に未割当領域が無い場合はここを使う |
There was a problem hiding this comment.
伝統C++と現代C++の違いの一つに「メンバ変数を書く位置」というのがあります。
なんとなく、伝統派が後置、現代派が前置なイメージを持っています。
これは仕様で規定されるものではないので完全な「好み」の問題なのでどっちでもいいと思っています。
| void AllocateBlock() | ||
| { | ||
| char* buff = reinterpret_cast<char*>(VirtualAlloc(NULL, BlockSize, MEM_COMMIT | MEM_RESERVE, PAGE_READWRITE)); | ||
| Node* next = m_currentBlock; |
There was a problem hiding this comment.
メモリ確保に失敗した場合はどうなるんですかね？
元のコードがそのケースを想定しとらんはずなので、
「知りません」でも構わんと思うんですけど、
シグニチャに例外の扱いを示すキーワードが含まれないのが気になりました。
There was a problem hiding this comment.
元のコードだと new でのメモリ確保に失敗したら std::bad_alloc 例外が送出されるんでしょうかね。
このPRのコードだと VirtualAlloc でメモリ確保に失敗したら NULL が返ってその後ポインタを操作するので access violation になりそうです。
極限のケースに対する対処をちゃんと入れるかどうかですが、まぁ自分は入れる必要が無いなら入れないです。
There was a problem hiding this comment.
do_allocate が 上位のメモリリソースが例外を投げない限り例外を投げないで、
do_deallocate が 例外を投げないという仕様のようでした。
access violation で落ちるのはバグっぽくて感じ悪いと思います。
個人的には throw std::bad_alloc(); したいです。
最終的にユーザーにどう伝えるかを実装しないといけないんですが、それは別課題でいいように思います。
There was a problem hiding this comment.
アプリが異常終了して作業データが失われてしまったら人権侵害な環境にいる人はストレスマッハになってしまうのでそのうち対策入れたいですね。
There was a problem hiding this comment.
この要望、どうなる感じですかね？
現状）access violationで落ちる
提案）処理されない例外bad_allocのせいで落ちる
windowsが勝手に出してくれるダイアログで、違いは識別できるような気がするので入れて欲しいです。
There was a problem hiding this comment.
a97e50e で対応済みという認識です。
動作確認はしていません。
あれ？今までPRの変更に付随してMinGWでビルドが通らなくなった場合に修正が求められるのがデフォだったような気がしてならない…。 |
つ「明らかにコードが原因でない問題の発生時は」 潜在的なコードの問題（主に字句文法）を検出するのがMinGWビルドの目的なので、コードに問題がないことが明らかな場合にまで対応をお願いする気はないです。今回はローカルでの検証ビルドは通っていて、引っ越ししたばかりの azure pipelines のビルドに問題がありそうです。 |
|
✅ Build sakura 1.0.2128 completed (commit 497bf32856 by @beru) |
|
✅ Build sakura 1.0.2130 completed (commit 368b969a05 by @beru) |
|
✅ Build sakura 1.0.2131 completed (commit b833c407af by @beru) |
There was a problem hiding this comment.
お待たせしました。一旦コレでやってみよう、でハラが決まりました。
ラストだと思いますが、対応のお願いが1件あります。
プロジェクトファイルに今回追加する mem/CPoolResource.h を追加したいです。
GUIから既存ファイルを追加すると、sakura.vcxproj と sakura.vcxproj.filters に差分が出るはずなので。
あと、こっちは対応不要ですが、new/deleteを関数化する方法を手元で書いてみました。
※CDocLineMgr.h内の記述です。
private:
//! 新しい物理行のメモリを確保して初期化する
CDocLine* _AllocLine()
{
CDocLine* pcDocLineNew = nullptr;
m_docLineAllocator.construct(pcDocLineNew);
return pcDocLineNew;
}
//! 指定した物理行を破棄してメモリを解放する
void _FreeLine(CDocLine * const pDocLine)
{
pDocLine->~CDocLine();
m_docLineAllocator.deallocate(pDocLine, 1);
}細かいところは、まだまだだいぶ研究が必要っぽいです:smile:
仮に後でこのPRの変更に問題が有る事が分かったら revert する事で元の状態には戻せるので、ざっと確認してみて問題が無ければ取り入れてしまって良いんじゃないかと思います。 きちんと品質確保をするのであればユニットテストを整備する必要がありますが、もし書くとしたら
91befa4 で対応しました。
ヘッダファイル側を変更してメソッドを追加する方法よりグローバル関数にする方が良い気がします。そのグローバル関数をどこに追加するかとかを考える必要が生じてしまいますが。。 |
|
✅ Build sakura 1.0.2140 completed (commit e8f6357dbd by @beru) |
確認しました。対応ありがとうございます。
とはいえ、ほぼノーチェックでやるならdevブランチとか切ったりしたくなるわけで、その辺のまとめをしてくれる人も現状はおらんので、とりあえずはｇｄｇｄとレビューっぽいことをしている感じです。 プロジェクトを見捨てて独自路線に走る手もありますが、それだと永久に合流できなくなる可能性高そうで微妙。
ベースとすべき機能要求が不透明すぎるので、 ユーザー視点で考えると、網羅率100%のユニットテストを用意しても「あ、そうなの」です。 興味があるのは、困っていた不具合が直ったかどうかだったり、要望した機能が要望通りに入ったかどうかだったり、変更によって今まで使えていた機能が使えなくなったりしてないかだったりだと思います。 これらを保証できる余地があるとすれば、ユニットテストではなくファンクションテストだと思います。 まーこの辺は、全力で「今後の課題」としたいところですけどね 😄
アロケータもリソースもprivate変数なので、private関数にする想定でした。 |
|
@berryzplus さん レビューありがとうございました。もし問題が見つかったら別のPRで対処を行います。 |
メモリプールを使う事でメモリ確保と解放を高速化
PR の目的
ログファイル等の行数が多いファイルを読んで表示するのに掛かる時間を減らす。
カテゴリ
PR のメリット
行数が極端に多いファイルを開く時の待ち時間が短くなります。またメモリ使用量も少し減ります。あと分かりづらいですがそのファイルを閉じる時の時間も短くなります。
テストケースとしてちょっと極端な例ですが、a\r\n という行が 16777216行連続しているファイルを開いてから編集出来るようになるまでの待ち時間が短くなるのが体感できます。
そのファイルをzipで圧縮したもの → test.zip
自分の環境では以前は 7.8 秒ぐらい掛かっていたのが 6.7 秒ぐらいになりました。Visual Studio 2017 の Performance Profiler で起動して上記の大きいファイルを開いて確認しました。
PR のデメリット (トレードオフとかあれば)
以前は極端に行数が多い状態を作った後に行数が短い状態を作るとメモリ解放が行われてメモリ使用量が減りましたが、このメモリプールを使うやり方にすると減りません。
メモリ解放のタイミングが
CDocLineMgrやCLayoutMgrが解放されるタイミングになるので、エディタのプロセスを閉じるまでメモリプールのメモリ解放がされません。実際のユースケース的にそれで困るという事はあんまり考えられない気もします。
PR の影響範囲
CDocLineMgrとCLayoutMgr関連チケット
#312 #980