Skip to content

session: Stop using php_combined_lcg() #13568

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Mar 2, 2024
Merged

Conversation

TimWolla
Copy link
Member

@TimWolla TimWolla commented Mar 1, 2024

No description provided.

TimWolla added 2 commits March 1, 2024 19:01
The previous ordering resulted in a needlessly large number of holes and split
several `zval`s across cache line boundaries. Do the bare minimum of reordering
to keep related members grouped, but reducing the struct size by 32 bytes and
keeping `zval`s within a single cache line.

Before:

    struct _php_session_rfc1867_progress {
            size_t                     sname_len;            /*     0     8 */
            zval                       sid;                  /*     8    16 */
            smart_str                  key;                  /*    24    16 */
            zend_long                  update_step;          /*    40     8 */
            zend_long                  next_update;          /*    48     8 */
            double                     next_update_time;     /*    56     8 */
            /* --- cacheline 1 boundary (64 bytes) --- */
            _Bool                      cancel_upload;        /*    64     1 */
            _Bool                      apply_trans_sid;      /*    65     1 */

            /* XXX 6 bytes hole, try to pack */

            size_t                     content_length;       /*    72     8 */
            zval                       data;                 /*    80    16 */
            zval *                     post_bytes_processed; /*    96     8 */
            zval                       files;                /*   104    16 */
            zval                       current_file;         /*   120    16 */
            /* --- cacheline 2 boundary (128 bytes) was 8 bytes ago --- */
            zval *                     current_file_bytes_processed; /*   136     8 */

            /* size: 144, cachelines: 3, members: 14 */
            /* sum members: 138, holes: 1, sum holes: 6 */
            /* last cacheline: 16 bytes */
    };
    struct _php_ps_globals {
            char *                     save_path;            /*     0     8 */
            char *                     session_name;         /*     8     8 */
            zend_string *              id;                   /*    16     8 */
            char *                     extern_referer_chk;   /*    24     8 */
            char *                     cache_limiter;        /*    32     8 */
            zend_long                  cookie_lifetime;      /*    40     8 */
            char *                     cookie_path;          /*    48     8 */
            char *                     cookie_domain;        /*    56     8 */
            /* --- cacheline 1 boundary (64 bytes) --- */
            _Bool                      cookie_secure;        /*    64     1 */
            _Bool                      cookie_httponly;      /*    65     1 */

            /* XXX 6 bytes hole, try to pack */

            char *                     cookie_samesite;      /*    72     8 */
            const ps_module  *         mod;                  /*    80     8 */
            const ps_module  *         default_mod;          /*    88     8 */
            void *                     mod_data;             /*    96     8 */
            php_session_status         session_status;       /*   104     4 */

            /* XXX 4 bytes hole, try to pack */

            zend_string *              session_started_filename; /*   112     8 */
            uint32_t                   session_started_lineno; /*   120     4 */

            /* XXX 4 bytes hole, try to pack */

            /* --- cacheline 2 boundary (128 bytes) --- */
            zend_long                  gc_probability;       /*   128     8 */
            zend_long                  gc_divisor;           /*   136     8 */
            zend_long                  gc_maxlifetime;       /*   144     8 */
            int                        module_number;        /*   152     4 */

            /* XXX 4 bytes hole, try to pack */

            zend_long                  cache_expire;         /*   160     8 */
            struct {
                    zval               ps_open;              /*   168    16 */
                    zval               ps_close;             /*   184    16 */
                    /* --- cacheline 3 boundary (192 bytes) was 8 bytes ago --- */
                    zval               ps_read;              /*   200    16 */
                    zval               ps_write;             /*   216    16 */
                    zval               ps_destroy;           /*   232    16 */
                    zval               ps_gc;                /*   248    16 */
                    /* --- cacheline 4 boundary (256 bytes) was 8 bytes ago --- */
                    zval               ps_create_sid;        /*   264    16 */
                    zval               ps_validate_sid;      /*   280    16 */
                    zval               ps_update_timestamp;  /*   296    16 */
            } mod_user_names;                                /*   168   144 */
            _Bool                      mod_user_implemented; /*   312     1 */
            _Bool                      mod_user_is_open;     /*   313     1 */

            /* XXX 6 bytes hole, try to pack */

            /* --- cacheline 5 boundary (320 bytes) --- */
            zend_string *              mod_user_class_name;  /*   320     8 */
            const struct ps_serializer_struct  * serializer; /*   328     8 */
            zval                       http_session_vars;    /*   336    16 */
            _Bool                      auto_start;           /*   352     1 */
            _Bool                      use_cookies;          /*   353     1 */
            _Bool                      use_only_cookies;     /*   354     1 */
            _Bool                      use_trans_sid;        /*   355     1 */

            /* XXX 4 bytes hole, try to pack */

            zend_long                  sid_length;           /*   360     8 */
            zend_long                  sid_bits_per_character; /*   368     8 */
            _Bool                      send_cookie;          /*   376     1 */
            _Bool                      define_sid;           /*   377     1 */

            /* XXX 6 bytes hole, try to pack */

            /* --- cacheline 6 boundary (384 bytes) --- */
            php_session_rfc1867_progress * rfc1867_progress; /*   384     8 */
            _Bool                      rfc1867_enabled;      /*   392     1 */
            _Bool                      rfc1867_cleanup;      /*   393     1 */

            /* XXX 6 bytes hole, try to pack */

            char *                     rfc1867_prefix;       /*   400     8 */
            char *                     rfc1867_name;         /*   408     8 */
            zend_long                  rfc1867_freq;         /*   416     8 */
            double                     rfc1867_min_freq;     /*   424     8 */
            _Bool                      use_strict_mode;      /*   432     1 */
            _Bool                      lazy_write;           /*   433     1 */
            _Bool                      in_save_handler;      /*   434     1 */
            _Bool                      set_handler;          /*   435     1 */

            /* XXX 4 bytes hole, try to pack */

            zend_string *              session_vars;         /*   440     8 */

            /* size: 448, cachelines: 7, members: 48 */
            /* sum members: 404, holes: 9, sum holes: 44 */
    };

After:

    struct _php_session_rfc1867_progress {
            size_t                     sname_len;            /*     0     8 */
            zval                       sid;                  /*     8    16 */
            smart_str                  key;                  /*    24    16 */
            zend_long                  update_step;          /*    40     8 */
            zend_long                  next_update;          /*    48     8 */
            double                     next_update_time;     /*    56     8 */
            /* --- cacheline 1 boundary (64 bytes) --- */
            _Bool                      cancel_upload;        /*    64     1 */
            _Bool                      apply_trans_sid;      /*    65     1 */

            /* XXX 6 bytes hole, try to pack */

            size_t                     content_length;       /*    72     8 */
            zval                       data;                 /*    80    16 */
            zval                       files;                /*    96    16 */
            zval *                     post_bytes_processed; /*   112     8 */
            zval *                     current_file_bytes_processed; /*   120     8 */
            /* --- cacheline 2 boundary (128 bytes) --- */
            zval                       current_file;         /*   128    16 */

            /* size: 144, cachelines: 3, members: 14 */
            /* sum members: 138, holes: 1, sum holes: 6 */
            /* last cacheline: 16 bytes */
    };
    struct _php_ps_globals {
            char *                     save_path;            /*     0     8 */
            char *                     session_name;         /*     8     8 */
            zend_string *              id;                   /*    16     8 */
            char *                     extern_referer_chk;   /*    24     8 */
            char *                     cache_limiter;        /*    32     8 */
            zend_long                  cookie_lifetime;      /*    40     8 */
            char *                     cookie_path;          /*    48     8 */
            char *                     cookie_domain;        /*    56     8 */
            /* --- cacheline 1 boundary (64 bytes) --- */
            char *                     cookie_samesite;      /*    64     8 */
            _Bool                      cookie_secure;        /*    72     1 */
            _Bool                      cookie_httponly;      /*    73     1 */

            /* XXX 6 bytes hole, try to pack */

            const ps_module  *         mod;                  /*    80     8 */
            const ps_module  *         default_mod;          /*    88     8 */
            void *                     mod_data;             /*    96     8 */
            php_session_status         session_status;       /*   104     4 */

            /* XXX 4 bytes hole, try to pack */

            zend_string *              session_started_filename; /*   112     8 */
            uint32_t                   session_started_lineno; /*   120     4 */
            int                        module_number;        /*   124     4 */
            /* --- cacheline 2 boundary (128 bytes) --- */
            zend_long                  gc_probability;       /*   128     8 */
            zend_long                  gc_divisor;           /*   136     8 */
            zend_long                  gc_maxlifetime;       /*   144     8 */
            zend_long                  cache_expire;         /*   152     8 */
            struct {
                    zval               ps_open;              /*   160    16 */
                    zval               ps_close;             /*   176    16 */
                    /* --- cacheline 3 boundary (192 bytes) --- */
                    zval               ps_read;              /*   192    16 */
                    zval               ps_write;             /*   208    16 */
                    zval               ps_destroy;           /*   224    16 */
                    zval               ps_gc;                /*   240    16 */
                    /* --- cacheline 4 boundary (256 bytes) --- */
                    zval               ps_create_sid;        /*   256    16 */
                    zval               ps_validate_sid;      /*   272    16 */
                    zval               ps_update_timestamp;  /*   288    16 */
            } mod_user_names;                                /*   160   144 */
            zend_string *              mod_user_class_name;  /*   304     8 */
            _Bool                      mod_user_implemented; /*   312     1 */
            _Bool                      mod_user_is_open;     /*   313     1 */
            _Bool                      auto_start;           /*   314     1 */
            _Bool                      use_cookies;          /*   315     1 */
            _Bool                      use_only_cookies;     /*   316     1 */
            _Bool                      use_trans_sid;        /*   317     1 */
            _Bool                      send_cookie;          /*   318     1 */
            _Bool                      define_sid;           /*   319     1 */
            /* --- cacheline 5 boundary (320 bytes) --- */
            const struct ps_serializer_struct  * serializer; /*   320     8 */
            zval                       http_session_vars;    /*   328    16 */
            zend_long                  sid_length;           /*   344     8 */
            zend_long                  sid_bits_per_character; /*   352     8 */
            php_session_rfc1867_progress * rfc1867_progress; /*   360     8 */
            char *                     rfc1867_prefix;       /*   368     8 */
            char *                     rfc1867_name;         /*   376     8 */
            /* --- cacheline 6 boundary (384 bytes) --- */
            zend_long                  rfc1867_freq;         /*   384     8 */
            double                     rfc1867_min_freq;     /*   392     8 */
            _Bool                      rfc1867_enabled;      /*   400     1 */
            _Bool                      rfc1867_cleanup;      /*   401     1 */
            _Bool                      use_strict_mode;      /*   402     1 */
            _Bool                      lazy_write;           /*   403     1 */
            _Bool                      in_save_handler;      /*   404     1 */
            _Bool                      set_handler;          /*   405     1 */

            /* XXX 2 bytes hole, try to pack */

            zend_string *              session_vars;         /*   408     8 */

            /* size: 416, cachelines: 7, members: 48 */
            /* sum members: 404, holes: 3, sum holes: 12 */
            /* last cacheline: 32 bytes */
    };
The CombinedLCG is a terrible RNG with a questionable API and should ideally
not be used anymore. While in the case of ext/session it is only used for
probabilistic garbage collection where the quality of the RNG is not of
particular importance, there are better choices.

Replace the RNG used for garbage collection by an ext/session specific instance
of PcgOneseq128XslRr64. Its 16 Byte state nicely fits into the memory freed up
by the previous reordering of the session globals struct, even allowing to the
storage of the php_random_algo_with_state struct, making using the RNG a little
nicer.

Instead multiplying the float returned by the CombinedLCG by the GC Divisor to
obtain an integer between 0 and the divisor we can just use `php_random_range`
to directly generate an appropriate integer, completely avoiding the floating
point maths, making it easier to verify the code for correctness.
Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you!

@Girgias Girgias merged commit f6c38fc into php:master Mar 2, 2024
@TimWolla TimWolla deleted the session-random branch March 2, 2024 11:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants