Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 7 commits into from
Aug 15, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions sakura/sakura.vcxproj
Original file line number Diff line number Diff line change
Expand Up @@ -438,6 +438,7 @@
<ClInclude Include="..\sakura_core\mem\CNativeA.h" />
<ClInclude Include="..\sakura_core\mem\CNativeT.h" />
<ClInclude Include="..\sakura_core\mem\CNativeW.h" />
<ClInclude Include="..\sakura_core\mem\CPoolResource.h" />
<ClInclude Include="..\sakura_core\mem\CRecycledBuffer.h" />
<ClInclude Include="..\sakura_core\mfclike\CMyWnd.h" />
<ClInclude Include="..\sakura_core\outline\CDlgFileTree.h" />
Expand Down
3 changes: 3 additions & 0 deletions sakura/sakura.vcxproj.filters
Original file line number Diff line number Diff line change
Expand Up @@ -1088,6 +1088,9 @@
<ClInclude Include="..\sakura_core\recent\CRecentExcludeFolder.h">
<Filter>Cpp Source Files\recent</Filter>
</ClInclude>
<ClInclude Include="..\sakura_core\mem\CPoolResource.h">
<Filter>Cpp Source Files\mem</Filter>
</ClInclude>
</ItemGroup>
<ItemGroup>
<None Include="..\sakura_core\Funccode_x.hsrc">
Expand Down
11 changes: 8 additions & 3 deletions sakura_core/doc/layout/CLayoutMgr.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
#include "charset/charcode.h"
#include "mem/CMemory.h"/// 2002/2/10 aroka
#include "mem/CMemoryIterator.h" // 2006.07.29 genta
#include "mem/CPoolResource.h"
#include "view/CViewFont.h"
#include "view/CTextMetrics.h"
#include "basis/SakuraBasis.h"
Expand All @@ -37,6 +38,8 @@

CLayoutMgr::CLayoutMgr()
: 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()) // メモリ使用量が大きい為に使用しない
{
m_pcDocLineMgr = NULL;
m_pTypeConfig = NULL;
Expand Down Expand Up @@ -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));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

m_layoutMemRes が指すポインタ型で、配置delを呼んでメモリ解放を行うメソッドを提供すれば1行で書けると思います。おそらく、1行で書けるメソッドを新設するほうがコードの意図が分かりやすくなる気がします。

Copy link
Contributor Author

@beru beru Aug 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_objectdelete_object メソッドは C++20 からのようです。当初のコミットでは std::pmr::polymorphic_allocator 経由のコードでしたが、メンバーの数が増えちゃうので使わないようにしました。

ちなみに何故抽象I/F型の std::pmr::memory_resource を使っているかというと差し替えられるようにする為にです。直接独自クラスを使うようにすれば berryzplus さんが提案するメソッドも追加して利用出来るし仮想関数呼び出しのオーバーヘッドも無いのでそうしたい気持ちも少し有ります。本来は(色々とレビューがややこしい事になりがちな)自作クラスを使わずに std::pmr::unsynchronized_pool_resource で済ませたかったんですがメモリ使用量が激増してしまったので諦めました。

Copy link
Contributor

@berryzplus berryzplus Aug 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

テキトーに行間読んでくださいね。こういうことです。
こういうことです。コードの行間はテキトーに補ってください。

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>()) {}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

いやーそこまで行間読むことは出来ませんでした。

その書き方にしたら将来的に std::pmr::unsynchronized_pool_resource への差し替えが行いにくくなるしどうかなぁと思います。それにそう書くくらいなら CPoolResource 直接使った方が良いのでは?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

テキトーに行間読んでくださいね。こういうことです。

いやーそこまで行間読むことは出来ませんでした。

ニュアンスが変に伝わったように感じたので訂正。

誤)テキトーに行間読んでくださいね。こういうことです。
正)こういうことです。コードの行間はテキトーに補ってください。

前者の書きっぷりだと「ちゃんと行間読み取ってもらわないと困ります。」の意味に取れないこともない気がしました。ぼくそんな高慢な発想しないんですが、そう読めないこともない 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#980 では CMemPool::Construct メソッド(Variadic template 使用)がメモリ確保とコンストラクタ呼び出しをまとめて行うようになっていて、CMemPool::Destruct はデストラクタ呼び出しとメモリ解放をまとめて行うようになってました。このPRでは抽象I/F型の std::pmr::memory_resource ベース前提で利用側の手続きが増えちゃってるので詳細を隠蔽したいマインドと相反するとは思います。

C++20 まで待てば std::pmr::polymorphic_allocatornew_objectdelete_object メソッドが追加されるみたいで型安全志向の人もにんまり、となるかもしれませんがなんちゃって銀の弾を使っているだけだと狼人間に食べられてしまいそうです。

https://tabesugi.net/memo/2009/1a.html

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

テキトーに行間読んでくださいね。こういうことです。

いやーそこまで行間読むことは出来ませんでした。

ニュアンスが変に伝わったように感じたので訂正。

誤)テキトーに行間読んでくださいね。こういうことです。
正)こういうことです。コードの行間はテキトーに補ってください。

前者の書きっぷりだと「ちゃんと行間読み取ってもらわないと困ります。」の意味に取れないこともない気がしました。ぼくそんな高慢な発想しないんですが、そう読めないこともない 😄

冷静に考えてみると、自然言語の数行の行間から20行程度の疑似コードを読み取ってくれ、っていうのも発想として無理がありますね(コンテキストから補間してそれが可能なケースもありそうですけど圧縮率がやけに高い…)

std::pmr::memory_resource ベースにしたせいで色々弊害というか記述がすっきりしない点が出てますね。#980 では cacay/MemoryPool を参考にした自作メモリプールだけで済ませたらその実装内容の詳細問い詰めコースに入ってしまいましたが、このPRでは今後どうなる事やら ❔

pLayout = pLayoutNext;
}
}
Expand Down Expand Up @@ -381,7 +385,7 @@ CLayout* CLayoutMgr::CreateLayout(
CLayoutColorInfo* colorInfo
)
{
CLayout* pLayout = new CLayout(
CLayout* pLayout = new (m_layoutMemRes->allocate(sizeof(CLayout))) CLayout(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

m_layoutMemRes が指すポインタ型で、メモリ確保を行って配置newを呼び出すメソッドを提供すれば1行で書けると思います。おそらく、1行で書けるメソッドを新設するほうがコードの意図が分かりやすくなる気がします。

Copy link
Contributor Author

@beru beru Aug 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#985 (comment) で書いたのと同じ理由ですが、抽象I/F型である std::pmr::memory_resource にはそういうメソッドは無いので難しいと思います。std::pmr::polymorphic_allocator::new_object がそういうメソッドですが C++20 以降のもので使えません。

自分が加えた変更が少し読みにくいのは否定出来ませんが我慢してほしいです。

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

この指摘、対応不要っす。
同じ系統ぜんぶ(=xxx が指すポインタ型でry)。

pCDocLine,
ptLogicPos,
nLength,
Expand Down Expand Up @@ -589,7 +593,8 @@ CLayout* CLayoutMgr::DeleteLayoutAsLogical(
DEBUG_TRACE( _T("バグバグ\n") );
}

delete pLayout;
pLayout->~CLayout();
m_layoutMemRes->deallocate(pLayout, sizeof(CLayout), alignof(CLayout));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

m_layoutMemRes が指すポインタ型で(ry

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#985 (comment) で回答しました。


m_nLines--; /* 全物理行数 */
if( NULL == pLayoutNext ){
Expand Down
2 changes: 2 additions & 0 deletions sakura_core/doc/layout/CLayoutMgr.h
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@

#include <Windows.h>// 2002/2/10 aroka
#include <vector>
#include <memory_resource>
#include "doc/CDocListener.h"
#include "_main/global.h"// 2002/2/10 aroka
#include "basis/SakuraBasis.h"
Expand Down Expand Up @@ -399,6 +400,7 @@ class CLayoutMgr : public CProgressSubject
//実データ
CLayout* m_pLayoutTop;
CLayout* m_pLayoutBot;
std::unique_ptr<std::pmr::memory_resource> m_layoutMemRes;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

std::unique_ptr<std::pmr::memory_resource>std::unique_ptr<独自型> とすれば先の提案が実現できると思います。独自に定義する型なら都合のいいインターフェースを定義できるはず。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

何故今の記述にしているかというと標準ライブラリにある std::pmr::memory_resource から派生しているクラスを使う事を本来は想定していたからです。


//タイプ別設定
const STypeConfig* m_pTypeConfig;
Expand Down
13 changes: 9 additions & 4 deletions sakura_core/doc/logic/CDocLineMgr.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
// May 15, 2000 genta
#include "CEol.h"
#include "mem/CMemory.h"// 2002/2/10 aroka
#include "mem/CPoolResource.h"

#include "io/CFileLoad.h" // 2002/08/30 Moca
#include "io/CIoBridge.h"
Expand All @@ -52,6 +53,8 @@
// -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- //

CDocLineMgr::CDocLineMgr()
: m_docLineMemRes(new CPoolResource<CDocLine>())
//: m_docLineMemRes(new std::pmr::unsynchronized_pool_resource()) // メモリ使用量が大きい為に使用しない
{
_Init();
}
Expand All @@ -68,15 +71,15 @@ CDocLineMgr::~CDocLineMgr()
//! pPosの直前に新しい行を挿入
CDocLine* CDocLineMgr::InsertNewLine(CDocLine* pPos)
{
CDocLine* pcDocLineNew = new CDocLine;
CDocLine* pcDocLineNew = new (m_docLineMemRes->allocate(sizeof(CDocLine))) CDocLine();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

前の指摘と同じです。 m_docLineMemRes に専用のメソッドを作って「 m_docLineMemRes で allocate する」とはどういうことか?の詳細を実装したメソッドを用意すべきだと思います。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#985 (comment) と同じ回答になりますが、m_docLineMemRes の型は std::unique_ptr<std::pmr::memory_resource> です。std::pmr::memory_resource は標準ライブラリの抽象I/F型で、派生クラスを直接使用しているわけではないので指摘に対応するのが難しいのではないかと考えています。

_InsertBeforePos(pcDocLineNew,pPos);
return pcDocLineNew;
}

//! 最下部に新しい行を挿入
CDocLine* CDocLineMgr::AddNewLine()
{
CDocLine* pcDocLineNew = new CDocLine;
CDocLine* pcDocLineNew = new (m_docLineMemRes->allocate(sizeof(CDocLine))) CDocLine();
_PushBottom(pcDocLineNew);
return pcDocLineNew;
}
Expand All @@ -87,7 +90,8 @@ void CDocLineMgr::DeleteAllLine()
CDocLine* pDocLine = m_pDocLineTop;
while( pDocLine ){
CDocLine* pDocLineNext = pDocLine->GetNextLine();
delete pDocLine;
pDocLine->~CDocLine();
m_docLineMemRes->deallocate(pDocLine, sizeof(CDocLine));
pDocLine = pDocLineNext;
}
_Init();
Expand Down Expand Up @@ -118,7 +122,8 @@ void CDocLineMgr::DeleteLine( CDocLine* pcDocLineDel )
}

//データ削除
delete pcDocLineDel;
pcDocLineDel->~CDocLine();
m_docLineMemRes->deallocate(pcDocLineDel, sizeof(CDocLine));

//行数減算
m_nLines--;
Expand Down
4 changes: 3 additions & 1 deletion sakura_core/doc/logic/CDocLineMgr.h
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,13 @@
#define _CDOCLINEMGR_H_

#include <Windows.h>
#include <memory_resource>
#include "_main/global.h" // 2002/2/10 aroka
#include "basis/SakuraBasis.h"
#include "util/design_template.h"
#include "COpe.h"
#include "CDocLine.h"

class CDocLine; // 2002/2/10 aroka
class CBregexp; // 2002/2/10 aroka

struct DocLineReplaceArg {
Expand Down Expand Up @@ -89,6 +90,7 @@ class CDocLineMgr{
CDocLine* m_pDocLineTop; //!< 最初の行
CDocLine* m_pDocLineBot; //!< 最後の行(※1行しかない場合はm_pDocLineTopと等しくなる)
CLogicInt m_nLines; //!< 全行数
std::unique_ptr<std::pmr::memory_resource> m_docLineMemRes;

public:
//$$ kobake注: 以下、絶対に切り離したい(最低切り離せなくても、変数の意味をコメントで明確に記すべき)変数群
Expand Down
137 changes: 137 additions & 0 deletions sakura_core/mem/CPoolResource.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,137 @@
/*
Copyright (C) 2018-2019 Sakura Editor Organization

This software is provided 'as-is', without any express or implied
warranty. In no event will the authors be held liable for any damages
arising from the use of this software.

Permission is granted to anyone to use this software for any purpose,
including commercial applications, and to alter it and redistribute it
freely, subject to the following restrictions:

1. The origin of this software must not be misrepresented;
you must not claim that you wrote the original software.
If you use this software in a product, an acknowledgment
in the product documentation would be appreciated but is
not required.

2. Altered source versions must be plainly marked as such,
and must not be misrepresented as being the original software.

3. This notice may not be removed or altered from any source
distribution.
*/

#ifndef SAKURA_CPOOLRESOURCE_H_
#define SAKURA_CPOOLRESOURCE_H_

#include <memory_resource>
#include <Windows.h>

// std::pmr::unsynchronized_pool_resource だとメモリ使用量が大きい為、自前実装を用意
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

正確な数字は要らんですが、どれくらい大きいかの情報があると嬉しいです。
たとえば「128MiB程度なら誤差なのでTS使ったほうがいいと思います」な考え方もありだと思います。

Copy link
Contributor Author

@beru beru Aug 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 を使った場合のメモリ使用量を減らす事が出来ませんでした。どうしてこんなにメモリ使用量が増えてしまうのか実装内容をちゃんと把握出来ていませんが、これだけ増えてしまうのでは使うのが厳しいなと判断して自前実装のメモリプールを使っています。

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1GiB余分に食ってしまう結果になったわけですね・・・。
検証にx64版を使っているのは、メモリプールだけで2GBを超えてしまうから、と・・・。

PRに含まれるコミットとこのあたりの資料を参考に、手元で少し検証してみようと思っています。
https://cpprefjp.github.io/reference/memory_resource/pool_resource.html

なんとなく「上流のメモリリソース」がキーワードな気がしています。

  • 上流のメモリリソース (確保単位ガバガバ)
    • CLayoutMgr のメモリリソース (確保単位=sizeof(CLayout))

技術的な詳細を把握しないままテキトー言ってるだけなので、試してみてやっぱり駄目かもしらんです。できるなら理想的な使い方をしたいのもありますし、できないならそれは暫定策を許容する理由になる気がします。

Copy link
Contributor Author

@beru beru Aug 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ビルドでは sizeof(CDocLine) が 64 でそれに sizeof(void *) を足した値が 72 になり、切り上げた2の累乗値が 128 なので、1つのインスタンス毎に 128 バイトずつ使ってしまうのかもしれないです。 ←間違い、Debugビルドで確認してました。

今こんな気分です。

     l⌒Yl  lY⌒l   \メモリプールでメモリ節約!/ 
    { ´┴`} { ´┴`}        (^ν^) 
    ( | ̄ ̄|   )       /( )\ 
     | | ̄ ̄ ̄ ̄ ̄|       | | 

   は? 
     l⌒Yl  lY⌒l 
    { ´┴`} { ´┴`}        (^ν^) ・・・ 
    ( | ̄ ̄|   )       /( )\ 
     | | ̄ ̄ ̄ ̄ ̄|       | | 


     l⌒Yl  lY⌒l   \C++17 マンセー!!/ 
    { ´┴`} { ´┴`}        (^ν^) 
    ( | ̄ ̄|   )       /( )\ 
     | | ̄ ̄ ̄ ̄ ̄|       | | 

 ___ 
/ || ̄ ̄|| 
|.....||__|| (ν^ )  <ゲイシ組はアスペ 標準ライブラリ実装が豚の餌 
| ̄ ̄\三⊂/ ̄ ̄ ̄/ 
|    | ( ./     / 

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

std::pmr::unsynchronized_pool_resource ですが手持ちのプールの容量が足りなくなった時に _Increase_capacity メソッドで upstream から追加で Chunk を確保しますが、そのサイズ(_Next_capacity)を倍々で増やしています。これがメモリを浪費している原因じゃないでしょうか。

Copy link
Contributor Author

@beru beru Aug 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

std::pmr::unsynchronized_pool_resourceを削る前のコードって、「上流のメモリプール」を使ってなくないですか?

std::pmr::unsynchronized_pool_resource のコンストラクタには上流リソースを指定していません(デフォルトコンストラクタを使用)。その場合は std::pmr::get_default_resource() が使われる仕様と実装になっていますね。

アラインメントに指定できる値の要求仕様は「2の累乗であること」らしいです。

std::pmr::unsynchronized_pool_resource の実装だとアライメントだけじゃなくメモリプール内の個々の要素であるブロックのサイズも2の累乗にしているのでメモリを無駄遣いしてるように思えます。

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

テスト条件 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をマージできるようにするための「落としどころ」がどの辺にありそうか、よく分らなくなってきてるのはぼくだけですかね?

早い話、一旦妥協してもいいんじゃね?と思い始めています。

Copy link
Contributor Author

@beru beru Aug 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

テスト条件 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の累乗になるように調整する(無駄な確保を無くす)
という方向にすべきなんでしょうね。

インスタンス1つにつき 8バイト無駄になるだけでは、1GB のメモリ使用量の差は説明できません。現在のVC++付属の std::pmr::unsynchronized_pool_resource を使うとメモリ使用量が大幅に増えてしまう主要因は、追加のメモリ確保を行う方法にあるという理解です。

デバッグしやすいように Debug ビルドで、CLayoutMgr.cppCPoolResource の代わりに 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としては、HeapAllocVirtualAlloc だと思いますが、余計なマネージメントをしてほしくない場合には VirtualAlloc が良いですね。

っていうか、このPRをマージできるようにするための「落としどころ」がどの辺にありそうか、よく分らなくなってきてるのはぼくだけですかね?

早い話、一旦妥協してもいいんじゃね?と思い始めています。

内部実装について考えるのが飽きてきたらブラックボックステストをするのはどうでしょうか?

  • 色々な動作確認をして問題のない挙動をしているか?
  • ファイル読み込みに掛かる時間は減っているか?
  • ファイル読み込み後のメモリ使用量は減っているか?

等で妥当かどうかの判断が出来ると思います。え?そんなのかったるいからPR作成者が徹底的にユニットテストを書けって?:sweat_smile:

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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の戻り値が返る、で間違いなさそうっすね。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

どっかの日本語サイトだと思いますが、覚えていないです。

😦

// T : 要素型
template <typename T>
class CPoolResource : public std::pmr::memory_resource
{
public:
CPoolResource()
{
// 始めのブロックをメモリ確保
AllocateBlock();
}

virtual ~CPoolResource()
{
// メモリ確保した領域の連結リストを辿って全てのブロック分のメモリ解放
Node* curr = m_currentBlock;
while (curr) {
Node* next = curr->next;
VirtualFree(curr, 0, MEM_RELEASE);
curr = next;
}
}

protected:
// 要素のメモリ確保処理、要素の領域のポインタを返す
// bytes と alignment 引数は使用しない、template 引数の型で静的に決定する
void* do_allocate([[maybe_unused]] std::size_t bytes,
[[maybe_unused]] std::size_t alignment) override
{
// メモリ確保時には未割当領域から使用していく
if (m_unassignedNode) {
T* ret = reinterpret_cast<T*>(m_unassignedNode);
m_unassignedNode = m_unassignedNode->next;
return ret;
}
else {
// 未割当領域が無い場合は、ブロックの中から切り出す
// 現在のブロックに新規確保するNodeサイズ分の領域が余っていない場合は新規のブロックを確保
Node* blockEnd = reinterpret_cast<Node*>(reinterpret_cast<char*>(m_currentBlock) + BlockSize);
if (m_currentNode + 1 >= blockEnd) {
AllocateBlock();
}
// 要素の領域のポインタを返すと同時にポインタを次に進めて切り出す位置を更新する
return reinterpret_cast<T*>(m_currentNode++);
}
}

// メモリ解放処理、要素の領域のポインタを受け取る
// 第1引数には、do_allocate メソッドで返したポインタを渡す事
// bytes と alignment 引数は使用しない、template 引数の型で静的に決定する
void do_deallocate(void* p,
[[maybe_unused]] std::size_t bytes,
[[maybe_unused]] std::size_t alignment) override
{
if (p) {
// メモリ解放した領域を未割当領域として自己参照共用体の片方向連結リストで繋げる
// 次回のメモリ確保時にその領域を再利用する
Node* next = m_unassignedNode;
m_unassignedNode = reinterpret_cast<Node*>(p);
m_unassignedNode->next = next;
}
}

bool do_is_equal(const std::pmr::memory_resource& other) const noexcept override
{
return this == &other;
}

private:
static constexpr size_t BlockSize = 1024 * 64; // VirtualAlloc を使用する為このサイズ

// 共用体を使う事で要素型と自己参照用のポインタを同じ領域に割り当てる
// 共用体のサイズは各メンバを格納できるサイズになる事を利用する
union Node {
~Node() {}
T element; // 要素型
Node* next; // ブロックのヘッダの場合は、次のブロックに繋がる
// 解放後の未割当領域の場合は次の未割当領域に繋がる
};

static_assert(2 * sizeof(Node) <= BlockSize, "sizeof(T) too big.");

// 呼び出しの度にメモリの動的確保を細かく行う事を避ける為に、一括でブロック領域を確保
// ブロックの先頭(head)にはブロックの連結用のポインタが配置され、残る領域(body)には要素が記録される
void AllocateBlock()
{
char* buff = reinterpret_cast<char*>(VirtualAlloc(NULL, BlockSize, MEM_COMMIT | MEM_RESERVE, PAGE_READWRITE));
if (!buff) throw std::bad_alloc();
Node* next = m_currentBlock;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

メモリ確保に失敗した場合はどうなるんですかね?

元のコードがそのケースを想定しとらんはずなので、
「知りません」でも構わんと思うんですけど、
シグニチャに例外の扱いを示すキーワードが含まれないのが気になりました。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

元のコードだと new でのメモリ確保に失敗したら std::bad_alloc 例外が送出されるんでしょうかね。

このPRのコードだと VirtualAlloc でメモリ確保に失敗したら NULL が返ってその後ポインタを操作するので access violation になりそうです。

極限のケースに対する対処をちゃんと入れるかどうかですが、まぁ自分は入れる必要が無いなら入れないです。

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do_allocate が 上位のメモリリソースが例外を投げない限り例外を投げないで、
do_deallocate が 例外を投げないという仕様のようでした。

access violation で落ちるのはバグっぽくて感じ悪いと思います。
個人的には throw std::bad_alloc(); したいです。
最終的にユーザーにどう伝えるかを実装しないといけないんですが、それは別課題でいいように思います。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

アプリが異常終了して作業データが失われてしまったら人権侵害な環境にいる人はストレスマッハになってしまうのでそのうち対策入れたいですね。

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

この要望、どうなる感じですかね?

現状)access violationで落ちる
提案)処理されない例外bad_allocのせいで落ちる

windowsが勝手に出してくれるダイアログで、違いは識別できるような気がするので入れて欲しいです。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a97e50e で対応済みという認識です。

動作確認はしていません。

// ブロック領域の先頭(head)はNodeのポインタとして扱い、以前に作成したブロックに連結する
m_currentBlock = reinterpret_cast<Node*>(buff);
m_currentBlock->next = next;

// ブロック領域の残る部分は要素の領域とするが、アライメントを取る
void* body = buff + sizeof(Node*);
size_t space = BlockSize - sizeof(Node*);
body = std::align(alignof(Node), sizeof(Node), body, space);
assert(body);
m_currentNode = reinterpret_cast<Node*>(body);
}

Node* m_unassignedNode = nullptr; // 未割当領域の先頭
Node* m_currentBlock = nullptr; // 現在のブロック
Node* m_currentNode = nullptr; // 要素確保処理時に現在のブロックの中から切り出すNodeを指すポインタ、メモリ確保時に未割当領域が無い場合はここを使う
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

伝統C++と現代C++の違いの一つに「メンバ変数を書く位置」というのがあります。
なんとなく、伝統派が後置、現代派が前置なイメージを持っています。
これは仕様で規定されるものではないので完全な「好み」の問題なのでどっちでもいいと思っています。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

間を取って中間ぐらいに書くべきでしょうか?

};

#endif /* SAKURA_CPOOLRESOURCE_H_ */