Skip to content

Commit c11a7c6

Browse files
authored
Remove some extraneous capnp dereferences. (#5196)
[sc-48746] We need to be conservative and follow the CAPNP guidelines on what's best practice when reading messages: only call `reader.getXYZ` once and reuse that value so that the traversal counter is not increased and we don't hit the limit earlier than needed. [Tips and best practices here: https://capnproto.org/cxx.html] --- TYPE: NO_HISTORY DESC: Remove some extraneous capnp dereferences.
1 parent 0915af6 commit c11a7c6

File tree

3 files changed

+36
-33
lines changed

3 files changed

+36
-33
lines changed

tiledb/sm/serialization/array.cc

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -105,8 +105,9 @@ Status metadata_from_capnp(
105105

106106
for (size_t i = 0; i < num_entries; i++) {
107107
auto entry_reader = entries_reader[i];
108-
auto key = std::string{std::string_view{
109-
entry_reader.getKey().cStr(), entry_reader.getKey().size()}};
108+
auto entry_key = entry_reader.getKey();
109+
auto key =
110+
std::string{std::string_view{entry_key.cStr(), entry_key.size()}};
110111
Datatype type = datatype_enum(entry_reader.getType());
111112
uint32_t value_num = entry_reader.getValueNum();
112113

tiledb/sm/serialization/capnp_utils.h

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -498,21 +498,19 @@ std::pair<Status, std::optional<NDRange>> deserialize_non_empty_domain_rv(
498498
(capnp::NonEmptyDomainList::Reader)reader;
499499

500500
NDRange ndRange;
501-
if (r.hasNonEmptyDomains() && r.getNonEmptyDomains().size() > 0) {
502-
auto nonEmptyDomains = r.getNonEmptyDomains();
503-
504-
for (uint32_t i = 0; i < nonEmptyDomains.size(); i++) {
505-
auto nonEmptyDomainObj = nonEmptyDomains[i];
501+
if (r.hasNonEmptyDomains()) {
502+
auto non_empty_domains = r.getNonEmptyDomains();
503+
for (auto non_empty_domain : non_empty_domains) {
506504
// We always store nonEmptyDomain as uint8 lists for the heterogeneous/var
507505
// length version
508-
auto list = nonEmptyDomainObj.getNonEmptyDomain().getUint8();
506+
auto list = non_empty_domain.getNonEmptyDomain().getUint8();
509507
std::vector<uint8_t> vec(list.size());
510508
for (uint32_t index = 0; index < list.size(); index++) {
511509
vec[index] = list[index];
512510
}
513511

514-
if (nonEmptyDomainObj.hasSizes()) {
515-
auto sizes = nonEmptyDomainObj.getSizes();
512+
if (non_empty_domain.hasSizes()) {
513+
auto sizes = non_empty_domain.getSizes();
516514
ndRange.emplace_back(vec.data(), vec.size(), sizes[0]);
517515
} else {
518516
ndRange.emplace_back(vec.data(), vec.size());

tiledb/sm/serialization/fragment_metadata.cc

Lines changed: 27 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -59,54 +59,58 @@ void generic_tile_offsets_from_capnp(
5959
FragmentMetadata::GenericTileOffsets& gt_offsets) {
6060
gt_offsets.rtree_ = gt_reader.getRtree();
6161
if (gt_reader.hasTileOffsets()) {
62-
gt_offsets.tile_offsets_.reserve(gt_reader.getTileOffsets().size());
63-
for (const auto& tile_offset : gt_reader.getTileOffsets()) {
62+
auto tile_offsets = gt_reader.getTileOffsets();
63+
gt_offsets.tile_offsets_.reserve(tile_offsets.size());
64+
for (const auto& tile_offset : tile_offsets) {
6465
gt_offsets.tile_offsets_.emplace_back(tile_offset);
6566
}
6667
}
6768
if (gt_reader.hasTileVarOffsets()) {
68-
gt_offsets.tile_var_offsets_.reserve(gt_reader.getTileVarOffsets().size());
69-
for (const auto& tile_var_offset : gt_reader.getTileVarOffsets()) {
69+
auto tile_var_offsets = gt_reader.getTileVarOffsets();
70+
gt_offsets.tile_var_offsets_.reserve(tile_var_offsets.size());
71+
for (const auto& tile_var_offset : tile_var_offsets) {
7072
gt_offsets.tile_var_offsets_.emplace_back(tile_var_offset);
7173
}
7274
}
7375
if (gt_reader.hasTileVarSizes()) {
74-
gt_offsets.tile_var_sizes_.reserve(gt_reader.getTileVarSizes().size());
75-
for (const auto& tile_var_size : gt_reader.getTileVarSizes()) {
76+
auto tile_var_sizes = gt_reader.getTileVarSizes();
77+
gt_offsets.tile_var_sizes_.reserve(tile_var_sizes.size());
78+
for (const auto& tile_var_size : tile_var_sizes) {
7679
gt_offsets.tile_var_sizes_.emplace_back(tile_var_size);
7780
}
7881
}
7982
if (gt_reader.hasTileValidityOffsets()) {
80-
gt_offsets.tile_validity_offsets_.reserve(
81-
gt_reader.getTileValidityOffsets().size());
82-
for (const auto& tile_validity_offset :
83-
gt_reader.getTileValidityOffsets()) {
83+
auto tile_validity_offsets = gt_reader.getTileValidityOffsets();
84+
gt_offsets.tile_validity_offsets_.reserve(tile_validity_offsets.size());
85+
for (const auto& tile_validity_offset : tile_validity_offsets) {
8486
gt_offsets.tile_validity_offsets_.emplace_back(tile_validity_offset);
8587
}
8688
}
8789
if (gt_reader.hasTileMinOffsets()) {
88-
gt_offsets.tile_min_offsets_.reserve(gt_reader.getTileMinOffsets().size());
89-
for (const auto& tile_min_offset : gt_reader.getTileMinOffsets()) {
90+
auto tile_min_offsets = gt_reader.getTileMinOffsets();
91+
gt_offsets.tile_min_offsets_.reserve(tile_min_offsets.size());
92+
for (const auto& tile_min_offset : tile_min_offsets) {
9093
gt_offsets.tile_min_offsets_.emplace_back(tile_min_offset);
9194
}
9295
}
9396
if (gt_reader.hasTileMaxOffsets()) {
94-
gt_offsets.tile_max_offsets_.reserve(gt_reader.getTileMaxOffsets().size());
95-
for (const auto& tile_max_offset : gt_reader.getTileMaxOffsets()) {
97+
auto tile_max_offsets = gt_reader.getTileMaxOffsets();
98+
gt_offsets.tile_max_offsets_.reserve(tile_max_offsets.size());
99+
for (const auto& tile_max_offset : tile_max_offsets) {
96100
gt_offsets.tile_max_offsets_.emplace_back(tile_max_offset);
97101
}
98102
}
99103
if (gt_reader.hasTileSumOffsets()) {
100-
gt_offsets.tile_sum_offsets_.reserve(gt_reader.getTileSumOffsets().size());
101-
for (const auto& tile_sum_offset : gt_reader.getTileSumOffsets()) {
104+
auto tile_sum_offsets = gt_reader.getTileSumOffsets();
105+
gt_offsets.tile_sum_offsets_.reserve(tile_sum_offsets.size());
106+
for (const auto& tile_sum_offset : tile_sum_offsets) {
102107
gt_offsets.tile_sum_offsets_.emplace_back(tile_sum_offset);
103108
}
104109
}
105110
if (gt_reader.hasTileNullCountOffsets()) {
106-
gt_offsets.tile_null_count_offsets_.reserve(
107-
gt_reader.getTileNullCountOffsets().size());
108-
for (const auto& tile_null_count_offset :
109-
gt_reader.getTileNullCountOffsets()) {
111+
auto tile_null_count_offsets = gt_reader.getTileNullCountOffsets();
112+
gt_offsets.tile_null_count_offsets_.reserve(tile_null_count_offsets.size());
113+
for (const auto& tile_null_count_offset : tile_null_count_offsets) {
110114
gt_offsets.tile_null_count_offsets_.emplace_back(tile_null_count_offset);
111115
}
112116
}
@@ -362,9 +366,9 @@ Status fragment_metadata_from_capnp(
362366

363367
frag_meta->version() = frag_meta_reader.getVersion();
364368
if (frag_meta_reader.hasTimestampRange()) {
365-
frag_meta->timestamp_range() = std::make_pair(
366-
frag_meta_reader.getTimestampRange()[0],
367-
frag_meta_reader.getTimestampRange()[1]);
369+
auto timestamp_range = frag_meta_reader.getTimestampRange();
370+
frag_meta->timestamp_range() =
371+
std::make_pair(timestamp_range[0], timestamp_range[1]);
368372
}
369373
frag_meta->last_tile_cell_num() = frag_meta_reader.getLastTileCellNum();
370374

0 commit comments

Comments
 (0)