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

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

Closed
wants to merge 10 commits into from
1 change: 1 addition & 0 deletions sakura/sakura.vcxproj
Original file line number Diff line number Diff line change
Expand Up @@ -430,6 +430,7 @@
<ClInclude Include="..\sakura_core\macro\CWSHManager.h" />
<ClInclude Include="..\sakura_core\mem\CMemory.h" />
<ClInclude Include="..\sakura_core\mem\CMemoryIterator.h" />
<ClInclude Include="..\sakura_core\mem\CMemPool.h" />
<ClInclude Include="..\sakura_core\mem\CNative.h" />
<ClInclude Include="..\sakura_core\mem\CNativeA.h" />
<ClInclude Include="..\sakura_core\mem\CNativeT.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\CMemPool.h">
<Filter>Cpp Source Files\mem</Filter>
</ClInclude>
</ItemGroup>
<ItemGroup>
<None Include="..\sakura_core\Funccode_x.hsrc">
Expand Down
8 changes: 4 additions & 4 deletions sakura_core/doc/logic/CDocLineMgr.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -68,15 +68,15 @@ CDocLineMgr::~CDocLineMgr()
//! pPosの直前に新しい行を挿入
CDocLine* CDocLineMgr::InsertNewLine(CDocLine* pPos)
{
CDocLine* pcDocLineNew = new CDocLine;
CDocLine* pcDocLineNew = m_memPool.Construct();
_InsertBeforePos(pcDocLineNew,pPos);
return pcDocLineNew;
}

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

//データ削除
delete pcDocLineDel;
m_memPool.Destruct(pcDocLineDel);

//行数減算
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 @@ -25,8 +25,9 @@
#include "basis/SakuraBasis.h"
#include "util/design_template.h"
#include "COpe.h"
#include "CDocLine.h"
#include "mem/CMemPool.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; //!< 全行数
CMemPool<CDocLine> m_memPool;

public:
//$$ kobake注: 以下、絶対に切り離したい(最低切り離せなくても、変数の意味をコメントで明確に記すべき)変数群
Expand Down
144 changes: 144 additions & 0 deletions sakura_core/mem/CMemPool.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,144 @@
/*
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_CMEMPOOL_H_
#define SAKURA_CMEMPOOL_H_

#include <memory>
#include "util/design_template.h"

// T : 要素型
// BlockSize : ブロックの大きさのバイト数
template <typename T, size_t BlockSize = 4096>
Copy link
Member

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.

追加しました。クラスのI/Fを意図的にミニマムにしたので試験内容も単純なものになっています。

class CMemPool final
Copy link
Member

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.

コメントを記載しました。

{
public:
DISALLOW_COPY_AND_ASSIGN(CMemPool);

CMemPool()
{
// 始めのブロックをメモリ確保
AllocateBlock();
}

~CMemPool()
{
// メモリ確保した領域の連結リストを辿って全てのブロック分のメモリ解放
Node* curr = m_currentBlock;
while (curr) {
Node* next = curr->next;
operator delete(reinterpret_cast<void*>(curr));
m-tmatma marked this conversation as resolved.
Show resolved Hide resolved
curr = next;
}
}

// 要素構築、引数は要素型のコンストラクタ引数
template <typename... Args>
T* Construct(Args&&... args)
{
T* p = Allocate();
new (p) T(std::forward<Args>(args)...);
return p;
}

// 要素破棄、引数は Construct が返したポインタ
void Destruct(T* p)
{
if (p) {
p->~T();
Free(p);
}
}

private:
// 共用体を使う事で要素型と自己参照用のポインタを同じ領域に割り当てる
// 共用体のサイズは各メンバを格納できるサイズになる事を利用する
union Node {
Copy link
Member

Choose a reason for hiding this comment

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

ここでなぜ union を使用しているのですか?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

下記の2つの理由が考えられます。

  • 領域を再利用する為に要素型と自己参照用のポインタを同じ領域に割り当てる
  • 共用体のサイズは各メンバを格納できるサイズになる事を利用

Copy link
Member

Choose a reason for hiding this comment

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

自己参照用のポインタと要素型を同じ領域に配置して問題ないのですか?
可能であれば、アスキーアート(あるいは SVG とか)でデータ構造の説明が欲しいです。

Copy link
Member

Choose a reason for hiding this comment

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

134行目の
body = std::align(alignof(Node), sizeof(Node), body, space); からすると

こんな感じ?


     +----------------------------------+
     |                                  V
+-----------+---------+------------+-----------+---------+------------+
| ポインタ  |         |            | ポインタ  |         |            |
|   next    | padding | T のデータ |   next    | padding | T のデータ |
|           |         |            |           |         |            |
+-----------+---------+------------+-----------+---------+------------+
                      ^                 |                                 ^
                      |                 +---------------------------------+
              タイプ T のサイズ向けに align

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 Author

Choose a reason for hiding this comment

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

@m-tmatma さん

可能であれば、アスキーアート(あるいは SVG とか)でデータ構造の説明が欲しいです。

図をPlantUMLとかで用意する事は出来ますが、行動に移すのを躊躇する何かが有ります。

今回作成したメモリプールの実装は100行未満の低機能な単純なものなので、分かりやすいコードの書き方にする事に注力した方が良いかも知れません。

Copy link
Member

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.

現状のコード読んで作りが理解できないですか?それならこのPR自体諦めますけど。

Copy link
Member

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.

コメントは不要です。

~Node() {}
T element; // 要素型
Node* next; // ブロックのヘッダの場合は、次のブロックに繋がる
// 解放後の未割当領域の場合は次の未割当領域に繋がる
};

// ブロックの大きさをNode2個分以上とする事で、最低限ブロック連結用のポインタとNode1つを記録出来る事を保証
static_assert(BlockSize >= 2 * sizeof(Node), "BlockSize too small.");
Copy link
Member

Choose a reason for hiding this comment

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

ここの 2 にはどういう意味がありますか?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ブロック領域のサイズがNode2個分以上でなければいけない、という確認です。
何故 2 なのか?という疑問が湧きますが、あんまり厳密なチェックではないと思います。

  • 要素型のサイズがポインタのサイズ以下のケース(Nodeのサイズはポインタのサイズ以上)
    • ブロック領域の先頭にはポインタが来ます。残りの領域のサイズも最低ポインタ分が無いと解放時に未割当領域に出来ません。ブロック領域のサイズがNode2個分以上ならそれを満たします。
  • 要素型のサイズがポインタのサイズより大きいケース(Nodeのサイズは要素型のサイズ以上)
    • ブロック領域の先頭にはポインタが来ます。残りの領域のサイズは Node 1つ以上有れば良いですが、ブロック領域のサイズがNode2個分以上なら条件満たします。

ケースを限定すればもっと必要なサイズを絞れる気もしますが、まぁ気張らないで今のやり方でも良いとは思います。

Copy link
Member

Choose a reason for hiding this comment

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

コメントには Node2個分以上 にする理由を記載いただきたいです。
Node2個分以上というのは見たらわかるので。

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* Allocate()
{
// メモリ確保時には未割当領域から使用していく
if (m_unassignedNode) {
T* ret = reinterpret_cast<T*>(m_unassignedNode);
m_unassignedNode = m_unassignedNode->next;
return ret;
}
else {
// 未割当領域が無い場合は、ブロックの中から切り出す
// 現在のブロックにNodeサイズ分の領域が無い場合は新規のブロックを確保
Node* border = reinterpret_cast<Node*>(reinterpret_cast<char*>(m_currentBlock) + BlockSize - sizeof(Node) + 1);
Copy link
Member

Choose a reason for hiding this comment

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

ここの +1 はどういう意味ですか?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ブロック領域中の残りの部分の大きさが Node の大きさ未満だともうそのブロック領域からは Node を切り出す事が出来ません。それを判定する際に使うポインタ変数のアドレス計算です。

Copy link
Member

Choose a reason for hiding this comment

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

データ構造はまだ理解できていませんが、
このコードを見たときに、 99 行目で +1 しているのは 100 行目で >= にしているからで
100 行目で > にすれば 99 行目の +1 はいらないんじゃないかと思ってコメントしました。

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
Member

Choose a reason for hiding this comment

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

細かいこと言うと、+1 の演算が余計に入るので現状コードのほうが少し遅い。

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
Member

Choose a reason for hiding this comment

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

もうちょっとロジックを理解してから再度コメントするか考えます。

if (m_currentNode >= border) {
AllocateBlock();
}
// 要素の領域のポインタを返すと同時にポインタを次に進めて切り出す位置を更新する
return reinterpret_cast<T*>(m_currentNode++);
}
}

// メモリ解放処理、要素の領域のポインタを受け取る
// Allocate メソッドで返したポインタを渡す事
void Free(T* p)
{
if (p) {
// メモリ解放した領域を未割当領域として自己参照共用体の片方向連結リストで繋げる
// 次回のメモリ確保時にその領域を再利用する
Node* next = m_unassignedNode;
m_unassignedNode = reinterpret_cast<Node*>(p);
m_unassignedNode->next = next;
}
}

// 呼び出しの度にメモリの動的確保を細かく行う事を避ける為に、一括でブロック領域を確保
// ブロックの先頭(head)にはブロックの連結用のポインタが配置され、残る領域(body)には要素が記録される
void AllocateBlock()
{
char* buff = reinterpret_cast<char*>(operator new (BlockSize));
Node* next = m_currentBlock;
// ブロック領域の先頭(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を指すポインタ、メモリ確保時に未割当領域が無い場合はここを使う
};

#endif /* SAKURA_CMEMPOOL_H_ */
140 changes: 140 additions & 0 deletions tests/unittests/test-cmempool.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,140 @@
/*! @file */
/*
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.
*/
#include <gtest/gtest.h>
#include "mem/CMemPool.h"
#include <vector>
#include <array>
#include <string>
#include <type_traits>
#include <stdint.h>
#include <Windows.h>
m-tmatma marked this conversation as resolved.
Show resolved Hide resolved

template <typename T>
class CMemPoolTest : public ::testing::Test{};

using test_types = ::testing::Types<
std::integral_constant<std::size_t, 512>,
Copy link
Member

Choose a reason for hiding this comment

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

BlockSize は 2の累乗である必要はありますか?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

2の累乗値である必要はありません。

Copy link
Member

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.

2の累乗値ではない値も追加しました。

std::integral_constant<std::size_t, 1024>,
std::integral_constant<std::size_t, 2048>,
std::integral_constant<std::size_t, 4096>,
std::integral_constant<std::size_t, 8192>,
std::integral_constant<std::size_t, 16384>,
std::integral_constant<std::size_t, 32768>,
std::integral_constant<std::size_t, 65536>
>;

TYPED_TEST_CASE(CMemPoolTest, test_types);

#define is_aligned(POINTER, BYTE_COUNT) (((uintptr_t)(const void *)(POINTER)) % (BYTE_COUNT) == 0)

template <typename T, size_t BlockSize>
void testPool(size_t sz)
{
CMemPool<T, BlockSize> pool;
std::vector<T*> ptrs(sz);
for (size_t i=0; i<sz; ++i) {
T* p = pool.Construct();
ptrs[i] = p;
ASSERT_TRUE(p != nullptr);
ASSERT_TRUE(is_aligned(p, alignof(T)));
}
for (size_t i=0; i<sz; ++i) {
pool.Destruct(ptrs[i]);
}
}

// デフォルトコンストラクタ
TYPED_TEST(CMemPoolTest, default_constructor)
{
constexpr size_t BlockSize = TypeParam::value;
struct TestStruct
{
int* ptr;
char buff[32];
long long ll;
double f64;
};

/*!
指定回数呼び出して落ちない事を確認する
*/
size_t sz = 1;
for (size_t i=0; i<10; ++i, sz*=2) {
testPool<int8_t, BlockSize>(sz);
testPool<int16_t, BlockSize>(sz);
testPool<int32_t, BlockSize>(sz);
testPool<int64_t, BlockSize>(sz);
testPool<float, BlockSize>(sz);
testPool<double, BlockSize>(sz);
testPool<FILETIME, BlockSize>(sz);
testPool<TestStruct, BlockSize>(sz);
testPool<std::wstring, BlockSize>(sz);
}
}

// 引数付きコンストラクタ
TYPED_TEST(CMemPoolTest, parameterized_constructor)
{
constexpr size_t BlockSize = TypeParam::value;
CMemPool<std::wstring, BlockSize> pool;

std::wstring* p0 = nullptr;
std::wstring* p1 = nullptr;
std::wstring* p2 = nullptr;
std::wstring* p3 = nullptr;

p0 = pool.Construct(L"あいうえお");
p1 = pool.Construct(L"nullptr");
p2 = pool.Construct(12345, '㌍');
std::wstring s{L"令和"};
p3 = pool.Construct(s);

ASSERT_TRUE(p0 != nullptr);
ASSERT_TRUE(p1 != nullptr);
ASSERT_TRUE(p2 != nullptr);
ASSERT_TRUE(p3 != nullptr);

ASSERT_TRUE(p0->size() == 5);
ASSERT_TRUE(p1->size() == 7);
ASSERT_TRUE(p2->size() == 12345);
ASSERT_TRUE(p3->size() == 2);
ASSERT_TRUE(*p3 == s);

pool.Destruct(p0);
pool.Destruct(p1);
pool.Destruct(p2);
pool.Destruct(p3);
}

// ブロックサイズは要素が2つ以上入る大きさにする確認
TEST(CMemPool, BlockSize)
Copy link
Member

Choose a reason for hiding this comment

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

このテストは何をテストしようとしていますか?

Copy link
Member

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 (comment) について確認するテストです。

Copy link
Member

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.

コメントを追加しました。

{
CMemPool<std::array<uint8_t, 1024>, 2048> pool0;
Copy link
Member

Choose a reason for hiding this comment

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

クラスをクラスタンス化するだけだと、AllocateBlock が呼ばれるだけですが、
Construct や Destruct を呼ぶ必要はないですか?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

色々な要素型の場合にBlockSizeを最小限の大きさにした場合に正常にCMemPoolが動作するかの確認をした方が良さそうですね。

CMemPool<std::array<uint8_t, 1025>, 4096> pool1;
CMemPool<std::array<uint8_t, 2048>, 4096> pool2;
CMemPool<std::array<uint8_t, 2049>, 8192> pool3;
CMemPool<std::array<uint8_t, 4096>, 8192> pool4;
}