Skip to content

Commit 2d09248

Browse files
jviottijviotti
authored
Unify Context .has() and .offset() into .find() (#848)
Signed-off-by: Juan Cruz Viotti <jv@jviotti.com>
1 parent d27ae67 commit 2d09248

File tree

4 files changed

+104
-87
lines changed

4 files changed

+104
-87
lines changed

‎src/runtime/encoder_any.cc‎

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -110,26 +110,26 @@ auto Encoder::ANY_PACKED_TYPE_TAG_BYTE_PREFIX(
110110
} else if (document.is_string()) {
111111
const sourcemeta::jsontoolkit::JSON::String value{document.to_string()};
112112
const auto size{document.byte_size()};
113-
const bool is_shared{this->context_.has(value, Context::Type::Standalone)};
113+
const auto shared{this->context_.find(value, Context::Type::Standalone)};
114114
if (size < uint_max<5>) {
115-
const std::uint8_t type{is_shared ? TYPE_SHARED_STRING : TYPE_STRING};
115+
const std::uint8_t type{shared.has_value() ? TYPE_SHARED_STRING
116+
: TYPE_STRING};
116117
this->put_byte(
117118
static_cast<std::uint8_t>(type | ((size + 1) << type_size)));
118-
if (is_shared) {
119-
this->put_varint(
120-
this->position() -
121-
this->context_.offset(value, Context::Type::Standalone));
119+
if (shared.has_value()) {
120+
this->put_varint(this->position() - shared.value());
122121
} else {
123122
this->context_.record(value, this->position(),
124123
Context::Type::Standalone);
125124
this->put_string_utf8(value, size);
126125
}
127-
} else if (size >= uint_max<5> && size < uint_max<5> * 2 && !is_shared) {
126+
} else if (size >= uint_max<5> && size < uint_max<5> * 2 &&
127+
!shared.has_value()) {
128128
this->put_byte(static_cast<std::uint8_t>(
129129
TYPE_LONG_STRING | ((size - uint_max<5>) << type_size)));
130130
this->put_string_utf8(value, size);
131131
} else if (size >= 2 << (SUBTYPE_LONG_STRING_BASE_EXPONENT_7 - 1) &&
132-
!is_shared) {
132+
!shared.has_value()) {
133133
const std::uint8_t exponent{closest_smallest_exponent(
134134
size, 2, SUBTYPE_LONG_STRING_BASE_EXPONENT_7,
135135
SUBTYPE_LONG_STRING_BASE_EXPONENT_10)};
@@ -141,7 +141,7 @@ auto Encoder::ANY_PACKED_TYPE_TAG_BYTE_PREFIX(
141141
// Exploit the fact that a shared string always starts
142142
// with an impossible length marker (0) to avoid having
143143
// to encode an additional tag
144-
if (!is_shared) {
144+
if (!shared.has_value()) {
145145
this->put_byte(TYPE_STRING);
146146
}
147147

‎src/runtime/encoder_string.cc‎

Lines changed: 16 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -21,20 +21,19 @@ auto Encoder::FLOOR_VARINT_PREFIX_UTF8_STRING_SHARED(
2121
const sourcemeta::jsontoolkit::JSON::String value{document.to_string()};
2222
const auto size{value.size()};
2323
assert(document.size() == size);
24-
const bool is_shared{this->context_.has(value, Context::Type::Standalone)};
24+
const auto shared{this->context_.find(value, Context::Type::Standalone)};
2525

2626
// (1) Write 0x00 if shared, else do nothing
27-
if (is_shared) {
27+
if (shared.has_value()) {
2828
this->put_byte(0);
2929
}
3030

3131
// (2) Write length of the string + 1 (so it will never be zero)
3232
this->put_varint(size - options.minimum + 1);
3333

3434
// (3) Write relative offset if shared, else write plain string
35-
if (is_shared) {
36-
this->put_varint(this->position() -
37-
this->context_.offset(value, Context::Type::Standalone));
35+
if (shared.has_value()) {
36+
this->put_varint(this->position() - shared.value());
3837
} else {
3938
this->context_.record(value, this->position(), Context::Type::Standalone);
4039
this->put_string_utf8(value, size);
@@ -49,20 +48,19 @@ auto Encoder::ROOF_VARINT_PREFIX_UTF8_STRING_SHARED(
4948
const auto size{value.size()};
5049
assert(document.size() == size);
5150
assert(size <= options.maximum);
52-
const bool is_shared{this->context_.has(value, Context::Type::Standalone)};
51+
const auto shared{this->context_.find(value, Context::Type::Standalone)};
5352

5453
// (1) Write 0x00 if shared, else do nothing
55-
if (is_shared) {
54+
if (shared.has_value()) {
5655
this->put_byte(0);
5756
}
5857

5958
// (2) Write length of the string + 1 (so it will never be zero)
6059
this->put_varint(options.maximum - size + 1);
6160

6261
// (3) Write relative offset if shared, else write plain string
63-
if (is_shared) {
64-
this->put_varint(this->position() -
65-
this->context_.offset(value, Context::Type::Standalone));
62+
if (shared.has_value()) {
63+
this->put_varint(this->position() - shared.value());
6664
} else {
6765
this->context_.record(value, this->position(), Context::Type::Standalone);
6866
this->put_string_utf8(value, size);
@@ -79,20 +77,19 @@ auto Encoder::BOUNDED_8BIT_PREFIX_UTF8_STRING_SHARED(
7977
assert(options.minimum <= options.maximum);
8078
assert(is_byte(options.maximum - options.minimum + 1));
8179
assert(is_within(size, options.minimum, options.maximum));
82-
const bool is_shared{this->context_.has(value, Context::Type::Standalone)};
80+
const auto shared{this->context_.find(value, Context::Type::Standalone)};
8381

8482
// (1) Write 0x00 if shared, else do nothing
85-
if (is_shared) {
83+
if (shared.has_value()) {
8684
this->put_byte(0);
8785
}
8886

8987
// (2) Write length of the string + 1 (so it will never be zero)
9088
this->put_byte(static_cast<std::uint8_t>(size - options.minimum + 1));
9189

9290
// (3) Write relative offset if shared, else write plain string
93-
if (is_shared) {
94-
this->put_varint(this->position() -
95-
this->context_.offset(value, Context::Type::Standalone));
91+
if (shared.has_value()) {
92+
this->put_varint(this->position() - shared.value());
9693
} else {
9794
this->context_.record(value, this->position(), Context::Type::Standalone);
9895
this->put_string_utf8(value, size);
@@ -129,12 +126,12 @@ auto Encoder::PREFIX_VARINT_LENGTH_STRING_SHARED(
129126
assert(document.is_string());
130127
const sourcemeta::jsontoolkit::JSON::String value{document.to_string()};
131128

132-
if (this->context_.has(value, Context::Type::PrefixLengthVarintPlusOne)) {
129+
const auto shared{
130+
this->context_.find(value, Context::Type::PrefixLengthVarintPlusOne)};
131+
if (shared.has_value()) {
133132
const auto new_offset{this->position()};
134133
this->put_byte(0);
135-
this->put_varint(
136-
this->position() -
137-
this->context_.offset(value, Context::Type::PrefixLengthVarintPlusOne));
134+
this->put_varint(this->position() - shared.value());
138135
// Bump the context cache for locality purposes
139136
this->context_.record(value, new_offset,
140137
Context::Type::PrefixLengthVarintPlusOne);

‎src/runtime/include/sourcemeta/jsonbinpack/runtime_encoder_context.h‎

Lines changed: 14 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -6,11 +6,11 @@
66

77
#include <sourcemeta/jsontoolkit/json.h>
88

9-
#include <cassert> // assert
10-
#include <iterator> // std::cbegin, std::cend
11-
#include <map> // std::map
12-
#include <stdexcept> // std::logic_error
13-
#include <utility> // std::pair, std::make_pair
9+
#include <cassert> // assert
10+
#include <iterator> // std::cbegin, std::cend
11+
#include <map> // std::map
12+
#include <optional> // std::optional, std::nullopt
13+
#include <utility> // std::pair, std::make_pair
1414

1515
// Encoding a shared string has some overhead, such as the
1616
// shared string marker + the offset, so its not worth
@@ -47,7 +47,8 @@ class SOURCEMETA_JSONBINPACK_RUNTIME_EXPORT Context {
4747

4848
// If the string already exists, we want to
4949
// bump the offset for locality purposes.
50-
if (this->has(value, type)) {
50+
const auto maybe_entry{this->find(value, type)};
51+
if (maybe_entry.has_value()) {
5152
const auto key{std::make_pair(value, type)};
5253
const auto previous_offset{this->strings[key]};
5354
if (offset > previous_offset) {
@@ -79,16 +80,14 @@ class SOURCEMETA_JSONBINPACK_RUNTIME_EXPORT Context {
7980
this->offsets.erase(iterator);
8081
}
8182

82-
auto has(const sourcemeta::jsontoolkit::JSON::String &value,
83-
const Type type) const -> bool {
84-
return this->strings.contains(std::make_pair(value, type));
85-
}
83+
auto find(const sourcemeta::jsontoolkit::JSON::String &value,
84+
const Type type) const -> std::optional<std::uint64_t> {
85+
const auto result{this->strings.find(std::make_pair(value, type))};
86+
if (result == this->strings.cend()) {
87+
return std::nullopt;
88+
}
8689

87-
auto offset(const sourcemeta::jsontoolkit::JSON::String &value,
88-
const Type type) const -> std::uint64_t {
89-
// This method assumes the value indeed exists for performance reasons
90-
assert(this->has(value, type));
91-
return this->strings.at(std::make_pair(value, type));
90+
return result->second;
9291
}
9392

9493
private:

‎test/runtime/encode_context_test.cc‎

Lines changed: 65 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -9,48 +9,59 @@
99
TEST(JSONBinPack_Encoder, context_record_string) {
1010
sourcemeta::jsonbinpack::Context context;
1111
using ContextType = sourcemeta::jsonbinpack::Context::Type;
12-
EXPECT_FALSE(context.has("foo", ContextType::Standalone));
12+
const auto result_1{context.find("foo", ContextType::Standalone)};
13+
EXPECT_FALSE(result_1.has_value());
1314
context.record("foo", 2, ContextType::Standalone);
14-
EXPECT_TRUE(context.has("foo", ContextType::Standalone));
15-
EXPECT_EQ(context.offset("foo", ContextType::Standalone), 2);
15+
const auto result_2{context.find("foo", ContextType::Standalone)};
16+
EXPECT_TRUE(result_2.has_value());
17+
EXPECT_EQ(result_2.value(), 2);
1618
}
1719

1820
TEST(JSONBinPack_Encoder, context_record_string_too_short) {
1921
sourcemeta::jsonbinpack::Context context;
2022
using ContextType = sourcemeta::jsonbinpack::Context::Type;
21-
EXPECT_FALSE(context.has("fo", ContextType::Standalone));
23+
const auto result_1{context.find("fo", ContextType::Standalone)};
24+
EXPECT_FALSE(result_1.has_value());
2225
context.record("fo", 2, ContextType::Standalone);
23-
EXPECT_FALSE(context.has("fo", ContextType::Standalone));
26+
const auto result_2{context.find("fo", ContextType::Standalone)};
27+
EXPECT_FALSE(result_2.has_value());
2428
}
2529

2630
TEST(JSONBinPack_Encoder, context_record_string_empty) {
2731
sourcemeta::jsonbinpack::Context context;
2832
using ContextType = sourcemeta::jsonbinpack::Context::Type;
29-
EXPECT_FALSE(context.has("", ContextType::Standalone));
33+
const auto result_1{context.find("fo", ContextType::Standalone)};
34+
EXPECT_FALSE(result_1.has_value());
3035
context.record("", 2, ContextType::Standalone);
31-
EXPECT_FALSE(context.has("", ContextType::Standalone));
36+
const auto result_2{context.find("", ContextType::Standalone)};
37+
EXPECT_FALSE(result_2.has_value());
3238
}
3339

3440
TEST(JSONBinPack_Encoder, context_has_on_unknown_string) {
3541
sourcemeta::jsonbinpack::Context context;
3642
using ContextType = sourcemeta::jsonbinpack::Context::Type;
37-
EXPECT_FALSE(context.has("foobarbaz", ContextType::Standalone));
43+
const auto result{context.find("foobarbaz", ContextType::Standalone)};
44+
EXPECT_FALSE(result.has_value());
3845
}
3946

4047
TEST(JSONBinPack_Encoder, context_increase_offset) {
4148
sourcemeta::jsonbinpack::Context context;
4249
using ContextType = sourcemeta::jsonbinpack::Context::Type;
4350
context.record("foo", 2, ContextType::Standalone);
4451
context.record("foo", 4, ContextType::Standalone);
45-
EXPECT_EQ(context.offset("foo", ContextType::Standalone), 4);
52+
const auto result{context.find("foo", ContextType::Standalone)};
53+
EXPECT_TRUE(result.has_value());
54+
EXPECT_EQ(result.value(), 4);
4655
}
4756

4857
TEST(JSONBinPack_Encoder, context_do_not_decrease_offset) {
4958
sourcemeta::jsonbinpack::Context context;
5059
using ContextType = sourcemeta::jsonbinpack::Context::Type;
5160
context.record("foo", 4, ContextType::Standalone);
5261
context.record("foo", 2, ContextType::Standalone);
53-
EXPECT_EQ(context.offset("foo", ContextType::Standalone), 4);
62+
const auto result{context.find("foo", ContextType::Standalone)};
63+
EXPECT_TRUE(result.has_value());
64+
EXPECT_EQ(result.value(), 4);
5465
}
5566

5667
TEST(JSONBinPack_Encoder, context_not_record_too_big) {
@@ -60,7 +71,8 @@ TEST(JSONBinPack_Encoder, context_not_record_too_big) {
6071
sourcemeta::jsonbinpack::Context context;
6172
using ContextType = sourcemeta::jsonbinpack::Context::Type;
6273
context.record(too_big, 1, ContextType::Standalone);
63-
EXPECT_FALSE(context.has(too_big, ContextType::Standalone));
74+
const auto result{context.find(too_big, ContextType::Standalone)};
75+
EXPECT_FALSE(result.has_value());
6476
}
6577

6678
TEST(JSONBinPack_Encoder, context_remove_oldest) {
@@ -70,27 +82,31 @@ TEST(JSONBinPack_Encoder, context_remove_oldest) {
7082
context.record("bar", 3, ContextType::Standalone);
7183
context.record("baz", 7, ContextType::PrefixLengthVarintPlusOne);
7284

73-
EXPECT_TRUE(context.has("foo", ContextType::Standalone));
74-
EXPECT_TRUE(context.has("bar", ContextType::Standalone));
75-
EXPECT_TRUE(context.has("baz", ContextType::PrefixLengthVarintPlusOne));
85+
EXPECT_TRUE(context.find("foo", ContextType::Standalone).has_value());
86+
EXPECT_TRUE(context.find("bar", ContextType::Standalone).has_value());
87+
EXPECT_TRUE(
88+
context.find("baz", ContextType::PrefixLengthVarintPlusOne).has_value());
7689

7790
context.remove_oldest();
7891

79-
EXPECT_TRUE(context.has("foo", ContextType::Standalone));
80-
EXPECT_FALSE(context.has("bar", ContextType::Standalone));
81-
EXPECT_TRUE(context.has("baz", ContextType::PrefixLengthVarintPlusOne));
92+
EXPECT_TRUE(context.find("foo", ContextType::Standalone).has_value());
93+
EXPECT_FALSE(context.find("bar", ContextType::Standalone).has_value());
94+
EXPECT_TRUE(
95+
context.find("baz", ContextType::PrefixLengthVarintPlusOne).has_value());
8296

8397
context.remove_oldest();
8498

85-
EXPECT_TRUE(context.has("foo", ContextType::Standalone));
86-
EXPECT_FALSE(context.has("bar", ContextType::Standalone));
87-
EXPECT_FALSE(context.has("baz", ContextType::PrefixLengthVarintPlusOne));
99+
EXPECT_TRUE(context.find("foo", ContextType::Standalone).has_value());
100+
EXPECT_FALSE(context.find("bar", ContextType::Standalone).has_value());
101+
EXPECT_FALSE(
102+
context.find("baz", ContextType::PrefixLengthVarintPlusOne).has_value());
88103

89104
context.remove_oldest();
90105

91-
EXPECT_FALSE(context.has("foo", ContextType::Standalone));
92-
EXPECT_FALSE(context.has("bar", ContextType::Standalone));
93-
EXPECT_FALSE(context.has("baz", ContextType::PrefixLengthVarintPlusOne));
106+
EXPECT_FALSE(context.find("foo", ContextType::Standalone).has_value());
107+
EXPECT_FALSE(context.find("bar", ContextType::Standalone).has_value());
108+
EXPECT_FALSE(
109+
context.find("baz", ContextType::PrefixLengthVarintPlusOne).has_value());
94110
}
95111

96112
TEST(JSONBinPack_Encoder, context_is_a_circular_buffer) {
@@ -117,27 +133,27 @@ TEST(JSONBinPack_Encoder, context_is_a_circular_buffer) {
117133
context.record(string_3, length * 2, ContextType::Standalone);
118134
context.record(string_4, length * 3, ContextType::Standalone);
119135

120-
EXPECT_TRUE(context.has(string_1, ContextType::Standalone));
121-
EXPECT_TRUE(context.has(string_2, ContextType::Standalone));
122-
EXPECT_TRUE(context.has(string_3, ContextType::Standalone));
123-
EXPECT_TRUE(context.has(string_4, ContextType::Standalone));
136+
EXPECT_TRUE(context.find(string_1, ContextType::Standalone).has_value());
137+
EXPECT_TRUE(context.find(string_2, ContextType::Standalone).has_value());
138+
EXPECT_TRUE(context.find(string_3, ContextType::Standalone).has_value());
139+
EXPECT_TRUE(context.find(string_4, ContextType::Standalone).has_value());
124140

125141
context.record(string_5, length * 4, ContextType::Standalone);
126142

127-
EXPECT_FALSE(context.has(string_1, ContextType::Standalone));
128-
EXPECT_TRUE(context.has(string_2, ContextType::Standalone));
129-
EXPECT_TRUE(context.has(string_3, ContextType::Standalone));
130-
EXPECT_TRUE(context.has(string_4, ContextType::Standalone));
131-
EXPECT_TRUE(context.has(string_5, ContextType::Standalone));
143+
EXPECT_FALSE(context.find(string_1, ContextType::Standalone).has_value());
144+
EXPECT_TRUE(context.find(string_2, ContextType::Standalone).has_value());
145+
EXPECT_TRUE(context.find(string_3, ContextType::Standalone).has_value());
146+
EXPECT_TRUE(context.find(string_4, ContextType::Standalone).has_value());
147+
EXPECT_TRUE(context.find(string_5, ContextType::Standalone).has_value());
132148

133149
context.record(string_6, length * 5, ContextType::Standalone);
134150

135-
EXPECT_FALSE(context.has(string_1, ContextType::Standalone));
136-
EXPECT_FALSE(context.has(string_2, ContextType::Standalone));
137-
EXPECT_TRUE(context.has(string_3, ContextType::Standalone));
138-
EXPECT_TRUE(context.has(string_4, ContextType::Standalone));
139-
EXPECT_TRUE(context.has(string_5, ContextType::Standalone));
140-
EXPECT_TRUE(context.has(string_6, ContextType::Standalone));
151+
EXPECT_FALSE(context.find(string_1, ContextType::Standalone).has_value());
152+
EXPECT_FALSE(context.find(string_2, ContextType::Standalone).has_value());
153+
EXPECT_TRUE(context.find(string_3, ContextType::Standalone).has_value());
154+
EXPECT_TRUE(context.find(string_4, ContextType::Standalone).has_value());
155+
EXPECT_TRUE(context.find(string_5, ContextType::Standalone).has_value());
156+
EXPECT_TRUE(context.find(string_6, ContextType::Standalone).has_value());
141157
}
142158

143159
TEST(JSONBinPack_Encoder, context_same_string_different_type) {
@@ -146,17 +162,22 @@ TEST(JSONBinPack_Encoder, context_same_string_different_type) {
146162
context.record("foo", 10, ContextType::Standalone);
147163
context.record("foo", 20, ContextType::PrefixLengthVarintPlusOne);
148164

149-
EXPECT_TRUE(context.has("foo", ContextType::Standalone));
150-
EXPECT_TRUE(context.has("foo", ContextType::PrefixLengthVarintPlusOne));
165+
const auto result_1{context.find("foo", ContextType::Standalone)};
166+
const auto result_2{
167+
context.find("foo", ContextType::PrefixLengthVarintPlusOne)};
151168

152-
EXPECT_EQ(context.offset("foo", ContextType::Standalone), 10);
153-
EXPECT_EQ(context.offset("foo", ContextType::PrefixLengthVarintPlusOne), 20);
169+
EXPECT_TRUE(result_1.has_value());
170+
EXPECT_TRUE(result_2.has_value());
171+
172+
EXPECT_EQ(result_1.value(), 10);
173+
EXPECT_EQ(result_2.value(), 20);
154174
}
155175

156176
TEST(JSONBinPack_Encoder, context_no_fallback_type) {
157177
sourcemeta::jsonbinpack::Context context;
158178
using ContextType = sourcemeta::jsonbinpack::Context::Type;
159179
context.record("foo", 10, ContextType::Standalone);
160-
EXPECT_TRUE(context.has("foo", ContextType::Standalone));
161-
EXPECT_FALSE(context.has("foo", ContextType::PrefixLengthVarintPlusOne));
180+
EXPECT_TRUE(context.find("foo", ContextType::Standalone).has_value());
181+
EXPECT_FALSE(
182+
context.find("foo", ContextType::PrefixLengthVarintPlusOne).has_value());
162183
}

0 commit comments

Comments
 (0)