Reduce memory overhead of DatePeriod via virtual properties #15598

kocsismate · 2024-08-27T11:07:38Z

Related to #11644 and #13988. Test for high memory consumption: https://3v4l.org/QW9Tm

ext/date/php_date.c

kocsismate · 2024-08-28T06:27:24Z

ext/date/tests/date_period_unserialize3.phpt

@@ -33,18 +33,25 @@ object(DatePeriod)#%d (%d) {
  ["start"]=>
  object(DateTime)#%d (%d) {
    ["date"]=>
-    string(26) "2022-07-14 00:00:00.000000"
+    string(26) "2024-08-28 00:00:00.000000"


Unserialization doesn't revert the already done changes, therefore if an error happens at include_start_date (due to the wrong type) then the expected result is that the new values are applied for current, start, end etc.

Isn't this a change in behaviour then? I don't quite understand why you need to make this changes.

The previous behavior was a by-product of storing properties in two places. The following happened: internal struct members of DatePeriod were assigned a value during unserialization, until an incompatible type of the include_start_date key caused a failure. That's why regular object properties were left intact, since https://github.com/php/php-src/pull/15598/files#diff-975ac482adc74b487c6ac9d8dde32153e4b8a803ef8ec41e8d6594f0a042db69L5846 was skipped. The test reflected the old object property values then, while the internal struct contained the new values.

The new behavior is consistent with the comment here:

php-src/ext/date/php_date.c

Line 5694 in 176d285

/* this function does no rollback on error */

(/* this function does no rollback on error */). If there is an error during serialization, there is no "rollback", so the test reflects the new values for those properties which were affected by the unserialization.

nielsdos

Your usage of virtual seems right. I'm not the codeowner of this component so I can't comment much on the other aspects. Ultimately it's up to Derick to decide what to do with this.

nielsdos · 2024-08-28T16:52:24Z

ext/date/php_date.c

+		return;
+	}
+
+	zend_class_entry *ce_ptr = ce;


You moved this out of the else block, but is there a reason for this except for coding style? I would rather not have code motion especially since it's unrelated to this PR. Or am I missing something?

Yes: #15598 (comment) Although I commented this to the wrong line, because line 320 (before my changes) was missing a return and therefore zend_throw_error was invoked twice

It is unrelated to the actual change though (memory reduction); I also don't think the code style should be updated in the same PR/commit, and even if you did, I would prefer that variable declarations be kept at the top of functions.

nielsdos · 2024-08-28T16:53:08Z

ext/date/php_date.c

@@ -2038,7 +1999,7 @@ static int date_object_compare_timezone(zval *tz1, zval *tz2) /* {{{ */

 	if (!o1->initialized || !o2->initialized) {
 		zend_throw_error(date_ce_date_object_error, "Trying to compare uninitialized DateTimeZone objects");
-		return 1;
+		return ZEND_UNCOMPARABLE;


Looks like an unrelated cleanup to this PR and should perhaps be done separately.

Applied separately

Shouldn't it then need a rebase — because it shouldn't show up in this PR again then surely?

nielsdos · 2024-08-28T16:53:49Z

ext/date/php_date.c

@@ -3066,6 +3027,7 @@ PHP_METHOD(DateTime, __wakeup)

 	if (!php_date_initialize_from_hash(&dateobj, myht)) {
 		zend_throw_error(NULL, "Invalid serialization data for DateTime object");
+		RETURN_THROWS();


Same here: I would not do these cleanups/clarifications in the code if it's not strictly necessary in this PR. Otherwise it shows up in the git history as if it's related and that's extra noise + confusion tbh.

Wasn't this done in another PR? Perhaps this needs a rebase then first.

nielsdos · 2024-08-28T16:54:32Z

ext/date/php_date.c

@@ -4656,6 +4623,10 @@ static void php_date_interval_initialize_from_hash(zval **return_value, php_inte
 				Z_STRVAL_P(date_str),
 				err->error_messages[0].position,
 				err->error_messages[0].character ? err->error_messages[0].character : ' ', err->error_messages[0].message);
+
+			timelib_time_dtor(time);


This looks like a real fix, should perhaps be backported?

Yes, that was my original plan but I haven't had time yet: #15598 (comment) Will do it soon.

nielsdos · 2024-08-28T17:30:57Z

ext/date/php_date.c

+
+static HashTable *date_period_get_properties_for(zend_object *object, zend_prop_purpose purpose)
+{
+	switch (purpose) {


I'm a bit confused by the implementation: why this switch is necessary here.

I copy-pasted this piece of code. I'm not sure why Nikita added the switch there... but after a closer examination, it seemed that ZEND_PROP_PURPOSE_GET_OBJECT_VARS is missing from the cases 🤔 I don't see the reason... but in any case, I now removed the switch case so that the custom handler kicks in in all cases now.

derickr · 2024-09-03T16:31:02Z

ext/date/php_date.c

+		return;
+	}
+
+	zend_class_entry *ce_ptr = ce;


It is unrelated to the actual change though (memory reduction); I also don't think the code style should be updated in the same PR/commit, and even if you did, I would prefer that variable declarations be kept at the top of functions.

derickr · 2024-09-03T16:31:41Z

ext/date/php_date.c

@@ -3066,6 +3027,7 @@ PHP_METHOD(DateTime, __wakeup)

 	if (!php_date_initialize_from_hash(&dateobj, myht)) {
 		zend_throw_error(NULL, "Invalid serialization data for DateTime object");
+		RETURN_THROWS();


Wasn't this done in another PR? Perhaps this needs a rebase then first.

derickr · 2024-09-03T16:32:22Z

ext/date/php_date.c

@@ -5934,20 +5903,47 @@ PHP_METHOD(DatePeriod, __wakeup)
 	ZEND_PARSE_PARAMETERS_NONE();

 	period_obj = Z_PHPPERIOD_P(object);
-


Please don't just add whitespace changes.

Sure, I'll get rid of the unnecessary changes once I rebase this PR on another batch of return fixes.

derickr · 2024-09-03T16:34:20Z

ext/date/tests/date_period_unserialize3.phpt

@@ -33,18 +33,25 @@ object(DatePeriod)#%d (%d) {
  ["start"]=>
  object(DateTime)#%d (%d) {
    ["date"]=>
-    string(26) "2022-07-14 00:00:00.000000"
+    string(26) "2024-08-28 00:00:00.000000"


Isn't this a change in behaviour then? I don't quite understand why you need to make this changes.

derickr · 2024-09-03T16:35:17Z

ext/date/tests/gh10747-4.phpt

+  ["var3":protected]=>
+  int(3)
+  ["var4":protected]=>
+  int(4)


This confused me. I thought these were now added where they originally were not. GH diff is confusing sometimes.

Yes, the diff in this case was very confusing for me as well... But in fact, the only thing that changed is the order of the properties...

derickr · 2024-09-03T16:37:37Z

ext/date/php_date.c

-			zend_throw_error(date_ce_date_object_error, "Object of type %s not been correctly initialized by calling parent::__construct() in its constructor", ZSTR_VAL(ce->name));
-		}
-		zend_throw_error(date_ce_date_object_error, "Object of type %s (inheriting %s) has not been correctly initialized by calling parent::__construct() in its constructor", ZSTR_VAL(ce->name), ZSTR_VAL(ce_ptr->name));
+		return;


This should be RETURN_THROWS(), not?

No, RETURN_THROWS() requires a return_value variable in scope (thus it can be mainly used inside PHP_FUNCTIONs/PHP_METHODs)

derickr · 2024-09-03T16:38:28Z

ext/date/php_date.c

 	}
+	zend_throw_error(date_ce_date_object_error, "Object of type %s (inheriting %s) has not been correctly initialized by calling parent::__construct() in its constructor", ZSTR_VAL(ce->name), ZSTR_VAL(ce_ptr->name));


this also misses a RETURN_THROWS() (yes, I know it's the last line in the function, but still)

I can add them separately for sure. I agree that the RETURN_THROWS() are useful

derickr · 2024-09-03T16:40:19Z

ext/date/php_date.c

@@ -2038,7 +1999,7 @@ static int date_object_compare_timezone(zval *tz1, zval *tz2) /* {{{ */

 	if (!o1->initialized || !o2->initialized) {
 		zend_throw_error(date_ce_date_object_error, "Trying to compare uninitialized DateTimeZone objects");
-		return 1;
+		return ZEND_UNCOMPARABLE;


Shouldn't it then need a rebase — because it shouldn't show up in this PR again then surely?

ext/date/php_date.stub.php

Issues originally found in #15598

nielsdos · 2024-09-14T20:50:28Z

Note that you'll need an unset handler that forbids unsetting these virtual properties, otherwise it's going to crash because there's no backing storage. I just did something similar for DOM: #15891

kocsismate · 2024-09-17T06:51:53Z

Note that you'll need an unset handler that forbids unsetting these virtual properties, otherwise it's going to crash because there's no backing storage. I just did something similar for DOM: #15891

Thanks for the heads up, I've just implemented it.

kocsismate · 2024-09-24T10:59:08Z

@nielsdos @derickr Can we still merge it into RC1? Or if it has already been tagged (haven't checked it), could you please take another look so that it can be merged into RC2?

nielsdos

I was at work when RC1 got tagged.
I'm not so sure that the rules allow optimizations to be merged still after RC1 is released.
From the PoV of how virtual properties works this LGTM. But wait for Derick to confirm. Also you still need to answer https://github.com/php/php-src/pull/15598/files#r1742364030

nielsdos

Spoke too soon.
The following will crash because when the object is uninitialized it executes the standard has_property object handler, but that can't handle virtuals of course:

<?php

$rc = new ReflectionClass('DatePeriod');
$di = $rc->newInstanceWithoutConstructor();
var_dump(isset($di->recurrences));

Related to php#11644 and php#13988

kocsismate · 2024-09-26T21:17:00Z

The following will crash because when the object is uninitialized it executes the standard has_property object handler, but that can't handle virtuals of course:

Very smart observation, thank you! I've just force pushed a new commit adding the missing functionality along with a rebase to recent master because I think this optimization can wait until PHP 8.5 indeed.

nielsdos

I only see one mistake anymore

ext/date/php_date.c

nielsdos

I don't see anything wrong anymore

Related to php#11644 and php#13988

kocsismate requested review from nielsdos and Girgias August 27, 2024 11:07

kocsismate requested a review from derickr as a code owner August 27, 2024 11:07

github-actions bot added the Extension: date label Aug 27, 2024

kocsismate marked this pull request as draft August 27, 2024 11:36

kocsismate force-pushed the dateinterval-virtual-properties branch 2 times, most recently from c70de9d to 9144b30 Compare August 27, 2024 14:53

kocsismate marked this pull request as ready for review August 28, 2024 05:36

kocsismate commented Aug 28, 2024

View reviewed changes

ext/date/php_date.c Outdated Show resolved Hide resolved

kocsismate commented Aug 28, 2024

View reviewed changes

nielsdos requested changes Aug 28, 2024

View reviewed changes

kocsismate mentioned this pull request Aug 28, 2024

Add missing return for DatePeriod::__unserialize #15624

Closed

derickr reviewed Sep 3, 2024

View reviewed changes

kocsismate force-pushed the dateinterval-virtual-properties branch from 79f50f4 to 042bfa6 Compare September 3, 2024 19:36

kocsismate mentioned this pull request Sep 3, 2024

Add missing returns in ext/date for PHP 8.3+ #15735

Merged

kocsismate added a commit that referenced this pull request Sep 14, 2024

Add missing returns in ext/date for PHP 8.3+ (#15735)

cb69900

Issues originally found in #15598

kocsismate force-pushed the dateinterval-virtual-properties branch 2 times, most recently from bbc6cf7 to ceb6a95 Compare September 14, 2024 20:23

kocsismate force-pushed the dateinterval-virtual-properties branch from f611bf7 to bd6162c Compare September 17, 2024 06:59

nielsdos approved these changes Sep 24, 2024

View reviewed changes

nielsdos requested changes Sep 24, 2024

View reviewed changes

kocsismate added 6 commits September 26, 2024 22:37

Reduce memory overhead of DatePeriod via virtual properties

f6e03a8

Related to php#11644 and php#13988

Serialization fixes

1112df4

Remove accidentally kept code

5b7e9e6

Remove unintended changes

3330199

Add DatePeriod unset handler

a316fa1

Add DatePeriod has property handler

176d285

kocsismate force-pushed the dateinterval-virtual-properties branch from bd6162c to 176d285 Compare September 26, 2024 21:14

nielsdos requested changes Sep 27, 2024

View reviewed changes

ext/date/php_date.c Show resolved Hide resolved

Fix memory leak

4145691

nielsdos approved these changes Sep 27, 2024

View reviewed changes

kocsismate merged commit 181ea64 into php:master Sep 27, 2024
10 checks passed

kocsismate deleted the dateinterval-virtual-properties branch September 27, 2024 20:54

jorgsowa pushed a commit to jorgsowa/php-src that referenced this pull request Oct 1, 2024

Reduce memory overhead of DatePeriod via virtual properties (php#15598)

ade877b

Related to php#11644 and php#13988

		@@ -5934,20 +5903,47 @@ PHP_METHOD(DatePeriod, __wakeup)
		ZEND_PARSE_PARAMETERS_NONE();

		period_obj = Z_PHPPERIOD_P(object);

		}
		zend_throw_error(date_ce_date_object_error, "Object of type %s (inheriting %s) has not been correctly initialized by calling parent::__construct() in its constructor", ZSTR_VAL(ce->name), ZSTR_VAL(ce_ptr->name));

Reduce memory overhead of DatePeriod via virtual properties #15598

Reduce memory overhead of DatePeriod via virtual properties #15598

Conversation

kocsismate commented Aug 27, 2024 • edited Loading

kocsismate Aug 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nielsdos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kocsismate Sep 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nielsdos commented Sep 14, 2024

kocsismate commented Sep 17, 2024

kocsismate commented Sep 24, 2024

nielsdos left a comment

Choose a reason for hiding this comment

nielsdos left a comment

Choose a reason for hiding this comment

kocsismate commented Sep 26, 2024

nielsdos left a comment

Choose a reason for hiding this comment

nielsdos left a comment

Choose a reason for hiding this comment

kocsismate commented Aug 27, 2024 •

edited

Loading

kocsismate Aug 28, 2024 •

edited

Loading

kocsismate Sep 4, 2024 •

edited

Loading