php: Added nanosecond support for Timestamp #3972

lhecker · 2017-11-29T08:54:50Z

This PR adds support for the nanoseconds field of the wellknown Timestamp type for PHP 5.6 and later.
The accuracy will be in the order of microseconds though, as that's the highest resolution PHP supports.

I hope this PR fullfills all necessary preconditions to be merged, but if it doesn't (e.g. because the Timestamp class is generated automatically), I'd be glad to help get this feature implemented whatever is necessary for that to happen. 🙂

grpc-kokoro · 2017-11-29T08:54:51Z

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

grpc-kokoro · 2017-11-29T08:54:51Z

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

lhecker · 2018-03-06T08:52:52Z

@TeBoring Could you please take a look at this ticket or assign it to someone? 🙂
I believe my code is correct and only fails due to regression tests – this PR is an improvement though and would thus not be compatible with previous tests.

TeBoring · 2018-06-25T21:43:05Z

Could you check the c extension also works? https://github.com/google/protobuf/tree/master/php/ext/google/protobuf

lhecker · 2018-06-27T13:21:28Z

@TeBoring You're right: The fromDateTime/toDateTime functions are actually also implemented in the native PHP extension. Didn't realize that. 😐

Can you run kokoro on this again?

I tried my best converting my PHP logic into C and also improving the existing C code a little bit (e.g. by not relying on the flaky gmtime() function, or by employing the ARRAY_SIZE macro).

But if possible someone more experienced in PHP extensions has to look over this PR as I was basically only poking around in the dark here (I really don't have any clue how to write PHP extensions).

TeBoring · 2018-07-10T00:20:32Z

If you run:

export ZEND_DONT_UNLOAD_MODULES=1
export USE_ZEND_ALLOC=0
valgrind --leak-check=yes php -dextension=../ext/google/protobuf/modules/protobuf.so memory_leak_test.php

You will get:

Warning: assert(): assert(0 == $timestamp->getNanos()) failed in /home/teboring/protobuf-timestamp/php/tests/memory_leak_test.php on line 155
==14741== 
==14741== HEAP SUMMARY:
==14741==     in use at exit: 77,591 bytes in 42 blocks
==14741==   total heap usage: 42,875 allocs, 42,833 frees, 9,228,805 bytes allocated
==14741== 
==14741== 32 bytes in 1 blocks are definitely lost in loss record 15 of 42
==14741==    at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==14741==    by 0x7039E8: __zend_malloc (zend_alloc.c:2820)
==14741==    by 0x8FEF208: zend_string_alloc (zend_string.h:122)
==14741==    by 0x8FEF208: zend_string_init (zend_string.h:158)
==14741==    by 0x8FEF208: zim_Timestamp_fromDateTime (message.c:1289)
==14741==    by 0x7D020F: ZEND_DO_FCALL_SPEC_RETVAL_UNUSED_HANDLER (zend_vm_execute.h:970)
==14741==    by 0x77753A: execute_ex (zend_vm_execute.h:429)
==14741==    by 0x7D21AF: zend_execute (zend_vm_execute.h:474)
==14741==    by 0x72DBF2: zend_execute_scripts (zend.c:1476)
==14741==    by 0x6CA37F: php_execute_script (main.c:2537)
==14741==    by 0x7D4435: do_cli (php_cli.c:993)
==14741==    by 0x432EFB: main (php_cli.c:1381)

TeBoring · 2018-07-10T00:32:43Z

php/ext/google/protobuf/message.c

+#endif
+
+    PHP_PROTO_ZVAL_STRING(&function_name, "date_format", 1);
+    PHP_PROTO_ZVAL_STRING(&format_string, "u", 1);


You forget to call zval_dtor(&format_string); which causes memory leak.

TeBoring · 2018-07-10T00:35:20Z

php/ext/google/protobuf/message.c

+    };
+
+    if (call_user_function(EG(function_table), NULL, &function_name, &retval,
+            ARRAY_SIZE(params), params TSRMLS_CC) == FAILURE) {


Instead of using ARRAY_SZIE, why not using actual number?

See my explanation for ARRAY_SIZE below. 🙂
Should I change it back?

Thanks for the explanation. ARRAY_SIZE looks good to me.

TeBoring · 2018-07-10T00:36:56Z

php/ext/google/protobuf/protobuf.h

@@ -42,6 +42,10 @@
 #define MAX_LENGTH_OF_INT64 20
 #define SIZEOF_INT64 8

+/* From Chromium. */
+#define ARRAY_SIZE(x) \
+    ((sizeof(x)/sizeof(0[x])) / ((size_t)(!(sizeof(x) % sizeof(0[x])))))


Why ARRAY_SIZE is like this? Is it related to implementation detail of php runtime?

I've taken that macro from here: https://github.com/google/protobuf/blob/3beb19945b599d448c2524666effe1acdf29165f/php/ext/google/protobuf/upb.c#L8107-L8109

It's a classical macro to determine the size of constant-sized arrays in C.
For instance if you have a char foo[25] then ARRAY_SIZE(foo) will evaluate to 25.

You can find an explanation here: https://stackoverflow.com/a/4415646

Since such a macro is much safer than hardcoding the size of arrays I've started replacing these hardcoded numbers in any PHP-extension code I touched, but I'd recommend using that macro in other places as well as soon as possible. It will probably reduce accidential mistakes in future PRs. 🙂

TeBoring · 2018-07-10T17:02:11Z

Warning: assert(): assert(0 == $timestamp->getNanos()) failed in /home/teboring/protobuf-timestamp/php/tests/memory_leak_test.php on line 155

You probably also need to fix the memory_leak_test.

TeBoring · 2018-07-12T18:12:29Z

any update?

lhecker · 2018-07-12T18:16:22Z

@TeBoring Sorry, I'm pretty busy at the moment and forgot about this PR for the last few days.
I "allocated" myself some time tomorrow though to fix the remaining issues in this PR. 🙂
(It'd be nice if I can get valgrind running on macOS though. 🤔)

TeBoring · 2018-07-12T18:45:22Z

The broken test is not about memory anymore. You can just run it with php.

…

On Thu, Jul 12, 2018 at 11:16 Leonard Hecker ***@***.***> wrote: @TeBoring <https://github.com/TeBoring> Sorry I'm pretty busy at the moment and forgot about this PR for the last few days. I "allocated" myself some time tomorrow though to finish up the remaining issues in this PR. 🙂 (It'd be nice if I can get valgrind running on macOS though. 🤔) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3972 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AE9H5ZzlVxTMi8zJw1LQl6gFTsqehCF0ks5uF5KVgaJpZM4QunPT> .

lhecker · 2018-07-13T12:03:53Z

@TeBoring I fixed this one remaining issue just now. Sorry that you had to wait that long. 🙂

TeBoring · 2018-07-17T20:42:46Z

For php 7.1, c extension test:

1) WellKnownTest::testTimestamp
Failed asserting that 12345000 is identical to 0.
/tmp/protobuf/protobuf/php/tests/protobuf/php/tests/well_known_test.php:315

lhecker · 2018-07-18T12:00:20Z

@TeBoring You (or Kokoro) seem to have made a mistake when testing my PR:
You're using the new C extension (= from this branch) which correctly returns 12345000 for getNanos(), but you're using the old phpunit test (= from master branch or older) for testing.

Proof for my "accusation":
The message

Failed asserting that <actual> is identical to <expected>.

is generated by phpunit's assertSame() function.
But if you check the diff of this PR you'll notice that I replaced the assertSame() in well_known_test.php:315.
It uses assertEqual() now instead, which would print an error message like this:

Failed asserting that <actual> matches expected <expected>.

If I'm correct this means that you're trying to test the changed C extension using the old test code, which is not possible in this case.

TeBoring · 2018-07-19T06:31:38Z

I doubt it's our compatibility test.
https://github.com/google/protobuf/blob/master/php/tests/compatibility_test.sh#L123
Here we have some sed script to fix compatibility test. Could you help add one to fix the broken case?

lhecker · 2018-07-19T16:43:50Z

@TeBoring My fork was out of date and would've resulted in a merge conflict if I touch that file.
I've thus taken the liberty to rebase my fork on the current master.

I also sadly didn't quite understand your last comment.
So I went ahead and wrote a sed command, which simply removes the entire testTimestamp function from the test file.

I feel like this solution is possibly a bit like shooting sparrows with cannons, but I didn't had any better idea. Do you happen to have one?

TeBoring · 2018-07-20T20:02:48Z

@lhecker Thanks for the fix. There were some incorrect test. When we update the against compatibility tests, we will have the correct test. Before that, we just manually disable them. I think your approach is acceptable, because that test is truly not correct.

lhecker · 2018-07-20T22:49:51Z

@TeBoring Thanks for merging this and helping me ironing out all these issues in my PR. 🙂🎉

googlebot added the cla: yes label Nov 29, 2017

lhecker force-pushed the master branch 4 times, most recently from 230b912 to 6655f4c Compare November 30, 2017 11:32

lhecker force-pushed the master branch from 6655f4c to 80d3bdb Compare February 7, 2018 16:01

xfxyjwf assigned TeBoring Jun 23, 2018

xfxyjwf added the php label Jun 23, 2018

TeBoring added the kokoro:run label Jun 25, 2018

kokoro-team removed the kokoro:run label Jun 25, 2018

TeBoring added the wait for user action label Jun 25, 2018

lhecker force-pushed the master branch 3 times, most recently from 8663105 to 59efdd5 Compare June 27, 2018 13:19

lhecker force-pushed the master branch from 59efdd5 to 8824b6b Compare June 28, 2018 10:23

TeBoring added the kokoro:run label Jun 29, 2018

kokoro-team removed the kokoro:run label Jun 29, 2018

TeBoring removed the wait for user action label Jul 9, 2018

TeBoring added the wait for user action label Jul 10, 2018

TeBoring reviewed Jul 10, 2018

View reviewed changes

TeBoring added the kokoro:run label Jul 11, 2018

kokoro-team removed the kokoro:run label Jul 11, 2018

TeBoring added the kokoro:run label Jul 16, 2018

kokoro-team removed the kokoro:run label Jul 16, 2018

TeBoring added the kokoro:run label Jul 19, 2018

lhecker added 2 commits July 19, 2018 18:18

php: Added nanosecond support for Timestamp

015cbd2

php: Fixed compatibility test

423e0bb

lhecker force-pushed the master branch from 4a322f0 to 423e0bb Compare July 19, 2018 16:40

kokoro-team removed the kokoro:run label Jul 19, 2018

TeBoring merged commit e7746f4 into protocolbuffers:master Jul 20, 2018

google-admin unassigned TeBoring Aug 21, 2018

xfxyjwf assigned TeBoring Aug 24, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

php: Added nanosecond support for Timestamp #3972

php: Added nanosecond support for Timestamp #3972

lhecker commented Nov 29, 2017 •

edited

grpc-kokoro commented Nov 29, 2017

grpc-kokoro commented Nov 29, 2017

lhecker commented Mar 6, 2018

TeBoring commented Jun 25, 2018

lhecker commented Jun 27, 2018

TeBoring commented Jul 10, 2018 •

edited

TeBoring Jul 10, 2018

lhecker Jul 10, 2018

TeBoring Jul 10, 2018

lhecker Jul 10, 2018

TeBoring Jul 10, 2018

TeBoring Jul 10, 2018

lhecker Jul 10, 2018

TeBoring commented Jul 10, 2018

TeBoring commented Jul 12, 2018

lhecker commented Jul 12, 2018 •

edited

TeBoring commented Jul 12, 2018 via email

lhecker commented Jul 13, 2018

TeBoring commented Jul 17, 2018

lhecker commented Jul 18, 2018

TeBoring commented Jul 19, 2018

lhecker commented Jul 19, 2018 •

edited

TeBoring commented Jul 20, 2018

lhecker commented Jul 20, 2018

php: Added nanosecond support for Timestamp #3972

php: Added nanosecond support for Timestamp #3972

Conversation

lhecker commented Nov 29, 2017 • edited

grpc-kokoro commented Nov 29, 2017

grpc-kokoro commented Nov 29, 2017

lhecker commented Mar 6, 2018

TeBoring commented Jun 25, 2018

lhecker commented Jun 27, 2018

TeBoring commented Jul 10, 2018 • edited

TeBoring Jul 10, 2018

Choose a reason for hiding this comment

lhecker Jul 10, 2018

Choose a reason for hiding this comment

TeBoring Jul 10, 2018

Choose a reason for hiding this comment

lhecker Jul 10, 2018

Choose a reason for hiding this comment

TeBoring Jul 10, 2018

Choose a reason for hiding this comment

TeBoring Jul 10, 2018

Choose a reason for hiding this comment

lhecker Jul 10, 2018

Choose a reason for hiding this comment

TeBoring commented Jul 10, 2018

TeBoring commented Jul 12, 2018

lhecker commented Jul 12, 2018 • edited

TeBoring commented Jul 12, 2018 via email

lhecker commented Jul 13, 2018

TeBoring commented Jul 17, 2018

lhecker commented Jul 18, 2018

TeBoring commented Jul 19, 2018

lhecker commented Jul 19, 2018 • edited

TeBoring commented Jul 20, 2018

lhecker commented Jul 20, 2018

lhecker commented Nov 29, 2017 •

edited

TeBoring commented Jul 10, 2018 •

edited

lhecker commented Jul 12, 2018 •

edited

lhecker commented Jul 19, 2018 •

edited