PHPC-2269: Add internal iterator logic for MongoDB\BSON\Iterator #1454

alcaeus · 2023-08-04T09:19:30Z

Adding to v1.17 only as we'd be backporting a number of compat macros from PHP itself, in addition to ZVAL_OBJ_COPY. Waiting on #1450 to be merged so I don't have to bother fixing the PHP 7.2 build.

alcaeus · 2023-08-04T09:21:11Z

src/BSON/Iterator.c

+{
+	php_phongo_iterator_t* intern = Z_ITERATOR_OBJ_P(&iter->data);
+
+	return intern->valid ? SUCCESS : FAILURE;


I'm once again reminded how dumb PHP is for having SUCCESS defined as 0, when bools work exactly the other way around. Took me a moment to figure out why as I was initially returning intern->valid directly.

I've been bitten by this before, but I've made peace with assuming PHP's behavior is inspired by process exit codes.

alcaeus · 2023-08-04T09:22:05Z

src/BSON/Iterator.c

@@ -263,10 +298,92 @@ static HashTable* php_phongo_iterator_get_properties(phongo_compat_object_handle
 	return php_phongo_iterator_get_properties_hash(object, false);
 }

+/* Iterator handlers */
+static void php_phongo_iterator_it_dtor(zend_object_iterator* iter)


This entire logic was shamelessly lifted from spl_array.c

Does "this entire logic" refer to all of the iterator functions in this file (not just dtor here)?

Yes - all of the php_phongo_iterator_it_* methods were heavily inspired by PHP's standard implementation.

alcaeus · 2023-08-04T09:24:30Z

src/BSON/Iterator.c

+{
+	php_phongo_iterator_t* intern = Z_ITERATOR_OBJ_P(&iter->data);
+
+	return php_phongo_iterator_get_current(intern);


Unlike the current() method itself, there's no need to copy/deref the zval we're returning here. Another notable difference to current() is that we're leaving an IS_UNDEF zval as-is, whereas current returns null in those cases. I don't think it makes a significant difference, and I'm not sure why Cursor::current() (which inspired the BSON iterator) returns null for an undef zval. I know there are some weird shenanigans going on when returning a IS_UNDEF, but I don't think this will ever matter in practice, as having that value returned indicates that something went seriously wrong during the iteration and there should've been an exception anyways.

alcaeus · 2023-08-04T09:25:13Z

src/BSON/Iterator.c

+	if (by_ref) {
+		phongo_throw_exception(PHONGO_ERROR_LOGIC, "An iterator cannot be used with foreach by reference");
+		return NULL;
+	}


There is precedent for this in PHP itself, although they emit an E_ERROR in such cases. I opted to throw instead as it's more consistent how we expose errors.

We can't support iteration by reference as the BSON structures themselves are read-only, so assigning to the decoded value makes no sense.

alcaeus · 2023-08-04T09:26:49Z

src/BSON/Iterator.c

+	php_phongo_iterator_it_get_current_key,
+	php_phongo_iterator_it_move_forward,
+	php_phongo_iterator_it_rewind,
+	NULL,


This is the function pointer for invalidate_current, which isn't used. Quoting from phpinternalsbook:

The only handler that has no correspondence in userland is invalidate_current, which can be used to destroy the current key/value. The handler is largely unused though, in particular foreach won’t even call it.

We could point this to php_phongo_iterator_free_current, although I'm not sure what the benefits of that would be.

php_phongo_iterator_free_current is already called from the necessary places.

I'm fine with leaving invalidate_current unset, unless there's prior art for other iterator implementations using it.

I do think it'd be worth adding a /* invalidate_current */ comment here so we don't need to reference PHONGO_ITERATOR_FUNCS's definition to see what's being omitted.

Added the comment as suggested.

alcaeus · 2023-08-04T12:38:32Z

Here are some results from the performance tests done using the tests added in mongodb/mongo-php-library#1141.

Results in PHP 8.2-debug:

$ ./vendor/bin/phpbench run benchmark/BSON/DocumentBench.php --config=phpbench.ci.json --filter="benchIteration$" --report=aggregate --ref=original
PHPBench (1.2.14) running benchmarks... #standwithukraine
with configuration file: phpbench.ci.json
with PHP version 8.2.8, xdebug ✔, opcache ❌
comparing [actual vs. original]

\MongoDB\Benchmark\BSON\DocumentBench

    benchIteration..........................I2 - [Mo23.925ms vs. Mo114.955ms] -79.19% (±0.19%)

Subjects: 1, Assertions: 0, Failures: 0, Errors: 0
+---------------+----------------+-----+------+-----+---------------+------------------+----------------+
| benchmark     | subject        | set | revs | its | mem_peak      | mode             | rstdev         |
+---------------+----------------+-----+------+-----+---------------+------------------+----------------+
| DocumentBench | benchIteration |     | 10   | 3   | 6.498mb 0.00% | 23.925ms -79.19% | ±0.19% -75.23% |
+---------------+----------------+-----+------+-----+---------------+------------------+----------------+

This shows an improvement of 79% with this new logic. Using a non-debug version of PHP however yields wildly different results:

$ ./vendor/bin/phpbench run benchmark/BSON/DocumentBench.php --config=phpbench.ci.json --filter="benchIteration$" --report=aggregate --ref=original
PHPBench (1.2.14) running benchmarks... #standwithukraine
with configuration file: phpbench.ci.json
with PHP version 8.2.8, xdebug ✔, opcache ❌
comparing [actual vs. original]

\MongoDB\Benchmark\BSON\DocumentBench

    benchIteration..........................I2 - [Mo75.472ms vs. Mo76.018ms] -0.72% (±0.37%)

Subjects: 1, Assertions: 0, Failures: 0, Errors: 0
+---------------+----------------+-----+------+-----+---------------+-----------------+----------------+
| benchmark     | subject        | set | revs | its | mem_peak      | mode            | rstdev         |
+---------------+----------------+-----+------+-----+---------------+-----------------+----------------+
| DocumentBench | benchIteration |     | 10   | 3   | 6.319mb 0.00% | 75.472ms -0.72% | ±0.37% -20.52% |
+---------------+----------------+-----+------+-----+---------------+-----------------+----------------+

Here, there is no improvement whatsoever, but interestingly enough the test is slower than in the debug version, which has me somewhat confused.

The extension was compiled the same way for both runs, always including the developer flags (--enable-mongodb-developer-flags).

alcaeus · 2023-08-18T07:04:28Z

Here are the latest results comparing iteration in non-debug PHP:

benchIteratorToArray....................I0 - [Mo25.636ms vs. Mo77.726ms] -67.02% (±0.00%)

These are the results I was looking for - turns out make install does not respect a custom extension_dir (which I'll have to investigate separately) and I wasn't testing the improvements at all.

jmikola

Some small comment suggestions but implementation LGTM.

jmikola · 2023-08-18T17:22:03Z

src/BSON/Iterator.c

 void php_phongo_iterator_init_ce(INIT_FUNC_ARGS)
 {
 	php_phongo_iterator_ce                = register_class_MongoDB_BSON_Iterator(zend_ce_iterator);
 	php_phongo_iterator_ce->create_object = php_phongo_iterator_create_object;
+	php_phongo_iterator_ce->get_iterator  = php_phongo_iterator_get_iterator;


Quoting PHPC-2269's description:

If this speeds up iteration, we should consider adding the same logic to The MongoDB\Cursor class, which may suffer from the same performance overhead.

Given that this does appear to speed things up, would you like to create a separate issue for making a similar change to Cursor?

Created PHPC-2278.

jmikola · 2023-08-18T17:23:55Z

src/BSON/Iterator.c

+	php_phongo_iterator_it_get_current_key,
+	php_phongo_iterator_it_move_forward,
+	php_phongo_iterator_it_rewind,
+	NULL,


php_phongo_iterator_free_current is already called from the necessary places.

I'm fine with leaving invalidate_current unset, unless there's prior art for other iterator implementations using it.

I do think it'd be worth adding a /* invalidate_current */ comment here so we don't need to reference PHONGO_ITERATOR_FUNCS's definition to see what's being omitted.

jmikola · 2023-08-18T17:24:08Z

src/BSON/Iterator.c

+{
+	php_phongo_iterator_t* intern = Z_ITERATOR_OBJ_P(&iter->data);
+
+	return intern->valid ? SUCCESS : FAILURE;


I've been bitten by this before, but I've made peace with assuming PHP's behavior is inspired by process exit codes.

jmikola · 2023-08-24T13:31:30Z

src/phongo_compat.h

+			(invalidate_current),                                                                                               \
+			(get_gc),                                                                                                           \
+	}
+#else


Suggested change

#else

#else /* PHP_VERSION_ID < 80000 */

This is used in some other longer #if blocks elsewhere in the file.

Note that we don't have prior art for this with #if(n)def, so I didn't suggest it below.

jmikola · 2023-08-24T13:31:47Z

src/phongo_compat.h

+			(rewind),                                                                                                           \
+			(invalidate_current),                                                                                               \
+	}
+#endif


Suggested change

#endif

#endif /* PHP_VERSION_ID >= 80000 */

jmikola · 2023-08-24T13:34:24Z

src/BSON/Iterator.c

@@ -263,10 +298,92 @@ static HashTable* php_phongo_iterator_get_properties(phongo_compat_object_handle
 	return php_phongo_iterator_get_properties_hash(object, false);
 }

+/* Iterator handlers */
+static void php_phongo_iterator_it_dtor(zend_object_iterator* iter)


Does "this entire logic" refer to all of the iterator functions in this file (not just dtor here)?

alcaeus requested a review from jmikola August 4, 2023 09:19

alcaeus self-assigned this Aug 4, 2023

alcaeus commented Aug 4, 2023

View reviewed changes

alcaeus force-pushed the phpc-2269-bson-iterator-internal branch from 3904abe to cbc8eb9 Compare August 4, 2023 10:40

alcaeus changed the base branch from v1.16 to master August 4, 2023 10:40

alcaeus force-pushed the phpc-2269-bson-iterator-internal branch from cbc8eb9 to 8171ae3 Compare August 4, 2023 10:41

alcaeus marked this pull request as draft August 4, 2023 15:51

alcaeus force-pushed the phpc-2269-bson-iterator-internal branch from 8171ae3 to 08fb5d9 Compare August 18, 2023 06:53

alcaeus marked this pull request as ready for review August 18, 2023 07:04

alcaeus force-pushed the phpc-2269-bson-iterator-internal branch from 08fb5d9 to a9a1be5 Compare August 22, 2023 12:41

jmikola approved these changes Aug 24, 2023

View reviewed changes

alcaeus added 3 commits August 25, 2023 09:30

PHPC-2269: Add internal iterator logic for MongoDB\BSON\Iterator

90435be

Add compat macros for PHP 7.4

7ef035e

Apply code review suggestions

75d478f

alcaeus force-pushed the phpc-2269-bson-iterator-internal branch from a9a1be5 to 75d478f Compare August 25, 2023 07:31

alcaeus merged commit cc31f3a into mongodb:master Aug 25, 2023

alcaeus deleted the phpc-2269-bson-iterator-internal branch August 25, 2023 07:56

PHPC-2269: Add internal iterator logic for MongoDB\BSON\Iterator #1454

PHPC-2269: Add internal iterator logic for MongoDB\BSON\Iterator #1454

Uh oh!

Conversation

alcaeus commented Aug 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alcaeus Aug 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alcaeus commented Aug 4, 2023

Uh oh!

alcaeus commented Aug 18, 2023

Uh oh!

jmikola left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alcaeus commented Aug 4, 2023 •

edited

Loading

alcaeus Aug 25, 2023 •

edited

Loading