-
Notifications
You must be signed in to change notification settings - Fork 163
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
Changes from 4 commits
3cb6d47
832a459
8140918
1dff8f5
5a77edf
003d908
581e94d
b1559cb
9a2f705
8110e42
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,121 @@ | ||
/* | ||
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" | ||
|
||
template <typename T, size_t BlockSize = 4096> | ||
class CMemPool final | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. データ構造やロジックの説明をコメントに追加してほしいです。 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
} | ||
|
||
void Destruct(T* p) | ||
{ | ||
if (p) { | ||
p->~T(); | ||
Free(p); | ||
} | ||
} | ||
|
||
private: | ||
union Node { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ここでなぜ union を使用しているのですか? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 下記の2つの理由が考えられます。
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 自己参照用のポインタと要素型を同じ領域に配置して問題ないのですか? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 134行目の こんな感じ?
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 自己参照用のポインタは要素が破棄されて未割当領域になった際に使われるので問題無い認識です。ブロック先頭部分のポインタはブロック間を接続するのに使われて、先頭部分に要素は記録されない為問題無いです。 ブロックを構造体型として定義した方がコードの見通しが良くなるかもしれないですね。 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @m-tmatma さん
図をPlantUMLとかで用意する事は出来ますが、行動に移すのを躊躇する何かが有ります。 今回作成したメモリプールの実装は100行未満の低機能な単純なものなので、分かりやすいコードの書き方にする事に注力した方が良いかも知れません。 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 別に図にしなくても構造を説明する文章でもいいですが There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 現状のコード読んで作りが理解できないですか?それならこのPR自体諦めますけど。 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. コメントを書いてほしい理由は、初めて見た人が There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. コメントは不要です。 |
||
~Node() {} | ||
T element; | ||
Node* next; | ||
}; | ||
|
||
static_assert(BlockSize >= 2 * sizeof(Node), "BlockSize too small."); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ここの There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ブロック領域のサイズがNode2個分以上でなければいけない、という確認です。
ケースを限定すればもっと必要なサイズを絞れる気もしますが、まぁ気張らないで今のやり方でも良いとは思います。 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. コメントには There was a problem hiding this comment. Choose a reason for hiding this commentThe 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* border = reinterpret_cast<Node*>(reinterpret_cast<char*>(m_currentBlock) + BlockSize - sizeof(Node) + 1); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ここの There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ブロック領域中の残りの部分の大きさが Node の大きさ未満だともうそのブロック領域からは Node を切り出す事が出来ません。それを判定する際に使うポインタ変数のアドレス計算です。 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. データ構造はまだ理解できていませんが、 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 判定的に等価になるならどっちでも良いかと思います。 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 細かいこと言うと、+1 の演算が余計に入るので現状コードのほうが少し遅い。 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. そうかもしれないですね。 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. もうちょっとロジックを理解してから再度コメントするか考えます。 |
||
if (m_currentNode >= border) { | ||
AllocateBlock(); | ||
} | ||
return reinterpret_cast<T*>(m_currentNode++); | ||
} | ||
} | ||
|
||
void Free(T* p) | ||
{ | ||
if (p) { | ||
Node* next = m_unassignedNode; | ||
m_unassignedNode = reinterpret_cast<Node*>(p); | ||
m_unassignedNode->next = next; | ||
} | ||
} | ||
|
||
void AllocateBlock() | ||
{ | ||
char* buff = reinterpret_cast<char*>(operator new (BlockSize)); | ||
Node* next = m_currentBlock; | ||
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; | ||
}; | ||
|
||
#endif /* SAKURA_CMEMPOOL_H_ */ |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,118 @@ | ||
/*! @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 <stdint.h> | ||
#include <Windows.h> | ||
m-tmatma marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
#define is_aligned(POINTER, BYTE_COUNT) (((uintptr_t)(const void *)(POINTER)) % (BYTE_COUNT) == 0) | ||
|
||
template <typename T> | ||
void testPool(size_t sz) | ||
{ | ||
CMemPool<T> 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]); | ||
} | ||
} | ||
|
||
// デフォルトコンストラクタ | ||
TEST(CMemPool, default_constructor) | ||
{ | ||
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>(sz); | ||
testPool<int16_t>(sz); | ||
testPool<int32_t>(sz); | ||
testPool<int64_t>(sz); | ||
testPool<float>(sz); | ||
testPool<double>(sz); | ||
testPool<FILETIME>(sz); | ||
testPool<TestStruct>(sz); | ||
testPool<std::wstring>(sz); | ||
} | ||
} | ||
|
||
// 引数付きコンストラクタ | ||
TEST(CMemPool, parameterized_constructor) | ||
{ | ||
CMemPool<std::wstring> pool; | ||
m-tmatma marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
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); | ||
} | ||
|
||
// ブロックサイズ | ||
TEST(CMemPool, BlockSize) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. このテストは何をテストしようとしていますか? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. もちろんブロックサイズのテストをしようとしているのはわかります。 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. #980 (comment) について確認するテストです。 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 個別のケースに関して具体的にどのような観点でテストしようとしているのかの There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. コメントを追加しました。 |
||
{ | ||
CMemPool<std::array<uint8_t, 2048>> pool; | ||
CMemPool<std::array<uint8_t, 4096>, 8192> pool2; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
このクラスに対して、単体テストを実装してほしいです。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
追加しました。クラスのI/Fを意図的にミニマムにしたので試験内容も単純なものになっています。