Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid too much overhead in layout_init #6716

Merged
merged 10 commits into from Oct 29, 2019

Conversation

TeBoring
Copy link
Contributor

@TeBoring TeBoring commented Oct 1, 2019

No description provided.

@fables-tales
Copy link
Contributor

@TeBoring we validated this is good on our end, can we merge this?

@TeBoring
Copy link
Contributor Author

sure, just some cleanup

@TeBoring TeBoring changed the title Avoid too much overhead in layout_init [WIP] Avoid too much overhead in layout_init Oct 22, 2019
Copy link
Contributor

@stanley-cheung stanley-cheung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't dive into code details. But discussed offline about overall rationale and goals of this PR. Thanks.

Copy link
Contributor

@stanley-cheung stanley-cheung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few comments for now. Will do another round later today.

new_php_string(cached, frame->sink.ptr, frame->sink.len);

stringsink_uninit(&frame->sink);
free(frame);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this safe?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. new_php_string copy the string
  2. 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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this safe? I mean is *closure always castable to stringfields_parseframe_t *?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the difference between TSRMLS_CC and PHP_PROTO_TSRMLS_CC?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what does insure mean here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, if not created, create it.
Should that be ensure?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea that should be ensure.

Copy link
Contributor

@stanley-cheung stanley-cheung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: this is only runtime changes - no protoc code generator changes right?

Copy link
Contributor

@stanley-cheung stanley-cheung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/mergeFromString and serializeToJsonString/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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;  

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;                                                                           
                                ^~     

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;                                                                           
                                ^~ 

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@TeBoring TeBoring merged commit c53e5b8 into protocolbuffers:master Oct 29, 2019
@TeBoring TeBoring deleted the php-optimization branch October 29, 2019 22:39
nlhien pushed a commit to nlhien/protobuf that referenced this pull request Feb 28, 2020
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants