ext/session: start to implement GH-14019#17575
Conversation
|
Stupid question: why glib? Could we get away with using the internals HashTable with shared memory? |
|
the glib hashtable implementation is multiplatform, solid and fast so it lessen the burden of implementation. |
devnexen
force-pushed
the
session_mem_rewrite
branch
from
f8bd3cf to
d4c9c26
Compare
Girgias
left a comment
Girgias
left a comment
There was a problem hiding this comment.
This seems reasonable, but I'm not a ZTS specialist, I would like @arnaud-lb opinion on the ZTS aspect. :)
| @@ -270,7 +180,7 @@ PHP_MINIT_FUNCTION(ps_mm) | |||
| char *ps_mm_path, euid[30]; | |||
| zend_result ret; | |||
|
|
|||
| ps_mm_instance = calloc(sizeof(*ps_mm_instance), 1); | |||
| ps_mm_instance = calloc(1, sizeof(*ps_mm_instance)); | |||
There was a problem hiding this comment.
Could this be ecalloc() or must it be the system malloc?
There was a problem hiding this comment.
hmmm not sure we can use zendmm that early but I ll try later
There was a problem hiding this comment.
It must be a persistent allocator (system one or pecalloc(..., true))
| @@ -16,10 +16,10 @@ | |||
|
|
|||
| #include "php.h" | |||
|
|
|||
| #ifdef HAVE_LIBMM | |||
| #ifdef HAVE_LIBGLIB | |||
There was a problem hiding this comment.
You need similar changes in ext/session/session.c, otherwise mod_mm is not enabled.
| static zend_result ps_mm_initialize(ps_mm *data, const char *path) | ||
| { | ||
| data->owner = getpid(); | ||
| data->mm = mm_create(0, path); | ||
| data->mm = g_mapped_file_new(path, TRUE, NULL); |
There was a problem hiding this comment.
This fails on my machine when path doesn't exist
| if (!data->mm) { | ||
| return FAILURE; | ||
| } | ||
|
|
||
| data->hash_cnt = 0; | ||
| data->hash_max = 511; | ||
| data->hash = mm_calloc(data->mm, data->hash_max + 1, sizeof(ps_sd *)); | ||
| data->hash = g_hash_table_new(ps_mm_hash, ps_mm_key_equals); |
There was a problem hiding this comment.
I'm not familiar with glib, does the call to g_mapped_file_new() above imply that every g_ function will use that mapped file for allocation?
|
The approach for ZTS supports seems fine, but I don't understand how this is using shared memory. Could you clarify @devnexen? |
|
I think I may have been "extra zealous" regarding ZTS but I ll look after your remarks sometime this week end |
devnexen
force-pushed
the
session_mem_rewrite
branch
from
bde5ddf to
d29d438
Compare
|
Is there a chance it may land in PHP 8.6? libmm is very outdated. Also it needs rebase, because mod_mm.c file has been changed. |
devnexen
force-pushed
the
session_mem_rewrite
branch
from
d29d438 to
bdbc671
Compare
Since you re into the session extension, I added a TODO as a possible follow-up if you re interested. |
5 participants
No description provided.