Skip to content

Commit 43e26c7

Browse files
committed
Fix memory leak for bitmap in PAX
Bitmap should free memory if the memory is allocated by itself.
1 parent 60eb10d commit 43e26c7

File tree

6 files changed

+25
-37
lines changed

6 files changed

+25
-37
lines changed

contrib/pax_storage/src/cpp/access/pax_visimap.cc

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -143,8 +143,7 @@ bool TestVisimap(Relation rel, const char *visimap_name, int offset) {
143143
fs = Singleton<LocalFileSystem>::GetInstance();
144144

145145
auto visimap = LoadVisimap(fs, options, file_path);
146-
auto bm = Bitmap8(BitmapRaw<uint8>(visimap->data(), visimap->size()),
147-
Bitmap8::ReadOnlyOwnBitmap);
146+
auto bm = Bitmap8(BitmapRaw<uint8>(visimap->data(), visimap->size()));
148147
auto is_set = bm.Test(offset);
149148
return !is_set;
150149
}

contrib/pax_storage/src/cpp/comm/bitmap.h

Lines changed: 19 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -147,19 +147,9 @@ struct BitmapRaw final {
147147
raw.bitmap = nullptr;
148148
raw.size = 0;
149149
}
150-
BitmapRaw &operator=(BitmapRaw) = delete;
151150
BitmapRaw &operator=(BitmapRaw &) = delete;
152151
BitmapRaw &operator=(const BitmapRaw &) = delete;
153-
BitmapRaw &operator=(BitmapRaw &&raw) {
154-
if (this != &raw) {
155-
PAX_DELETE_ARRAY(bitmap);
156-
bitmap = raw.bitmap;
157-
size = raw.size;
158-
raw.bitmap = nullptr;
159-
raw.size = 0;
160-
}
161-
return *this;
162-
}
152+
BitmapRaw &operator=(BitmapRaw &&raw) = delete;
163153

164154
~BitmapRaw() = default;
165155

@@ -171,40 +161,46 @@ template <typename T>
171161
class BitmapTpl final {
172162
public:
173163
using BitmapMemoryPolicy = void (*)(BitmapRaw<T> &, uint32);
174-
explicit BitmapTpl(uint32 initial_size = 16,
175-
BitmapMemoryPolicy policy = DefaultBitmapMemoryPolicy) {
164+
explicit BitmapTpl(uint32 initial_size = 16) {
176165
static_assert(sizeof(T) == 1 || sizeof(T) == 2 || sizeof(T) == 4 ||
177166
sizeof(T) == 8);
178167
static_assert(BM_WORD_BITS == (1 << BM_WORD_SHIFTS));
179-
policy_ = policy;
180-
policy(raw_, Max(initial_size, 16));
168+
policy_ = DefaultBitmapMemoryPolicy;
169+
policy_(raw_, Max(initial_size, 16));
181170
}
182-
explicit BitmapTpl(const BitmapRaw<T> &raw, BitmapMemoryPolicy policy) {
171+
explicit BitmapTpl(const BitmapRaw<T> &raw) {
183172
static_assert(sizeof(T) == 1 || sizeof(T) == 2 || sizeof(T) == 4 ||
184173
sizeof(T) == 8);
185174
static_assert(BM_WORD_BITS == (1 << BM_WORD_SHIFTS));
186-
Assert(policy == ReadOnlyRefBitmap || policy == ReadOnlyOwnBitmap);
187-
policy_ = policy;
175+
policy_ = ReadOnlyRefBitmap;
188176
raw_.bitmap = raw.bitmap;
189177
raw_.size = raw.size;
190178
}
191179
BitmapTpl(const BitmapTpl &tpl) = delete;
192180
BitmapTpl(BitmapTpl &&tpl)
193-
: raw_(std::move(tpl.raw_)), policy_(tpl.policy_) {}
181+
: raw_(std::move(tpl.raw_)), policy_(tpl.policy_) {
182+
tpl.raw_.bitmap = nullptr;
183+
tpl.policy_ = ReadOnlyRefBitmap;
184+
}
194185
BitmapTpl(BitmapRaw<T> &&raw)
195-
: raw_(std::move(raw)), policy_(DefaultBitmapMemoryPolicy) {}
186+
: raw_(std::move(raw)), policy_(ReadOnlyRefBitmap) {}
196187
BitmapTpl &operator=(const BitmapTpl &tpl) = delete;
197188
BitmapTpl &operator=(BitmapTpl &&tpl) = delete;
198189
~BitmapTpl() {
199190
// Reference doesn't free the memory
200-
if (policy_ == ReadOnlyRefBitmap) raw_.bitmap = nullptr;
191+
if (policy_ == DefaultBitmapMemoryPolicy)
192+
PAX_DELETE_ARRAY(raw_.bitmap);
193+
raw_.bitmap = nullptr;
201194
}
202195

203196
std::unique_ptr<BitmapTpl> Clone() const {
197+
auto bm = std::make_unique<BitmapTpl>(raw_);
204198
auto p = PAX_NEW_ARRAY<T>(raw_.size);
205199
memcpy(p, raw_.bitmap, sizeof(T) * raw_.size);
206-
BitmapRaw<T> bm_raw(p, raw_.size);
207-
return std::make_unique<BitmapTpl>(std::move(bm_raw));
200+
bm->raw_.bitmap = p;
201+
bm->raw_.size = raw_.size;
202+
bm->policy_ = DefaultBitmapMemoryPolicy;
203+
return bm;
208204
}
209205

210206
inline size_t WordBits() const { return BM_WORD_BITS; }
@@ -272,9 +268,6 @@ class BitmapTpl final {
272268
// raise
273269
CBDB_RAISE(cbdb::CException::kExTypeInvalidMemoryOperation);
274270
}
275-
static void ReadOnlyOwnBitmap(BitmapRaw<T> & /*raw*/, uint32 /*index*/) {
276-
CBDB_RAISE(cbdb::CException::kExTypeInvalidMemoryOperation);
277-
}
278271

279272
static inline size_t RequireWords(size_t nbits) {
280273
return nbits ? ((nbits - 1) >> BM_WORD_SHIFTS) + 1 : 0;

contrib/pax_storage/src/cpp/storage/orc/orc_format_reader.cc

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -909,8 +909,7 @@ std::unique_ptr<PaxColumns> OrcFormatReader::ReadStripe(
909909
reinterpret_cast<uint8 *>(data_buffer->GetAvailableBuffer());
910910

911911
Assert(non_null_stream.kind() == pax::porc::proto::Stream_Kind_PRESENT);
912-
non_null_bitmap = std::make_unique<Bitmap8>(BitmapRaw<uint8>(bm_bytes, bm_nbytes),
913-
BitmapTpl<uint8>::ReadOnlyRefBitmap);
912+
non_null_bitmap = std::make_unique<Bitmap8>(BitmapRaw<uint8>(bm_bytes, bm_nbytes));
914913
data_buffer->Brush(bm_nbytes);
915914
}
916915

contrib/pax_storage/src/cpp/storage/pax.cc

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -615,8 +615,7 @@ void TableDeleter::DeleteWithVisibilityMap(
615615
auto buffer = LoadVisimap(file_system_, file_system_options_,
616616
visibility_map_filename);
617617
auto visibility_file_bitmap =
618-
Bitmap8(BitmapRaw<uint8>(buffer->data(), buffer->size()),
619-
Bitmap8::ReadOnlyOwnBitmap);
618+
Bitmap8(BitmapRaw<uint8>(buffer->data(), buffer->size()));
620619
visi_bitmap =
621620
Bitmap8::Union(&visibility_file_bitmap, delete_visi_bitmap.get());
622621

@@ -664,8 +663,7 @@ void TableDeleter::DeleteWithVisibilityMap(
664663
// Update the stats in pax aux table
665664
// Notice that: PAX won't update the stats in group
666665
UpdateStatsInAuxTable(catalog_update, micro_partition_metadata,
667-
std::make_shared<Bitmap8>(visi_bitmap->Raw(),
668-
Bitmap8::ReadOnlyOwnBitmap),
666+
std::make_shared<Bitmap8>(visi_bitmap->Raw()),
669667
min_max_col_idxs,
670668
cbdb::GetBloomFilterColumnIndexes(rel_),
671669
stats_updater_projection);

contrib/pax_storage/src/cpp/storage/vec/pax_porc_adpater.cc

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -576,8 +576,7 @@ std::pair<size_t, size_t> VecAdapter::AppendPorcFormat(PaxColumns *columns,
576576
vec_buffer->Set(boolean_buffer, align_size);
577577

578578
Bitmap8 vec_bool_bitmap(
579-
BitmapRaw<uint8>((uint8 *)(boolean_buffer), align_size),
580-
BitmapTpl<uint8>::ReadOnlyRefBitmap);
579+
BitmapRaw<uint8>((uint8 *)(boolean_buffer), align_size));
581580

582581
CopyBitPackedBuffer(column, micro_partition_visibility_bitmap_,
583582
group_base_offset_, range_begin, range_lens,

contrib/pax_storage/src/cpp/storage/vec_parallel_pax.cc

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,7 @@ class MicroPartitionInfo : public MicroPartitionInfoProvider {
6060
if (!visimap_name.empty()) {
6161
visimap = pax::LoadVisimap(file_system, nullptr, visimap_name);
6262
BitmapRaw<uint8_t> raw(visimap->data(), visimap->size());
63-
bitmap = std::make_unique<Bitmap8>(raw, BitmapTpl<uint8>::ReadOnlyRefBitmap);
63+
bitmap = std::make_unique<Bitmap8>(raw);
6464
}
6565
return {std::move(visimap), std::move(bitmap)};
6666
}

0 commit comments

Comments
 (0)