ext/pgsql: using ZPP api for calls. #14099

devnexen · 2024-05-01T12:19:31Z

No description provided.

SakiTakamachi · 2024-05-02T16:03:04Z

I did some testing to improve BCMath performance, but in most cases using UNEXPECTED slowed it down. It's probably preventing the compiler from optimizing.

On the other hand, there were some areas where it was quite effective. In my opinion, when adding EXPECTED or UNEXPECTED, it is better to measure the performance, even if it is rough.

devnexen · 2024-05-02T16:30:25Z

To be honest it was mostly negligible most of the time (why I kept as separate commit too), I may not keep it.

nielsdos · 2024-05-02T18:14:06Z

ext/pgsql/pgsql.c

-					if (ZSTR_LEN(tmp) > 0 && *(query + ZSTR_LEN(tmp) - 1) != '\n') {
-						strlcat(query, "\n", ZSTR_LEN(tmp) + 2);
+					zquery = zend_string_alloc(ZSTR_LEN(tmp) + 2, false);
+					if (UNEXPECTED(!zquery)) {


Note that zquery cannot be NULL because zend_string_alloc cannot fail.

devnexen · 2024-05-13T19:33:12Z

ping :)

nielsdos

I only made a quick check, I might've missed some things, but you can start from this.
Please fix these mistakes. There's a lot of repeated mistakes so after fixing what I found please double check everything.

nielsdos · 2024-05-13T19:36:50Z

ext/pgsql/pgsql.c

-	}
+	ZEND_PARSE_PARAMETERS_START(0, 1)
+		Z_PARAM_OPTIONAL
+		Z_PARAM_OBJECT_OF_CLASS(pgsql_link, pgsql_link_ce)


This no longer accepts pg_close(null) anymore, but the signature is function pg_close(?PgSql\Connection $connection = null): true {} so it should be allowed.
The same remark holds in other places too.

nielsdos · 2024-05-13T19:37:41Z

ext/pgsql/pgsql.c

-		}
+		ZEND_PARSE_PARAMETERS_START(2, 2)
+			Z_PARAM_STRING(query, query_len)
+			Z_PARAM_ZVAL(pv_param_arr)


the old parameter parsing using "a" for array, but you now use zval?

nielsdos · 2024-05-13T19:37:46Z

ext/pgsql/pgsql.c

+		ZEND_PARSE_PARAMETERS_START(3, 3)
+			Z_PARAM_OBJECT_OF_CLASS(pgsql_link, pgsql_link_ce)
+			Z_PARAM_STRING(query, query_len)
+			Z_PARAM_ZVAL(pv_param_arr)


nielsdos · 2024-05-13T19:38:12Z

ext/pgsql/pgsql.c

-		}
+		ZEND_PARSE_PARAMETERS_START(2, 2)
+			Z_PARAM_STRING(stmtname, stmtname_len)
+			Z_PARAM_ZVAL(pv_param_arr)


nielsdos · 2024-05-13T19:38:17Z

ext/pgsql/pgsql.c

+		ZEND_PARSE_PARAMETERS_START(3, 3)
+			Z_PARAM_OBJECT_OF_CLASS(pgsql_link, pgsql_link_ce)
+			Z_PARAM_STRING(stmtname, stmtname_len)
+			Z_PARAM_ZVAL(pv_param_arr)


nielsdos · 2024-05-13T19:45:01Z

ext/pgsql/pgsql.c

+	ZEND_PARSE_PARAMETERS_START(3, 4)
+		Z_PARAM_OBJECT_OF_CLASS(pgsql_link, pgsql_link_ce)
+		Z_PARAM_STR(table)
+		Z_PARAM_ZVAL(ids)


nielsdos · 2024-05-13T19:45:08Z

ext/pgsql/pgsql.c

-	}
+	ZEND_PARSE_PARAMETERS_START(2, 5)
+		Z_PARAM_OBJECT_OF_CLASS(pgsql_link, pgsql_link_ce)
+		Z_PARAM_STR(table)


nielsdos · 2024-05-13T19:45:12Z

ext/pgsql/pgsql.c

+		Z_PARAM_OBJECT_OF_CLASS(pgsql_link, pgsql_link_ce)
+		Z_PARAM_STR(table)
+		Z_PARAM_OPTIONAL
+		Z_PARAM_ZVAL(ids)


nielsdos · 2024-05-13T19:45:30Z

ext/pgsql/pgsql.c

-	}
+	ZEND_PARSE_PARAMETERS_START(2, 4)
+		Z_PARAM_OBJECT_OF_CLASS(pgsql_link, pgsql_link_ce)
+		Z_PARAM_STR(table_name)


nielsdos · 2024-05-13T19:45:40Z

ext/pgsql/pgsql.c

-	}
+	ZEND_PARSE_PARAMETERS_START(3, 5)
+		Z_PARAM_OBJECT_OF_CLASS(pgsql_link, pgsql_link_ce)
+		Z_PARAM_STR(table_name)


nielsdos · 2024-05-13T20:43:46Z

@devnexen Note that we have Z_PARAM_OBJ_OF_CLASS_OR_NULL ;) No need to use the EX thing

devnexen · 2024-05-13T21:07:31Z

@devnexen Note that we have Z_PARAM_OBJ_OF_CLASS_OR_NULL ;) No need to use the EX thing

You mean Z_PARAM_OBJECT_OF_CLASS_OR_NULL ?

nielsdos · 2024-05-13T21:10:11Z

@devnexen Note that we have Z_PARAM_OBJ_OF_CLASS_OR_NULL ;) No need to use the EX thing

You mean Z_PARAM_OBJECT_OF_CLASS_OR_NULL ?

Oops, yes

nielsdos

Note that if I say "The same remark holds in other places too." it means that there are many other places where it applies, but not necessarily all of the places.
Also a nit: the old code was already using ZPP, ZPP just stands for "Zend Parameter Parsing", the PR title should be "using fast ZPP API for calls"

nielsdos · 2024-05-14T17:58:42Z

ext/pgsql/pgsql.c

-		RETURN_THROWS();
-	}
+	ZEND_PARSE_PARAMETERS_START(1, 1)
+		Z_PARAM_OBJECT_OF_CLASS_OR_NULL(pgsql_link, pgsql_link_ce)


This wasn't nullable before, but you made it nullable now. Looking at the stub, it should not be nullable.

Cool, at least I learnt a lot of new possible param I did not know existed (esp. path).

nielsdos · 2024-05-14T18:05:06Z

ext/pgsql/pgsql.c

-			RETURN_THROWS();
-		}
+		ZEND_PARSE_PARAMETERS_START(3, 3)
+			Z_PARAM_OBJECT_OF_CLASS_OR_NULL(pgsql_link, pgsql_link_ce)


Should not be nullable.

nielsdos · 2024-05-14T18:07:54Z

ext/pgsql/pgsql.c

-			RETURN_THROWS();
-		}
+		ZEND_PARSE_PARAMETERS_START(3, 3)
+			Z_PARAM_OBJECT_OF_CLASS_OR_NULL(pgsql_link, pgsql_link_ce)


Not nullable

nielsdos · 2024-05-14T18:15:50Z

ext/pgsql/pgsql.c

-		RETURN_THROWS();
-	}
+	ZEND_PARSE_PARAMETERS_START(3, 4)
+		Z_PARAM_OBJECT_OF_CLASS_OR_NULL(pgsql_link, pgsql_link_ce)


Should not be nullable

nielsdos · 2024-05-14T18:16:02Z

ext/pgsql/pgsql.c

-		RETURN_THROWS();
-	}
+	ZEND_PARSE_PARAMETERS_START(3, 4)
+		Z_PARAM_OBJECT_OF_CLASS_OR_NULL(pgsql_link, pgsql_link_ce)


Should not be nullable.

nielsdos · 2024-05-14T18:16:09Z

ext/pgsql/pgsql.c

-		RETURN_THROWS();
-	}
+	ZEND_PARSE_PARAMETERS_START(4, 5)
+		Z_PARAM_OBJECT_OF_CLASS_OR_NULL(pgsql_link, pgsql_link_ce)


Should not be nullable

nielsdos · 2024-05-14T18:16:24Z

ext/pgsql/pgsql.c

-		RETURN_THROWS();
-	}
+	ZEND_PARSE_PARAMETERS_START(3, 4)
+		Z_PARAM_OBJECT_OF_CLASS_OR_NULL(pgsql_link, pgsql_link_ce)


Should not be nullable

nielsdos · 2024-05-14T18:16:36Z

ext/pgsql/pgsql.c

-		RETURN_THROWS();
-	}
+	ZEND_PARSE_PARAMETERS_START(2, 5)
+		Z_PARAM_OBJECT_OF_CLASS_OR_NULL(pgsql_link, pgsql_link_ce)


Should not be nullable

nielsdos

Only one remark remaining

nielsdos · 2024-05-14T19:43:26Z

ext/pgsql/pgsql.c

-					if (ZSTR_LEN(tmp) > 0 && *(query + ZSTR_LEN(tmp) - 1) != '\n') {
-						strlcat(query, "\n", ZSTR_LEN(tmp) + 2);
+					zend_string *zquery = zend_string_alloc(ZSTR_LEN(tmp) + 2, false);
+					memcpy(ZSTR_VAL(zquery), ZSTR_VAL(tmp), ZSTR_LEN(tmp) + 2);


This is wrong, no?
You're copying 1 byte outside of the tmp string. It should be + 1 instead of + 2.

nielsdos · 2024-05-14T20:02:58Z

ext/pgsql/pgsql.c

 					}
-					if (PQputCopyData(pgsql, query, (int)strlen(query)) != 1) {
-						efree(query);
+					if (PQputCopyData(pgsql, ZSTR_VAL(tmp), ZSTR_LEN(tmp)) != 1) {


The use of ZSTR_LEN(tmp) is wrong, because you need to include the '\n' if you added it in the if-check above.

nielsdos · 2024-05-14T20:46:46Z

ext/pgsql/pgsql.c

 					}
-					if (PQputCopyData(pgsql, query, (int)strlen(query)) != 1) {
-						efree(query);
+					if (PQputCopyData(pgsql, ZSTR_VAL(zquery), ZSTR_LEN(tmp) > 0 ? ZSTR_LEN(tmp) + 1 : 0) != 1) {


I see what you do here, and I'm not sure it matters that we can send over an extra NUL terminator.
However, wouldn't it be better to simply adjust the length of the string zquery if it didn't have \n at the end initially?
I also realize now that you only need one additional byte instead of 2 for zend_string_alloc.

nielsdos · 2024-05-15T17:35:34Z

ext/pgsql/pgsql.c

 					}
-					if (PQputCopyData(pgsql, query, (int)strlen(query)) != 1) {
-						efree(query);
+					if (PQputCopyData(pgsql, ZSTR_VAL(zquery), ZSTR_LEN(tmp) > 0 ? ZSTR_LEN(tmp) + 1 : 0) != 1) {


Isn't this what we want?

Suggested change

if (PQputCopyData(pgsql, ZSTR_VAL(zquery), ZSTR_LEN(tmp) > 0 ? ZSTR_LEN(tmp) + 1 : 0) != 1) {

if (PQputCopyData(pgsql, ZSTR_VAL(zquery), ZSTR_LEN(zquery)) != 1) {

Yes that s the point of the length changes :) I forgot to update before going to sleep it seems.

Relatable :P

github-actions bot added the Extension: pgsql label May 1, 2024

devnexen force-pushed the pgsql_zpp branch 2 times, most recently from 851d6c6 to e3f3972 Compare May 1, 2024 22:05

devnexen requested a review from SakiTakamachi May 1, 2024 23:27

devnexen marked this pull request as ready for review May 1, 2024 23:27

nielsdos reviewed May 2, 2024

View reviewed changes

devnexen force-pushed the pgsql_zpp branch from d00a513 to 5a8f07b Compare May 2, 2024 19:31

nielsdos requested changes May 13, 2024

View reviewed changes

devnexen force-pushed the pgsql_zpp branch from 5a8f07b to 6cff632 Compare May 13, 2024 20:35

devnexen force-pushed the pgsql_zpp branch 2 times, most recently from 71692f6 to 4309af0 Compare May 13, 2024 22:00

devnexen changed the title ~~[WIP] ext/pgsql: using ZPP api for calls.~~ ext/pgsql: using ZPP api for calls. May 13, 2024

nielsdos requested changes May 14, 2024

View reviewed changes

devnexen force-pushed the pgsql_zpp branch from 4309af0 to 59f5234 Compare May 14, 2024 18:25

nielsdos requested changes May 14, 2024

View reviewed changes

devnexen force-pushed the pgsql_zpp branch from 59f5234 to 2d531bd Compare May 14, 2024 19:52

nielsdos reviewed May 14, 2024

View reviewed changes

devnexen force-pushed the pgsql_zpp branch from 2d531bd to 5564458 Compare May 14, 2024 20:21

nielsdos reviewed May 14, 2024

View reviewed changes

nielsdos reviewed May 15, 2024

View reviewed changes

ext/pgsql: using fast ZPP api for calls.

4277600

devnexen force-pushed the pgsql_zpp branch from 87c67bc to 4277600 Compare May 15, 2024 17:51

nielsdos approved these changes May 15, 2024

View reviewed changes

devnexen merged commit 0218af8 into php:master May 15, 2024
10 checks passed

	if (PQputCopyData(pgsql, ZSTR_VAL(zquery), ZSTR_LEN(tmp) > 0 ? ZSTR_LEN(tmp) + 1 : 0) != 1) {
	if (PQputCopyData(pgsql, ZSTR_VAL(zquery), ZSTR_LEN(zquery)) != 1) {

ext/pgsql: using ZPP api for calls. #14099

ext/pgsql: using ZPP api for calls. #14099

Conversation

devnexen commented May 1, 2024

SakiTakamachi commented May 2, 2024

devnexen commented May 2, 2024

Choose a reason for hiding this comment

devnexen commented May 13, 2024

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

nielsdos commented May 13, 2024

devnexen commented May 13, 2024

nielsdos commented May 13, 2024

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

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