Skip to content

Commit 399d08c

Browse files
pauldreiklemire
authored andcommitted
use unique_ptr in class parsedjson (simdjson#417)
* refactor parsedjson to use unique_ptr instead of owning raw pointer * fix a potential undefined behavior * output only first cpu in /proc/cpuinfo
1 parent 6f79943 commit 399d08c

7 files changed

Lines changed: 49 additions & 115 deletions

File tree

.drone.yml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ steps:
2222
image: gcc:8
2323
environment:
2424
CHECKPERF_REPOSITORY: https://github.com/lemire/simdjson
25-
commands: [ cat /proc/cpuinfo, make checkperf ]
25+
commands: [ sed '/^$/Q' /proc/cpuinfo, make checkperf ]
2626
---
2727
kind: pipeline
2828
name: x64-build

include/simdjson/parsedjson.h

Lines changed: 18 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
#include "simdjson/simdjson.h"
66
#include <cstring>
77
#include <iostream>
8+
#include <memory>
89

910
#define JSON_VALUE_MASK 0xFFFFFFFFFFFFFF
1011

@@ -21,10 +22,14 @@ class ParsedJson {
2122
public:
2223
// create a ParsedJson container with zero capacity, call allocate_capacity to
2324
// allocate memory
24-
ParsedJson();
25-
~ParsedJson();
26-
ParsedJson(ParsedJson &&p);
27-
ParsedJson &operator=(ParsedJson &&o);
25+
ParsedJson()=default;
26+
~ParsedJson()=default;
27+
28+
// this is a move only class
29+
ParsedJson(ParsedJson &&p) = default;
30+
ParsedJson(const ParsedJson &p) = delete;
31+
ParsedJson &operator=(ParsedJson &&o) = default;
32+
ParsedJson &operator=(const ParsedJson &o) = delete;
2833

2934
// if needed, allocate memory so that the object is able to process JSON
3035
// documents having up to len bytes and max_depth "depth"
@@ -77,7 +82,8 @@ class ParsedJson {
7782

7883
really_inline void write_tape_s64(int64_t i) {
7984
write_tape(0, 'l');
80-
tape[current_loc++] = *(reinterpret_cast<uint64_t *>(&i));
85+
std::memcpy(&tape[current_loc], &i, sizeof(i));
86+
++current_loc;
8187
}
8288

8389
really_inline void write_tape_u64(uint64_t i) {
@@ -113,27 +119,22 @@ class ParsedJson {
113119
uint32_t current_loc{0};
114120
uint32_t n_structural_indexes{0};
115121

116-
uint32_t *structural_indexes;
122+
std::unique_ptr<uint32_t[]> structural_indexes;
123+
124+
std::unique_ptr<uint64_t[]> tape;
125+
std::unique_ptr<uint32_t[]> containing_scope_offset;
117126

118-
uint64_t *tape;
119-
uint32_t *containing_scope_offset;
120127
#ifdef SIMDJSON_USE_COMPUTED_GOTO
121-
void **ret_address;
128+
std::unique_ptr<void*[]> ret_address;
122129
#else
123-
char *ret_address;
130+
std::unique_ptr<char[]> ret_address;
124131
#endif
125132

126-
uint8_t *string_buf; // should be at least byte_capacity
133+
std::unique_ptr<uint8_t[]> string_buf;// should be at least byte_capacity
127134
uint8_t *current_string_buf_loc;
128135
bool valid{false};
129136
int error_code{simdjson::UNITIALIZED};
130137

131-
private:
132-
// we don't want the default constructor to be called
133-
ParsedJson(const ParsedJson &p) =
134-
delete; // we don't want the default constructor to be called
135-
// we don't want the assignment to be called
136-
ParsedJson &operator=(const ParsedJson &o) = delete;
137138
};
138139

139140
// dump bits low to high

include/simdjson/parsedjsoniterator.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -64,14 +64,14 @@ template <size_t max_depth> class ParsedJson::BasicIterator {
6464
// within the string: get_string_length determines the true string length.
6565
inline const char *get_string() const {
6666
return reinterpret_cast<const char *>(
67-
pj->string_buf + (current_val & JSON_VALUE_MASK) + sizeof(uint32_t));
67+
pj->string_buf.get() + (current_val & JSON_VALUE_MASK) + sizeof(uint32_t));
6868
}
6969

7070
// return the length of the string in bytes
7171
inline uint32_t get_string_length() const {
7272
uint32_t answer;
7373
memcpy(&answer,
74-
reinterpret_cast<const char *>(pj->string_buf +
74+
reinterpret_cast<const char *>(pj->string_buf.get() +
7575
(current_val & JSON_VALUE_MASK)),
7676
sizeof(uint32_t));
7777
return answer;

scripts/checkperf.sh

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
set -e
44
SCRIPTPATH="$( cd "$(dirname "$0")" ; pwd -P )"
55

6-
if [ -z "$CHECKPERF_REPOSITORY"]; then CHECKPERF_REPOSITORY=.; fi
6+
if [ -z "$CHECKPERF_REPOSITORY" ]; then CHECKPERF_REPOSITORY=.; fi
77

88
# Arguments: perfdiff.sh <branch> <test json files>
99
if [ -z "$1" ]; then reference_branch="master"; else reference_branch=$1; shift; fi

src/generic/stage1_find_marks.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -371,15 +371,15 @@ int find_structural_bits(const uint8_t *buf, size_t len, simdjson::ParsedJson &p
371371
return simdjson::CAPACITY;
372372
}
373373
utf8_checker utf8_checker{};
374-
json_structural_scanner scanner{pj.structural_indexes};
374+
json_structural_scanner scanner{pj.structural_indexes.get()};
375375
scanner.scan<STEP_SIZE>(buf, len, utf8_checker);
376376

377377
simdjson::ErrorValues error = scanner.detect_errors_on_eof();
378378
if (!streaming && unlikely(error != simdjson::SUCCESS)) {
379379
return error;
380380
}
381381

382-
pj.n_structural_indexes = scanner.structural_indexes.tail - pj.structural_indexes;
382+
pj.n_structural_indexes = scanner.structural_indexes.tail - pj.structural_indexes.get();
383383
/* a valid JSON file cannot have zero structural indexes - we should have
384384
* found something */
385385
if (unlikely(pj.n_structural_indexes == 0u)) {

src/generic/stringparsing.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,7 @@ WARN_UNUSED really_inline bool parse_string(UNUSED const uint8_t *buf,
7373
UNUSED size_t len, ParsedJson &pj,
7474
UNUSED const uint32_t depth,
7575
UNUSED uint32_t offset) {
76-
pj.write_tape(pj.current_string_buf_loc - pj.string_buf, '"');
76+
pj.write_tape(pj.current_string_buf_loc - pj.string_buf.get(), '"');
7777
const uint8_t *src = &buf[offset + 1]; /* we know that buf at offset is a " */
7878
uint8_t *dst = pj.current_string_buf_loc + sizeof(uint32_t);
7979
const uint8_t *const start_of_string = dst;

src/parsedjson.cpp

Lines changed: 24 additions & 91 deletions
Original file line numberDiff line numberDiff line change
@@ -2,58 +2,6 @@
22
#include "simdjson/jsonformatutils.h"
33

44
namespace simdjson {
5-
ParsedJson::ParsedJson()
6-
: structural_indexes(nullptr), tape(nullptr),
7-
containing_scope_offset(nullptr), ret_address(nullptr),
8-
string_buf(nullptr), current_string_buf_loc(nullptr) {}
9-
10-
ParsedJson::~ParsedJson() { deallocate(); }
11-
12-
ParsedJson::ParsedJson(ParsedJson &&p)
13-
: byte_capacity(p.byte_capacity), depth_capacity(p.depth_capacity),
14-
tape_capacity(p.tape_capacity), string_capacity(p.string_capacity),
15-
current_loc(p.current_loc), n_structural_indexes(p.n_structural_indexes),
16-
structural_indexes(p.structural_indexes), tape(p.tape),
17-
containing_scope_offset(p.containing_scope_offset),
18-
ret_address(p.ret_address), string_buf(p.string_buf),
19-
current_string_buf_loc(p.current_string_buf_loc), valid(p.valid) {
20-
p.structural_indexes = nullptr;
21-
p.tape = nullptr;
22-
p.containing_scope_offset = nullptr;
23-
p.ret_address = nullptr;
24-
p.string_buf = nullptr;
25-
p.current_string_buf_loc = nullptr;
26-
}
27-
28-
ParsedJson &ParsedJson::operator=(ParsedJson &&p) {
29-
byte_capacity = p.byte_capacity;
30-
p.byte_capacity = 0;
31-
depth_capacity = p.depth_capacity;
32-
p.depth_capacity = 0;
33-
tape_capacity = p.tape_capacity;
34-
p.tape_capacity = 0;
35-
string_capacity = p.string_capacity;
36-
p.string_capacity = 0;
37-
current_loc = p.current_loc;
38-
p.current_loc = 0;
39-
n_structural_indexes = p.n_structural_indexes;
40-
p.n_structural_indexes = 0;
41-
structural_indexes = p.structural_indexes;
42-
p.structural_indexes = nullptr;
43-
tape = p.tape;
44-
p.tape = nullptr;
45-
containing_scope_offset = p.containing_scope_offset;
46-
p.containing_scope_offset = nullptr;
47-
ret_address = p.ret_address;
48-
p.ret_address = nullptr;
49-
string_buf = p.string_buf;
50-
p.string_buf = nullptr;
51-
current_string_buf_loc = p.current_string_buf_loc;
52-
p.current_string_buf_loc = nullptr;
53-
valid = p.valid;
54-
p.valid = false;
55-
return *this;
56-
}
575

586
WARN_UNUSED
597
bool ParsedJson::allocate_capacity(size_t len, size_t max_depth) {
@@ -74,7 +22,8 @@ bool ParsedJson::allocate_capacity(size_t len, size_t max_depth) {
7422
byte_capacity = 0; // will only set it to len after allocations are a success
7523
n_structural_indexes = 0;
7624
uint32_t max_structures = ROUNDUP_N(len, 64) + 2 + 7;
77-
structural_indexes = new (std::nothrow) uint32_t[max_structures];
25+
structural_indexes.reset( new (std::nothrow) uint32_t[max_structures]);
26+
7827
// a pathological input like "[[[[..." would generate len tape elements, so
7928
// need a capacity of at least len + 1, but it is also possible to do
8029
// worse with "[7,7,7,7,6,7,7,7,6,7,7,6,[7,7,7,7,6,7,7,7,6,7,7,6,7,7,7,7,7,7,6"
@@ -84,24 +33,19 @@ bool ParsedJson::allocate_capacity(size_t len, size_t max_depth) {
8433
// a document with only zero-length strings... could have len/3 string
8534
// and we would need len/3 * 5 bytes on the string buffer
8635
size_t local_string_capacity = ROUNDUP_N(5 * len / 3 + 32, 64);
87-
string_buf = new (std::nothrow) uint8_t[local_string_capacity];
88-
tape = new (std::nothrow) uint64_t[local_tape_capacity];
89-
containing_scope_offset = new (std::nothrow) uint32_t[max_depth];
36+
string_buf.reset( new (std::nothrow) uint8_t[local_string_capacity]);
37+
tape.reset(new (std::nothrow) uint64_t[local_tape_capacity]);
38+
containing_scope_offset.reset(new (std::nothrow) uint32_t[max_depth]);
9039
#ifdef SIMDJSON_USE_COMPUTED_GOTO
91-
ret_address = new (std::nothrow) void *[max_depth];
40+
//ret_address = new (std::nothrow) void *[max_depth];
41+
ret_address.reset(new (std::nothrow) void *[max_depth]);
9242
#else
93-
ret_address = new (std::nothrow) char[max_depth];
43+
ret_address.reset(new (std::nothrow) char[max_depth]);
9444
#endif
95-
if ((string_buf == nullptr) || (tape == nullptr) ||
96-
(containing_scope_offset == nullptr) || (ret_address == nullptr) ||
97-
(structural_indexes == nullptr)) {
45+
if (!string_buf || !tape ||
46+
!containing_scope_offset || !ret_address ||
47+
!structural_indexes) {
9848
std::cerr << "Could not allocate memory" << std::endl;
99-
delete[] ret_address;
100-
delete[] containing_scope_offset;
101-
delete[] tape;
102-
delete[] string_buf;
103-
delete[] structural_indexes;
104-
10549
return false;
10650
}
10751
/*
@@ -131,16 +75,16 @@ void ParsedJson::deallocate() {
13175
depth_capacity = 0;
13276
tape_capacity = 0;
13377
string_capacity = 0;
134-
delete[] ret_address;
135-
delete[] containing_scope_offset;
136-
delete[] tape;
137-
delete[] string_buf;
138-
delete[] structural_indexes;
78+
ret_address.reset();
79+
containing_scope_offset.reset();
80+
tape.reset();
81+
string_buf.reset();
82+
structural_indexes.reset();
13983
valid = false;
14084
}
14185

14286
void ParsedJson::init() {
143-
current_string_buf_loc = string_buf;
87+
current_string_buf_loc = string_buf.get();
14488
current_loc = 0;
14589
valid = false;
14690
}
@@ -168,8 +112,8 @@ bool ParsedJson::print_json(std::ostream &os) const {
168112
return false;
169113
}
170114
tape_idx++;
171-
bool *in_object = new bool[depth_capacity];
172-
auto *in_object_idx = new size_t[depth_capacity];
115+
std::unique_ptr<bool[]> in_object(new bool[depth_capacity]);
116+
std::unique_ptr<size_t[]> in_object_idx(new size_t[depth_capacity]);
173117
int depth = 1; // only root at level 0
174118
in_object_idx[depth] = 0;
175119
in_object[depth] = false;
@@ -195,32 +139,26 @@ bool ParsedJson::print_json(std::ostream &os) const {
195139
switch (type) {
196140
case '"': // we have a string
197141
os << '"';
198-
memcpy(&string_length, string_buf + payload, sizeof(uint32_t));
142+
memcpy(&string_length, string_buf.get() + payload, sizeof(uint32_t));
199143
print_with_escapes(
200-
(const unsigned char *)(string_buf + payload + sizeof(uint32_t)),
144+
(const unsigned char *)(string_buf.get() + payload + sizeof(uint32_t)),
201145
os, string_length);
202146
os << '"';
203147
break;
204148
case 'l': // we have a long int
205149
if (tape_idx + 1 >= how_many) {
206-
delete[] in_object;
207-
delete[] in_object_idx;
208150
return false;
209151
}
210152
os << static_cast<int64_t>(tape[++tape_idx]);
211153
break;
212154
case 'u':
213155
if (tape_idx + 1 >= how_many) {
214-
delete[] in_object;
215-
delete[] in_object_idx;
216156
return false;
217157
}
218158
os << tape[++tape_idx];
219159
break;
220160
case 'd': // we have a double
221161
if (tape_idx + 1 >= how_many) {
222-
delete[] in_object;
223-
delete[] in_object_idx;
224162
return false;
225163
}
226164
double answer;
@@ -258,18 +196,12 @@ bool ParsedJson::print_json(std::ostream &os) const {
258196
break;
259197
case 'r': // we start and end with the root node
260198
fprintf(stderr, "should we be hitting the root node?\n");
261-
delete[] in_object;
262-
delete[] in_object_idx;
263199
return false;
264200
default:
265201
fprintf(stderr, "bug %c\n", type);
266-
delete[] in_object;
267-
delete[] in_object_idx;
268202
return false;
269203
}
270204
}
271-
delete[] in_object;
272-
delete[] in_object_idx;
273205
return true;
274206
}
275207

@@ -301,9 +233,10 @@ bool ParsedJson::dump_raw_tape(std::ostream &os) const {
301233
switch (type) {
302234
case '"': // we have a string
303235
os << "string \"";
304-
memcpy(&string_length, string_buf + payload, sizeof(uint32_t));
236+
memcpy(&string_length, string_buf.get() + payload, sizeof(uint32_t));
305237
print_with_escapes(
306-
(const unsigned char *)(string_buf + payload + sizeof(uint32_t)),
238+
(const unsigned char *)(string_buf.get() + payload + sizeof(uint32_t)),
239+
os,
307240
string_length);
308241
os << '"';
309242
os << '\n';

0 commit comments

Comments
 (0)