Skip to content

Commit a7c6198

Browse files
dktappsdktapps
committed
Improved ByteBufferWriter performance by 5-6%
Using zend_string for buffers was convenient and allowed multiple buffers to reference the same buffer, but in practice this was just slowing things down. Writers will now always copy memory when cloned, ensuring that every writer has full ownership of the memory in question. This eliminates the potential for a string to be shared, and we don't have to worry about zend interned strings or any of that jazz.
1 parent fbf325a commit a7c6198

File tree

5 files changed

+70
-68
lines changed

5 files changed

+70
-68
lines changed

‎Serializers.h‎

Lines changed: 30 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@
22
#define SERIALIZERS_H
33

44
extern "C" {
5-
#include "Zend/zend_string.h"
65
#include "Zend/zend_exceptions.h"
76
}
87
#include "classes/DataDecodeException.h"
@@ -184,31 +183,27 @@ static inline bool readSignedVarInt(unsigned char* bytes, size_t used, size_t& o
184183
return true;
185184
}
186185

187-
188-
static inline void extendBuffer(zend_string*& buffer, size_t offset, size_t usedBytes) {
186+
static inline void extendBuffer(unsigned char*& buffer, size_t& length, size_t offset, size_t usedBytes) {
189187
size_t requiredSize = offset + usedBytes;
190-
if (ZSTR_LEN(buffer) < requiredSize) {
191-
size_t doubleSize = ZSTR_LEN(buffer) * 2;
192-
buffer = zend_string_realloc(buffer, doubleSize > requiredSize ? doubleSize : requiredSize, 0);
193-
ZSTR_VAL(buffer)[ZSTR_LEN(buffer)] = '\0'; //make sure null terminator is always set, to stop sprintf reading out-of-bounds
194-
}
195-
else {
196-
buffer = zend_string_separate(buffer, 0);
188+
if (length < requiredSize) {
189+
size_t doubleSize = length * 2;
190+
length = doubleSize > requiredSize ? doubleSize : requiredSize;
191+
buffer = reinterpret_cast<unsigned char*>(erealloc(buffer, length));
197192
}
198193
}
199194

200195
template<typename TValue>
201-
static void writeByte(zend_string*& buffer, size_t& offset, TValue value) {
202-
extendBuffer(buffer, offset, sizeof(TValue));
196+
static void writeByte(unsigned char*& buffer, size_t& length, size_t& offset, TValue value) {
197+
extendBuffer(buffer, length, offset, sizeof(TValue));
203198

204-
ZSTR_VAL(buffer)[offset] = *reinterpret_cast<char*>(&value);
199+
buffer[offset] = *reinterpret_cast<char*>(&value);
205200

206201
offset += sizeof(TValue);
207202
}
208203

209204
template<typename TValue, ByteOrder byteOrder>
210-
static void writeFixedSizeType(zend_string*& buffer, size_t& offset, TValue value) {
211-
extendBuffer(buffer, offset, sizeof(TValue));
205+
static void writeFixedSizeType(unsigned char*& buffer, size_t& length, size_t& offset, TValue value) {
206+
extendBuffer(buffer, length, offset, sizeof(TValue));
212207

213208
Flipper<TValue> flipper;
214209
flipper.value = value;
@@ -217,15 +212,15 @@ static void writeFixedSizeType(zend_string*& buffer, size_t& offset, TValue valu
217212
std::reverse(std::begin(flipper.bytes), std::end(flipper.bytes));
218213
}
219214

220-
memcpy(&ZSTR_VAL(buffer)[offset], flipper.bytes, sizeof(flipper.bytes));
215+
memcpy(&buffer[offset], flipper.bytes, sizeof(flipper.bytes));
221216

222217
offset += sizeof(TValue);
223218
}
224219

225220
template<typename TValue, ByteOrder byteOrder>
226-
static void writeFixedSizeTypeArray(zend_string*& buffer, size_t& offset, std::vector<TValue>& valueArray) {
221+
static void writeFixedSizeTypeArray(unsigned char*& buffer, size_t& length, size_t& offset, std::vector<TValue>& valueArray) {
227222
size_t arraySizeBytes = valueArray.size() * sizeof(TValue);
228-
extendBuffer(buffer, offset, arraySizeBytes);
223+
extendBuffer(buffer, length, offset, arraySizeBytes);
229224

230225
if (byteOrder != ByteOrder::Native) {
231226
Flipper<TValue> flipper;
@@ -237,41 +232,41 @@ static void writeFixedSizeTypeArray(zend_string*& buffer, size_t& offset, std::v
237232
}
238233
}
239234

240-
memcpy(&ZSTR_VAL(buffer)[offset], reinterpret_cast<char*>(valueArray.data()), arraySizeBytes);
235+
memcpy(&buffer[offset], reinterpret_cast<char*>(valueArray.data()), arraySizeBytes);
241236
offset += arraySizeBytes;
242237
}
243238

244239
template<typename TValue>
245-
using writeComplexTypeFunc_t = void (*) (zend_string*& buffer, size_t& offset, TValue value);
240+
using writeComplexTypeFunc_t = void (*) (unsigned char*& buffer, size_t& length, size_t& offset, TValue value);
246241

247242
template<typename TValue, writeComplexTypeFunc_t<TValue> writeNaiveTypeFunc>
248-
static void writeComplexTypeArray(zend_string*& buffer, size_t& offset, std::vector<TValue>& valueArray) {
243+
static void writeComplexTypeArray(unsigned char*& buffer, size_t& length, size_t& offset, std::vector<TValue>& valueArray) {
249244
for (size_t i = 0; i < valueArray.size(); i++) {
250-
writeNaiveTypeFunc(buffer, offset, valueArray[i]);
245+
writeNaiveTypeFunc(buffer, length, offset, valueArray[i]);
251246
}
252247
}
253248

254249
template<typename TValue, ByteOrder byteOrder>
255-
static void writeInt24(zend_string*& buffer, size_t& offset, TValue value) {
250+
static void writeInt24(unsigned char*& buffer, size_t& length, size_t& offset, TValue value) {
256251
const size_t SIZE = 3;
257-
extendBuffer(buffer, offset, SIZE);
252+
extendBuffer(buffer, length, offset, SIZE);
258253

259254
if (byteOrder == ByteOrder::LittleEndian) {
260-
ZSTR_VAL(buffer)[offset] = value & 0xff;
261-
ZSTR_VAL(buffer)[offset + 1] = (value >> 8) & 0xff;
262-
ZSTR_VAL(buffer)[offset + 2] = (value >> 16) & 0xff;
255+
buffer[offset] = value & 0xff;
256+
buffer[offset + 1] = (value >> 8) & 0xff;
257+
buffer[offset + 2] = (value >> 16) & 0xff;
263258
}
264259
else {
265-
ZSTR_VAL(buffer)[offset] = (value >> 16) & 0xff;
266-
ZSTR_VAL(buffer)[offset + 1] = (value >> 8) & 0xff;
267-
ZSTR_VAL(buffer)[offset + 2] = value & 0xff;
260+
buffer[offset] = (value >> 16) & 0xff;
261+
buffer[offset + 1] = (value >> 8) & 0xff;
262+
buffer[offset + 2] = value & 0xff;
268263
}
269264

270265
offset += SIZE;
271266
}
272267

273268
template<typename TValue>
274-
static inline void writeUnsignedVarInt(zend_string*& buffer, size_t& offset, TValue value) {
269+
static inline void writeUnsignedVarInt(unsigned char*& buffer, size_t& length, size_t& offset, TValue value) {
275270
const auto TYPE_BITS = sizeof(TValue) * CHAR_BIT;
276271
char result[VarIntConstants::MAX_BYTES<TYPE_BITS>];
277272

@@ -286,8 +281,8 @@ static inline void writeUnsignedVarInt(zend_string*& buffer, size_t& offset, TVa
286281
result[i] = nextByte;
287282

288283
auto usedBytes = i + 1;
289-
extendBuffer(buffer, offset, usedBytes);
290-
memcpy(&ZSTR_VAL(buffer)[offset], &result[0], usedBytes);
284+
extendBuffer(buffer, length, offset, usedBytes);
285+
memcpy(&buffer[offset], &result[0], usedBytes);
291286
offset += usedBytes;
292287

293288
return;
@@ -301,14 +296,14 @@ static inline void writeUnsignedVarInt(zend_string*& buffer, size_t& offset, TVa
301296
}
302297

303298
template<typename TUnsignedType, typename TSignedType>
304-
static inline void writeSignedVarInt(zend_string*& buffer, size_t& offset, TSignedType value) {
299+
static inline void writeSignedVarInt(unsigned char*& buffer, size_t& length, size_t& offset, TSignedType value) {
305300
TUnsignedType mask = 0;
306301
if (value < 0) {
307302
//we don't know the type of TUnsignedType here, can't use ~0 directly (the compiler will optimise this anyway)
308303
mask = ~mask;
309304
}
310305

311-
writeUnsignedVarInt<TUnsignedType>(buffer, offset, (static_cast<TUnsignedType>(value) << 1) ^ mask);
306+
writeUnsignedVarInt<TUnsignedType>(buffer, length, offset, (static_cast<TUnsignedType>(value) << 1) ^ mask);
312307
}
313308

314309
#endif

‎classes/ByteBufferWriter.cpp‎

Lines changed: 29 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -11,17 +11,25 @@ extern "C" {
1111
static zend_object_handlers byte_buffer_writer_zend_object_handlers;
1212
zend_class_entry* byte_buffer_writer_ce;
1313

14-
static void writer_init_properties(byte_buffer_writer_zend_object* object, zend_string* buffer, size_t used, size_t offset) {
15-
object->writer.buffer = buffer;
16-
zend_string_addref(buffer);
14+
static void writer_init_properties(byte_buffer_writer_zend_object* object, unsigned char* buffer, size_t length, size_t offset) {
15+
object->writer.length = length; //we don't need to copy reserved memory
1716
object->writer.offset = offset;
18-
object->writer.used = used;
17+
object->writer.used = length;
18+
if (length == 0) {
19+
const unsigned int preallocSize = 16;
20+
object->writer.buffer = reinterpret_cast<unsigned char*>(emalloc(preallocSize));
21+
object->writer.length = preallocSize;
22+
} else {
23+
object->writer.buffer = reinterpret_cast<unsigned char*>(emalloc(length));
24+
memcpy(object->writer.buffer, buffer, length);
25+
}
1926
}
2027

28+
2129
static zend_object* writer_new(zend_class_entry* ce) {
2230
auto object = alloc_custom_zend_object<byte_buffer_writer_zend_object>(ce, &byte_buffer_writer_zend_object_handlers);
2331

24-
writer_init_properties(object, zend_empty_string, 0, 0);
32+
writer_init_properties(object, nullptr, 0, 0);
2533

2634
return &object->std;
2735
}
@@ -40,7 +48,7 @@ static zend_object* writer_clone(zend_object* object) {
4048
static void writer_free(zend_object* std) {
4149
auto object = fetch_from_zend_object<byte_buffer_writer_zend_object>(std);
4250

43-
zend_string_release_ex(object->writer.buffer, 0);
51+
efree(object->writer.buffer);
4452
}
4553

4654
static int writer_compare_objects(zval* obj1, zval* obj2) {
@@ -52,8 +60,8 @@ static int writer_compare_objects(zval* obj1, zval* obj2) {
5260
if (
5361
object1->writer.offset == object2->writer.offset &&
5462
object1->writer.used == object2->writer.used &&
55-
zend_string_equals(object1->writer.buffer, object2->writer.buffer)
56-
) {
63+
memcmp(object1->writer.buffer, object2->writer.buffer, object1->writer.used) == 0
64+
) {
5765
return 0;
5866
}
5967
}
@@ -75,22 +83,22 @@ WRITER_METHOD(__construct) {
7583

7684
object = WRITER_THIS();
7785
if (object->writer.buffer) {
78-
zend_string_release_ex(object->writer.buffer, 0);
86+
efree(object->writer.buffer);
7987
}
8088

8189
if (buffer == NULL) {
8290
buffer = zend_empty_string;
8391
}
8492

8593
//write offset is placed at the end, as if the given string was written using writeByteArray()
86-
writer_init_properties(object, buffer, ZSTR_LEN(buffer), ZSTR_LEN(buffer));
94+
writer_init_properties(object, reinterpret_cast<unsigned char*>(ZSTR_VAL(buffer)), ZSTR_LEN(buffer), ZSTR_LEN(buffer));
8795
}
8896

8997
WRITER_METHOD(getData) {
9098
zend_parse_parameters_none_throw();
9199

92100
auto object = WRITER_THIS();
93-
RETURN_STRINGL(ZSTR_VAL(object->writer.buffer), object->writer.used);
101+
RETURN_STRINGL(reinterpret_cast<const char*>(object->writer.buffer), object->writer.used);
94102
}
95103

96104
WRITER_METHOD(writeByteArray) {
@@ -106,8 +114,8 @@ WRITER_METHOD(writeByteArray) {
106114

107115
auto size = ZSTR_LEN(value);
108116

109-
extendBuffer(object->writer.buffer, object->writer.offset, size);
110-
memcpy(ZSTR_VAL(object->writer.buffer) + object->writer.offset, ZSTR_VAL(value), size);
117+
extendBuffer(object->writer.buffer, object->writer.length, object->writer.offset, size);
118+
memcpy(&object->writer.buffer[object->writer.offset], ZSTR_VAL(value), size);
111119
object->writer.offset += size;
112120
if (object->writer.offset > object->writer.used) {
113121
object->writer.used = object->writer.offset;
@@ -147,7 +155,7 @@ WRITER_METHOD(getReservedLength) {
147155
zend_parse_parameters_none_throw();
148156

149157
auto object = WRITER_THIS();
150-
RETURN_LONG(ZSTR_LEN(object->writer.buffer));
158+
RETURN_LONG(object->writer.length); //don't count null terminator
151159
}
152160

153161
WRITER_METHOD(reserve) {
@@ -162,15 +170,16 @@ WRITER_METHOD(reserve) {
162170
return;
163171
}
164172
auto object = WRITER_THIS();
165-
extendBuffer(object->writer.buffer, static_cast<size_t>(zlength), 0);
173+
extendBuffer(object->writer.buffer, object->writer.length, static_cast<size_t>(zlength), 0);
166174
}
167175

168176
WRITER_METHOD(trim) {
169177
zend_parse_parameters_none_throw();
170178

171179
auto object = WRITER_THIS();
172-
if (ZSTR_LEN(object->writer.buffer) > object->writer.used) {
173-
object->writer.buffer = zend_string_truncate(object->writer.buffer, object->writer.used, 0);
180+
if (object->writer.length > object->writer.used) {
181+
object->writer.buffer = reinterpret_cast<unsigned char*>(erealloc(object->writer.buffer, object->writer.used));
182+
object->writer.length = object->writer.used;
174183
}
175184
}
176185

@@ -189,7 +198,7 @@ WRITER_METHOD(__serialize) {
189198
array_init(return_value);
190199

191200
//don't return the writer buffer directly - it may have uninitialized reserved memory
192-
add_assoc_stringl(return_value, "buffer", ZSTR_VAL(object->writer.buffer), object->writer.used);
201+
add_assoc_stringl(return_value, "buffer", reinterpret_cast<char*>(object->writer.buffer), object->writer.used);
193202
add_assoc_long(return_value, "offset", object->writer.offset);
194203
}
195204

@@ -225,7 +234,7 @@ WRITER_METHOD(__unserialize) {
225234

226235
auto object = WRITER_THIS();
227236

228-
writer_init_properties(object, Z_STR_P(buffer), Z_STRLEN_P(buffer), Z_LVAL_P(offset));
237+
writer_init_properties(object, reinterpret_cast<unsigned char*>(Z_STRVAL_P(buffer)), Z_STRLEN_P(buffer), Z_LVAL_P(offset));
229238
}
230239

231240
WRITER_METHOD(__debugInfo) {
@@ -235,7 +244,7 @@ WRITER_METHOD(__debugInfo) {
235244
array_init(return_value);
236245

237246
//don't return the writer buffer directly - it may have uninitialized reserved memory
238-
add_assoc_stringl(return_value, "buffer", ZSTR_VAL(object->writer.buffer), object->writer.used);
247+
add_assoc_stringl(return_value, "buffer", reinterpret_cast<char*>(object->writer.buffer), object->writer.used);
239248
add_assoc_long(return_value, "offset", object->writer.offset);
240249
}
241250

‎classes/ByteBufferWriter.h‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,8 @@ extern "C" {
77
#include "../ZendUtil.h"
88

99
typedef struct _byte_buffer_writer_t {
10-
zend_string* buffer;
10+
unsigned char* buffer;
11+
size_t length;
1112
size_t offset;
1213
size_t used;
1314
} byte_buffer_writer_t;

‎classes/Types.cpp‎

Lines changed: 9 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -125,11 +125,11 @@ bool parseWriteTypeParams(zend_execute_data* execute_data, byte_buffer_writer_ze
125125
}
126126

127127
template<typename TValue>
128-
using writeTypeFunc_t = void (*)(zend_string*& buffer, size_t& offset, TValue value);
128+
using writeTypeFunc_t = void (*)(unsigned char*& buffer, size_t& length, size_t& offset, TValue value);
129129

130130
template<typename TValue, writeTypeFunc_t<TValue> writeTypeFunc>
131131
inline void writeTypeCommon(INTERNAL_FUNCTION_PARAMETERS, byte_buffer_writer_t& writer, TValue value) {
132-
writeTypeFunc(writer.buffer, writer.offset, value);
132+
writeTypeFunc(writer.buffer, writer.length, writer.offset, value);
133133
if (writer.offset > writer.used) {
134134
writer.used = writer.offset;
135135
}
@@ -178,22 +178,21 @@ template<typename TValue, writeTypeFunc_t<TValue> writeTypeFunc, typename TZendV
178178
void ZEND_FASTCALL zif_packType(INTERNAL_FUNCTION_PARAMETERS) {
179179
TValue value;
180180
byte_buffer_writer_t writer = { 0 };
181+
unsigned char stackBuffer[16]; //big enough for all currently supported types - it'd be good if the serializers told us how big the type is, but that's a problem for another time
181182

182183
if (!parsePackTypeParams(execute_data, value, std::type_identity<TZendValue>{})) {
183184
return;
184185
}
185186

186-
writer.buffer = zend_empty_string;
187+
writer.buffer = stackBuffer;
188+
writer.length = sizeof(stackBuffer);
187189

188190
writeTypeCommon<TValue, writeTypeFunc>(INTERNAL_FUNCTION_PARAM_PASSTHRU, writer, value);
189191

190-
if (ZSTR_LEN(writer.buffer) != writer.used) {
191-
ZEND_ASSERT(0); //we don't expect unused bytes from oneshot packing
192-
193-
RETVAL_STRINGL(ZSTR_VAL(writer.buffer), writer.used);
194-
zend_string_release_ex(writer.buffer, 0);
195-
} else {
196-
RETURN_STR(writer.buffer);
192+
RETVAL_STRINGL(reinterpret_cast<char*>(writer.buffer), writer.used);
193+
if (writer.buffer != stackBuffer) {
194+
printf("stack buffer wasn't big enough :(\n");
195+
efree(writer.buffer);
197196
}
198197
}
199198

‎tests/writer/reserve.phpt‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@ use pmmp\encoding\ByteBufferWriter;
77
use pmmp\encoding\Byte;
88

99
$buffer = new ByteBufferWriter("");
10-
var_dump($buffer->getReservedLength()); //none
1110

1211
$buffer->reserve(40);
1312
var_dump($buffer->getReservedLength()); //40
@@ -28,7 +27,6 @@ try{
2827
}
2928
?>
3029
--EXPECT--
31-
int(0)
3230
int(40)
3331
string(0) ""
3432
int(40)

0 commit comments

Comments
 (0)