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 commented Oct 1, 2019

No description provided.

@TeBoring TeBoring force-pushed the TeBoring:php-optimization branch from fcdc4aa to e9012fc Oct 10, 2019
@TeBoring TeBoring added the kokoro:run label Oct 10, 2019
@TeBoring TeBoring added the kokoro:run label Oct 10, 2019
@penelopezone

This comment has been minimized.

Copy link

penelopezone commented Oct 22, 2019

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

@TeBoring

This comment has been minimized.

Copy link
Contributor Author

TeBoring commented Oct 22, 2019

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
@TeBoring TeBoring requested a review from haberman Oct 22, 2019
Copy link
Contributor

stanley-cheung left a comment

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

Copy link
Contributor

stanley-cheung left a comment

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);

This comment has been minimized.

Copy link
@stanley-cheung

stanley-cheung Oct 29, 2019

Contributor

Is this safe?

This comment has been minimized.

Copy link
@TeBoring

TeBoring Oct 29, 2019

Author Contributor
  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;

This comment has been minimized.

Copy link
@stanley-cheung

stanley-cheung Oct 29, 2019

Contributor

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

This comment has been minimized.

Copy link
@TeBoring

TeBoring Oct 29, 2019

Author Contributor

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);

This comment has been minimized.

Copy link
@stanley-cheung

stanley-cheung Oct 29, 2019

Contributor

What's the difference between TSRMLS_CC and PHP_PROTO_TSRMLS_CC?

This comment has been minimized.

Copy link
@TeBoring

TeBoring Oct 29, 2019

Author Contributor

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,

This comment has been minimized.

Copy link
@stanley-cheung

stanley-cheung Oct 29, 2019

Contributor

what does insure mean here?

This comment has been minimized.

Copy link
@TeBoring

TeBoring Oct 29, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@stanley-cheung

stanley-cheung Oct 29, 2019

Contributor

Yea that should be ensure.

Copy link
Contributor

stanley-cheung left a comment

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

Copy link
Contributor

stanley-cheung left a 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/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;

This comment has been minimized.

Copy link
@stanley-cheung

stanley-cheung Oct 29, 2019

Contributor

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;  

This comment has been minimized.

Copy link
@TeBoring

TeBoring Oct 29, 2019

Author Contributor

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;

This comment has been minimized.

Copy link
@stanley-cheung

stanley-cheung Oct 29, 2019

Contributor

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

This comment has been minimized.

Copy link
@TeBoring

TeBoring Oct 29, 2019

Author Contributor

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;

This comment has been minimized.

Copy link
@stanley-cheung

stanley-cheung Oct 29, 2019

Contributor

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

This comment has been minimized.

Copy link
@TeBoring

TeBoring Oct 29, 2019

Author Contributor

Fixed

@TeBoring TeBoring added the kokoro:run label Oct 29, 2019
@TeBoring TeBoring merged commit c53e5b8 into protocolbuffers:master Oct 29, 2019
56 checks passed
56 checks passed
Bazel Kokoro build successful
Details
Dist artifact installation Kokoro build successful
Details
Linux 32-bit Kokoro build successful
Details
Linux C# Kokoro build successful
Details
Linux C++ Distcheck Kokoro build successful
Details
Linux C++ TC Malloc Kokoro build successful
Details
Linux Golang Kokoro build successful
Details
Linux Java Compatibility Kokoro build successful
Details
Linux Java JDK 7 Kokoro build successful
Details
Linux Java Linkage Monitor Kokoro build successful
Details
Linux Java Oracle 7 Kokoro build successful
Details
Linux JavaScript Kokoro build successful
Details
Linux PHP Kokoro build successful
Details
Linux Python Kokoro build successful
Details
Linux Python 2.7 Kokoro build successful
Details
Linux Python 2.7 C++ Kokoro build successful
Details
Linux Python 3.3 Kokoro build successful
Details
Linux Python 3.3 C++ Kokoro build successful
Details
Linux Python 3.4 Kokoro build successful
Details
Linux Python 3.4 C++ Kokoro build successful
Details
Linux Python 3.5 Kokoro build successful
Details
Linux Python 3.5 C++ Kokoro build successful
Details
Linux Python 3.6 Kokoro build successful
Details
Linux Python 3.6 C++ Kokoro build successful
Details
Linux Python 3.7 Kokoro build successful
Details
Linux Python 3.7 C++ Kokoro build successful
Details
Linux Python C++ Kokoro build successful
Details
Linux Python Compatibility Kokoro build successful
Details
Linux Python Release Kokoro build successful
Details
Linux Ruby 2.3 Kokoro build successful
Details
Linux Ruby 2.4 Kokoro build successful
Details
Linux Ruby 2.5 Kokoro build successful
Details
Linux Ruby 2.6 Kokoro build successful
Details
Linux Ruby Release Kokoro build successful
Details
MacOS C++ Kokoro build successful
Details
MacOS C++ Distcheck Kokoro build successful
Details
MacOS JavaScript Kokoro build successful
Details
MacOS Obj-C CocoaPods Integration Kokoro build successful
Details
MacOS Obj-C OS X Kokoro build successful
Details
MacOS Obj-C iOS Debug Kokoro build successful
Details
MacOS Obj-C iOS Release Kokoro build successful
Details
MacOS PHP5.6 Kokoro build successful
Details
MacOS PHP7.0 Kokoro build successful
Details
MacOS Python Kokoro build successful
Details
MacOS Python C++ Kokoro build successful
Details
MacOS Python Release Kokoro build successful
Details
MacOS Ruby 2.3 Kokoro build successful
Details
MacOS Ruby 2.4 Kokoro build successful
Details
MacOS Ruby 2.5 Kokoro build successful
Details
MacOS Ruby 2.6 Kokoro build successful
Details
MacOS Ruby Release Kokoro build successful
Details
Mergeable Mergeable Run has been Completed!
Details
Windows C# Kokoro build successful
Details
Windows Csharp Release Kokoro build successful
Details
Windows Python Release Kokoro build successful
Details
cla/google All necessary CLAs are signed
@TeBoring TeBoring deleted the TeBoring:php-optimization branch Oct 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.