Avoid too much overhead in layout_init #6716
Conversation
TeBoring
force-pushed
the
php-optimization
branch
from
fcdc4aa to
e9012fc
Compare
|
@TeBoring we validated this is good on our end, can we merge this? |
|
sure, just some cleanup |
TeBoring
changed the title
| new_php_string(cached, frame->sink.ptr, frame->sink.len); | ||
|
|
||
| stringsink_uninit(&frame->sink); | ||
| free(frame); |
There was a problem hiding this comment.
- new_php_string copy the string
- The frame is created in str_handler, which is always paired with str_end_handler
| @@ -371,6 +369,21 @@ static void* str_handler(void *closure, | |||
| } | |||
|
|
|||
| static bool str_end_handler(void *closure, const void *hd) { | |||
| stringfields_parseframe_t* frame = closure; | |||
There was a problem hiding this comment.
Is this safe? I mean is *closure always castable to stringfields_parseframe_t *?
There was a problem hiding this comment.
Yes. str_end_handler is always paired with str_handler.
You can check add_handlers_for_singular_field where we register handlers for different fields.
| TSRMLS_FETCH(); | ||
| zval* map = CACHED_PTR_TO_ZVAL_PTR( | ||
| DEREF(message_data(msg), mapdata->ofs, CACHED_VALUE*)); | ||
| map_field_insure_created(mapdata->fd, cache PHP_PROTO_TSRMLS_CC); |
There was a problem hiding this comment.
What's the difference between TSRMLS_CC and PHP_PROTO_TSRMLS_CC?
There was a problem hiding this comment.
PHP_PROTO_TSRMLS_CC is to support both php 5 and php 7.
Some php internal API differ from php 5 and php7.
In order to unify code, I created PHP_PROTO_TSRMLS_CC.
When it's php 5, PHP_PROTO_TSRMLS_CC is TSRMLS_CC
Otherwise, it's empty
php/ext/google/protobuf/map.c
Outdated
| @@ -243,6 +243,18 @@ map_field_handlers->write_dimension = map_field_write_dimension; | |||
| map_field_handlers->get_gc = map_field_get_gc; | |||
| PHP_PROTO_INIT_CLASS_END | |||
|
|
|||
| void map_field_insure_created(const upb_fielddef *field, | |||
There was a problem hiding this comment.
what does insure mean here?
There was a problem hiding this comment.
I mean, if not created, create it.
Should that be ensure?
There was a problem hiding this comment.
Yea that should be ensure.
There was a problem hiding this comment.
I tested this rather deeply and things are looking great. I tested with
- PHP 5.6
- PHP 7.2
- With ZTS or without.
- Tested with regular fields, repeated fields and maps
- Tested with
serializeToString/mergeFromStringandserializeToJsonString/mergeFromJsonString - Set various breakpoints in these new code and verified and exercised most of these code paths.
| upb_handlers* h, const upb_fielddef* field) { | ||
| void** hd_field = (void**)malloc(sizeof(void*)); | ||
| PHP_PROTO_ASSERT(hd_field != NULL); | ||
| *hd_field = field; |
There was a problem hiding this comment.
Compiler warning:
/tmp/pear/temp/protobuf/encode_decode.c:146:13: warning: assignment discards 'const' qualifier from pointer t\
arget type [-Wdiscarded-qualifiers]
*hd_field = field;
| @@ -221,8 +232,11 @@ static const void *newoneofhandlerdata(upb_handlers *h, | |||
| // this field (such an instance always exists even in an empty message). | |||
| static void *startseq_handler(void* closure, const void* hd) { | |||
| MessageHeader* msg = closure; | |||
| const size_t *ofs = hd; | |||
| return CACHED_PTR_TO_ZVAL_PTR(DEREF(message_data(msg), *ofs, CACHED_VALUE*)); | |||
| const upb_fielddef** field = hd; | |||
There was a problem hiding this comment.
Compiler warning:
/tmp/pear/temp/protobuf/encode_decode.c:235:32: warning: initialization discards 'const' qualifier from point\
er target type [-Wdiscarded-qualifiers]
const upb_fielddef** field = hd;
^~
| @@ -371,6 +369,21 @@ static void* str_handler(void *closure, | |||
| } | |||
|
|
|||
| static bool str_end_handler(void *closure, const void *hd) { | |||
| stringfields_parseframe_t* frame = closure; | |||
| const upb_fielddef **field = hd; | |||
There was a problem hiding this comment.
Compiler warning:
/tmp/pear/temp/protobuf/encode_decode.c:373:32: warning: initialization discards 'const' qualifier from point\
er target type [-Wdiscarded-qualifiers]
const upb_fielddef **field = hd;
^~
* Avoid initializing primitive fields in layout_init * Avoid initializing string/bytes/message fields in layout_init * Lazily create map when needed * Lazily create repeated fields * Change layout_init to only do memcpy * Fix test for php-7.0 * Fix conformance test where default value of string/message map is not encoded * Fix test for zts * Clean up * Fix comments
No description provided.