FAQ
Hi internals!

The discussions around the patch for improved 64bit support have
deteriorated from a semi-reasonable discussions to pointless accusations
and name-calling. I'd like to get back to discussing this issue from a
technical point of view and see which parts of the patch can be salvaged
and which can not.

As the current voting results show, the 64bit changes, if accepted, will be
integrated into phpng rather than into master. As such I will argue purely
from a phpng perspective.

Before going any further, it should be established that two aspects of the
64 bit patch are, as far as I can see, acceptable to everyone: The
introduction of 64bit integers on Win64 and other LLP64 architectures and
the use of an unsigned integer type for lengths and sizes.

The disputed aspect of the patch is the use of 64 bit lengths and sizes. It
has been argued that the use of 64bit sizes will significantly increase
memory consumption of the phpng branch and may also negatively impact
performance.

If you take a look at an early patch for porting the 64bit changes for
phpng [1] you'll find that these are very real concerns, as the changes
increase the sizes of many commonly used data structures.

However, this is only the case if 64bit sizes are blindly used in every
possible place. In the following I'll argue how a more careful choice of
the used size type in key places can make this patch a lot more realistic.

First of all it should be noted that the patch introduces size_t usage in
several places where, as far as I can see, it is not necessary. For example
it changes the storage type of line numbers from zend_uint to zend_size_t:

From zend_opline:
     - uint lineno;
     + zend_size_t lineno;

From zend_class_entry:
     - zend_uint line_start;
     - zend_uint line_end;
     + zend_size_t line_start;
     + zend_size_t line_end;

I don't think we need to concern ourselves about files with more than 4
billion lines.

Similarly the patch also changes the length of argument names and class
typehints to 64bit:

From zend_arg_info:
     - zend_uint name_len;
     + zend_size_t name_len;
        const char *class_name;
     - zend_uint class_name_len;
     + zend_size_t class_name_len;

This once again seem rather unnecessary and wasteful. Another case are the
number of arguments of a function:

From zend_op_array:
     - zend_uint num_args;
     - zend_uint required_num_args;
     + zend_size_t num_args;
     + zend_size_t required_num_args;

Again it seems doubtful whether functions with more than 4 billion
arguments are quite relevant.

While there are more examples than these, I'll not go into this point any
further, as the pattern should be clear. Instead we'll be moving on to the
zend_string and HashTable structures.

For the HashTable structure, the diff currently looks as follows:

     - zend_uint nTableSize;
     - zend_uint nTableMask;
     - zend_uint nNumUsed;
     - zend_uint nNumOfElements;
     - long nNextFreeElement;
     + zend_uint_t nTableSize;
     + zend_uint_t nTableMask;
     + zend_uint_t nNumUsed;
     + zend_uint_t nNumOfElements;
     + zend_int_t nNextFreeElement;
      Bucket *arData;
     - zend_uint *arHash;
     + zend_uint_t *arHash;
       dtor_func_t pDestructor;
       zend_uint nInternalPointer;

This actually misses changing nInternalPointer. If we change that one as
well and account for differences in alignment, the total increase for the
HashTable structure is 6*4 = 24 bytes. For a heavily used structure, that's
a lot. However, even worse are the per-bucket memory increases:

From the diff above you can already see that the arHash array will be twice
as large, i.e. you'll get an additional 4 bytes per bucket. However - and
this is currently not represented in the patch stub - changing hashtables
to 64bit lengths will make the use of Z_NEXT(bucket->val) to store the next
collision resolution index impossible. As such another 8 bytes per bucket
will be needed.

This brings us to a total increase of 24 bytes per hashtable and 12 bytes
per bucket. Especially the latter seems unacceptable to me.

As such, I would suggest to *not* change sizes for the hashtable structure
and limit these changes to other parts of the engine (strings in
particular).

To put this into context, consider what it actually means to have an array
with more than 4 billion elements (which is the point where the different
size type becomes relevant). With hashtables as implemented in master with
144 bytes per bucket, this would require 2^32 * 144 = 576 GB of memory. If
we introduced 64bit hashtable sizes in phpng, you'd still need 206 GB of
memory for to create an array that can make use of those sizes.

I hope this visualizes that supporting 64bit sizes for hashtables is not
necessary. We can easily implement a size restriction in
zend_hash_do_resize. As hashtables go through a centralized API controlling
the size limitation is a lot more straightforward than for strings.

Now that we have covered the HashTable changes, we're only left with one
significant change. The diff for the zend_string structure is as follows:

       zend_refcounted gc;
     - zend_ulong h; /* hash value */
     - int len;
     + zend_uint_t h; /* hash value */
     + zend_size_t len;
       char val[1];

This means that a zend_string will on average be approximately 4 bytes
longer. (Approximately because the exact difference is subject to details
of the allocation process and depends on the string length distribution.)

I think that this difference might be acceptable, if this is the cost of
uniform string size usage in the codebase. I tested the impact that adding
4 bytes on the string structure has on one application and the result was a
change from 38.2 to 38.5 MiB, which translates to a 0.7% difference. That
difference is acceptable to me.

However I don't know how representative that result is and I don't
currently have time to set up a testing environment for something like
wordpress on a 64bit vm. I would appreciate it if somebody who already has
such a setup could run a test for this, to confirm that there is indeed no
large difference in memory consumption. Just add a "int unused" after the
len member and compare memory usage.

So, what I'm suggesting here is the following:

We implement 64bit integers on LLP64, we implement unsigned sizes, we
implement size_t for strings lengths. I honestly still don't fully
understand the necessity for the latter, but if the impact in phpng is
indeed low (in the 1% range), then I think I can compromise on that.

What we don't implement is indiscriminate usage of size_t or other 64bit
types all over the place (like for linenos, argument counts, argument
lengths etc) and usage of 64bit sizes for hashtables.

This would be acceptable for me and I hope that it would also be acceptable
for others.

While we're at it, I would also like to quickly touch on the topic of
naming and renaming. I hope it is clear now that we need both 64bit types
for some things and 32bit types for others (like HT sizes, linenos, etc).
As such I would suggest to use zend_(u)long_t as the 64bit type (on 64bit
platforms of course) and zend_(u)int_t as the 32bit type. This more or less
matches our current naming and hopefully avoids confusion.

With that naming convention, we should also continue using IS_LONG and
Z_LVAL instead of the suggested IS_INT and Z_IVAL. I'd also keep using
Z_STRLEN instead of Z_STRSIZE and wouldn't do changes to zpp, especially as
they conflict with other zpp changes in phpng. I understand why these
changes were beneficial in the original 64bit patch, however considering
that the usage of many APIs changed in phpng and extensions need to be
carefully reviewed anyway, I don't think these renames make a lot of sense
anymore.

So, that's my opinion on how we should proceed with the 64bit patch. I very
much hope that we can reach some consensus about this.

Credits: This mail is the result of a discussion between Anthony, Bob and
myself.

Nikita

   [1]: https://gist.github.com/weltling/a941d8cf6c731640b51f

Search Discussions

  • Zeev Suraski at May 17, 2014 at 6:46 pm
    Nikita,

    I think your email is spot on, except for one thing. Thankfully, there
    haven't any accusations or name calling - at least none that I've seen. And
    that's a good thing, even when we have a heated debate.
    I'll vote Yes on an RFC-translated version of this email in a heartbeat.

    Zeev
    -----Original Message-----
    From: Nikita Popov
    Sent: Saturday, May 17, 2014 9:27 PM
    To: PHP internals
    Subject: [PHP-DEV] Rethinking 64bit sizes and PHP-NG

    Hi internals!

    The discussions around the patch for improved 64bit support have
    deteriorated
    from a semi-reasonable discussions to pointless accusations and
    name-calling.
    I'd like to get back to discussing this issue from a technical point of
    view and see
    which parts of the patch can be salvaged and which can not.

    As the current voting results show, the 64bit changes, if accepted, will
    be
    integrated into phpng rather than into master. As such I will argue purely
    from a
    phpng perspective.

    Before going any further, it should be established that two aspects of the
    64 bit patch are, as far as I can see, acceptable to everyone: The
    introduction of
    64bit integers on Win64 and other LLP64 architectures and the use of an
    unsigned integer type for lengths and sizes.

    The disputed aspect of the patch is the use of 64 bit lengths and sizes.
    It has
    been argued that the use of 64bit sizes will significantly increase memory
    consumption of the phpng branch and may also negatively impact
    performance.

    If you take a look at an early patch for porting the 64bit changes for
    phpng [1]
    you'll find that these are very real concerns, as the changes increase the
    sizes of
    many commonly used data structures.

    However, this is only the case if 64bit sizes are blindly used in every
    possible
    place. In the following I'll argue how a more careful choice of the used
    size type
    in key places can make this patch a lot more realistic.

    First of all it should be noted that the patch introduces size_t usage in
    several
    places where, as far as I can see, it is not necessary. For example it
    changes the
    storage type of line numbers from zend_uint to zend_size_t:

    From zend_opline:
    - uint lineno;
    + zend_size_t lineno;

    From zend_class_entry:
    - zend_uint line_start;
    - zend_uint line_end;
    + zend_size_t line_start;
    + zend_size_t line_end;

    I don't think we need to concern ourselves about files with more than 4
    billion
    lines.

    Similarly the patch also changes the length of argument names and class
    typehints to 64bit:

    From zend_arg_info:
    - zend_uint name_len;
    + zend_size_t name_len;
    const char *class_name;
    - zend_uint class_name_len;
    + zend_size_t class_name_len;

    This once again seem rather unnecessary and wasteful. Another case are the
    number of arguments of a function:

    From zend_op_array:
    - zend_uint num_args;
    - zend_uint required_num_args;
    + zend_size_t num_args;
    + zend_size_t required_num_args;

    Again it seems doubtful whether functions with more than 4 billion
    arguments
    are quite relevant.

    While there are more examples than these, I'll not go into this point any
    further,
    as the pattern should be clear. Instead we'll be moving on to the
    zend_string and
    HashTable structures.

    For the HashTable structure, the diff currently looks as follows:

    - zend_uint nTableSize;
    - zend_uint nTableMask;
    - zend_uint nNumUsed;
    - zend_uint nNumOfElements;
    - long nNextFreeElement;
    + zend_uint_t nTableSize;
    + zend_uint_t nTableMask;
    + zend_uint_t nNumUsed;
    + zend_uint_t nNumOfElements;
    + zend_int_t nNextFreeElement;
    Bucket *arData;
    - zend_uint *arHash;
    + zend_uint_t *arHash;
    dtor_func_t pDestructor;
    zend_uint nInternalPointer;

    This actually misses changing nInternalPointer. If we change that one as
    well
    and account for differences in alignment, the total increase for the
    HashTable
    structure is 6*4 = 24 bytes. For a heavily used structure, that's a lot.
    However,
    even worse are the per-bucket memory increases:

    From the diff above you can already see that the arHash array will be
    twice as
    large, i.e. you'll get an additional 4 bytes per bucket. However - and
    this is
    currently not represented in the patch stub - changing hashtables to 64bit
    lengths will make the use of Z_NEXT(bucket->val) to store the next
    collision
    resolution index impossible. As such another 8 bytes per bucket will be
    needed.

    This brings us to a total increase of 24 bytes per hashtable and 12 bytes
    per
    bucket. Especially the latter seems unacceptable to me.

    As such, I would suggest to *not* change sizes for the hashtable structure
    and
    limit these changes to other parts of the engine (strings in particular).

    To put this into context, consider what it actually means to have an array
    with
    more than 4 billion elements (which is the point where the different size
    type
    becomes relevant). With hashtables as implemented in master with
    144 bytes per bucket, this would require 2^32 * 144 = 576 GB of memory. If
    we
    introduced 64bit hashtable sizes in phpng, you'd still need 206 GB of
    memory for
    to create an array that can make use of those sizes.

    I hope this visualizes that supporting 64bit sizes for hashtables is not
    necessary.
    We can easily implement a size restriction in zend_hash_do_resize. As
    hashtables go through a centralized API controlling the size limitation is
    a lot
    more straightforward than for strings.

    Now that we have covered the HashTable changes, we're only left with one
    significant change. The diff for the zend_string structure is as follows:

    zend_refcounted gc;
    - zend_ulong h; /* hash value */
    - int len;
    + zend_uint_t h; /* hash value */
    + zend_size_t len;
    char val[1];

    This means that a zend_string will on average be approximately 4 bytes
    longer.
    (Approximately because the exact difference is subject to details of the
    allocation process and depends on the string length distribution.)

    I think that this difference might be acceptable, if this is the cost of
    uniform
    string size usage in the codebase. I tested the impact that adding
    4 bytes on the string structure has on one application and the result was
    a
    change from 38.2 to 38.5 MiB, which translates to a 0.7% difference. That
    difference is acceptable to me.

    However I don't know how representative that result is and I don't
    currently
    have time to set up a testing environment for something like wordpress on
    a
    64bit vm. I would appreciate it if somebody who already has such a setup
    could
    run a test for this, to confirm that there is indeed no large difference
    in memory
    consumption. Just add a "int unused" after the len member and compare
    memory usage.

    So, what I'm suggesting here is the following:

    We implement 64bit integers on LLP64, we implement unsigned sizes, we
    implement size_t for strings lengths. I honestly still don't fully
    understand the
    necessity for the latter, but if the impact in phpng is indeed low (in the
    1%
    range), then I think I can compromise on that.

    What we don't implement is indiscriminate usage of size_t or other 64bit
    types
    all over the place (like for linenos, argument counts, argument lengths
    etc) and
    usage of 64bit sizes for hashtables.

    This would be acceptable for me and I hope that it would also be
    acceptable for
    others.

    While we're at it, I would also like to quickly touch on the topic of
    naming and
    renaming. I hope it is clear now that we need both 64bit types for some
    things
    and 32bit types for others (like HT sizes, linenos, etc).
    As such I would suggest to use zend_(u)long_t as the 64bit type (on 64bit
    platforms of course) and zend_(u)int_t as the 32bit type. This more or
    less
    matches our current naming and hopefully avoids confusion.

    With that naming convention, we should also continue using IS_LONG and
    Z_LVAL instead of the suggested IS_INT and Z_IVAL. I'd also keep using
    Z_STRLEN instead of Z_STRSIZE and wouldn't do changes to zpp, especially
    as
    they conflict with other zpp changes in phpng. I understand why these
    changes
    were beneficial in the original 64bit patch, however considering that the
    usage
    of many APIs changed in phpng and extensions need to be carefully reviewed
    anyway, I don't think these renames make a lot of sense anymore.

    So, that's my opinion on how we should proceed with the 64bit patch. I
    very
    much hope that we can reach some consensus about this.

    Credits: This mail is the result of a discussion between Anthony, Bob and
    myself.

    Nikita

    [1]: https://gist.github.com/weltling/a941d8cf6c731640b51f
  • Nikita Nefedov at May 17, 2014 at 7:02 pm

    On Sat, 17 May 2014 22:27:02 +0400, Nikita Popov wrote:

    Hi internals!

    The discussions around the patch for improved 64bit support have
    deteriorated from a semi-reasonable discussions to pointless accusations
    and name-calling. I'd like to get back to discussing this issue from a
    technical point of view and see which parts of the patch can be salvaged
    and which can not.

    Hey,

    First thank you for arranging all this points.

    I just wanted to ask: could we also make nNextFreeElement be unsigned?
    Because with your proposal it would be possible to overflow it with the
    code like this: $array[pow(2, 32) + 1] = 123; (if I understand it
    correctly)

    And besides that we can't have negative keys anyway.
  • Nikita Popov at May 17, 2014 at 7:11 pm

    On Sat, May 17, 2014 at 9:01 PM, Nikita Nefedov wrote:

    Hey,

    First thank you for arranging all this points.

    I just wanted to ask: could we also make nNextFreeElement be unsigned?
    Because with your proposal it would be possible to overflow it with the
    code like this: $array[pow(2, 32) + 1] = 123; (if I understand it correctly)

    And besides that we can't have negative keys anyway.
    PHP arrays store integral keys larger than PHP_INT_MAX as a string rather
    than an integer. Auto-incrementing does not work past that point. If you
    write something like this...

         $array[PHP_INT_MAX] = 42;
         $array[] = 43;

    ...you'll receive an error (this is intentional). You get that error
    because the code explicitly makes sure that nNextFreeElement can never
    overflow. (Here is an example of such an overflow check:
    http://lxr.php.net/xref/PHP_TRUNK/Zend/zend_hash.c#435).

    Nikita
  • Nikita Nefedov at May 17, 2014 at 7:38 pm

    On Sat, 17 May 2014 23:11:13 +0400, Nikita Popov wrote:
    On Sat, May 17, 2014 at 9:01 PM, Nikita Nefedov wrote:


    Hey,



    First thank you for arranging all this points.



    I just wanted to ask: could we also make nNextFreeElement be unsigned?
    Because with your proposal it >>would be possible to overflow it with
    the code like this: $array[pow(2, 32) + 1] = 123; (if I understand >>it
    correctly)




    And besides that we can't have negative keys anyway.
    PHP arrays store integral keys larger than PHP_INT_MAX as a string
    rather than an integer. Auto->incrementing does not work past that
    point. If you write something like this...


    $array[PHP_INT_MAX] = 42;
    $array[] = 43;

    ...you'll receive an error (this is intentional). You get that error
    because the code explicitly makes >sure that nNextFreeElement can never
    overflow. (Here is an example of such an overflow check:
    http://>lxr.php.net/xref/PHP_TRUNK/Zend/zend_hash.c#435).


    Nikita
    Ah well, my fault didn't see it :)
  • Ferenc Kovacs at May 17, 2014 at 7:43 pm

    On Sat, May 17, 2014 at 8:27 PM, Nikita Popov wrote:

    Hi internals!

    The discussions around the patch for improved 64bit support have
    deteriorated from a semi-reasonable discussions to pointless accusations
    and name-calling. I'd like to get back to discussing this issue from a
    technical point of view and see which parts of the patch can be salvaged
    and which can not.

    As the current voting results show, the 64bit changes, if accepted, will be
    integrated into phpng rather than into master. As such I will argue purely
    from a phpng perspective.

    Before going any further, it should be established that two aspects of the
    64 bit patch are, as far as I can see, acceptable to everyone: The
    introduction of 64bit integers on Win64 and other LLP64 architectures and
    the use of an unsigned integer type for lengths and sizes.

    The disputed aspect of the patch is the use of 64 bit lengths and sizes. It
    has been argued that the use of 64bit sizes will significantly increase
    memory consumption of the phpng branch and may also negatively impact
    performance.

    If you take a look at an early patch for porting the 64bit changes for
    phpng [1] you'll find that these are very real concerns, as the changes
    increase the sizes of many commonly used data structures.

    However, this is only the case if 64bit sizes are blindly used in every
    possible place. In the following I'll argue how a more careful choice of
    the used size type in key places can make this patch a lot more realistic.

    First of all it should be noted that the patch introduces size_t usage in
    several places where, as far as I can see, it is not necessary. For example
    it changes the storage type of line numbers from zend_uint to zend_size_t:

    From zend_opline:
    - uint lineno;
    + zend_size_t lineno;

    From zend_class_entry:
    - zend_uint line_start;
    - zend_uint line_end;
    + zend_size_t line_start;
    + zend_size_t line_end;

    I don't think we need to concern ourselves about files with more than 4
    billion lines.

    Similarly the patch also changes the length of argument names and class
    typehints to 64bit:

    From zend_arg_info:
    - zend_uint name_len;
    + zend_size_t name_len;
    const char *class_name;
    - zend_uint class_name_len;
    + zend_size_t class_name_len;

    This once again seem rather unnecessary and wasteful. Another case are the
    number of arguments of a function:

    From zend_op_array:
    - zend_uint num_args;
    - zend_uint required_num_args;
    + zend_size_t num_args;
    + zend_size_t required_num_args;

    Again it seems doubtful whether functions with more than 4 billion
    arguments are quite relevant.

    While there are more examples than these, I'll not go into this point any
    further, as the pattern should be clear. Instead we'll be moving on to the
    zend_string and HashTable structures.

    For the HashTable structure, the diff currently looks as follows:

    - zend_uint nTableSize;
    - zend_uint nTableMask;
    - zend_uint nNumUsed;
    - zend_uint nNumOfElements;
    - long nNextFreeElement;
    + zend_uint_t nTableSize;
    + zend_uint_t nTableMask;
    + zend_uint_t nNumUsed;
    + zend_uint_t nNumOfElements;
    + zend_int_t nNextFreeElement;
    Bucket *arData;
    - zend_uint *arHash;
    + zend_uint_t *arHash;
    dtor_func_t pDestructor;
    zend_uint nInternalPointer;

    This actually misses changing nInternalPointer. If we change that one as
    well and account for differences in alignment, the total increase for the
    HashTable structure is 6*4 = 24 bytes. For a heavily used structure, that's
    a lot. However, even worse are the per-bucket memory increases:

    From the diff above you can already see that the arHash array will be twice
    as large, i.e. you'll get an additional 4 bytes per bucket. However - and
    this is currently not represented in the patch stub - changing hashtables
    to 64bit lengths will make the use of Z_NEXT(bucket->val) to store the next
    collision resolution index impossible. As such another 8 bytes per bucket
    will be needed.

    This brings us to a total increase of 24 bytes per hashtable and 12 bytes
    per bucket. Especially the latter seems unacceptable to me.

    As such, I would suggest to *not* change sizes for the hashtable structure
    and limit these changes to other parts of the engine (strings in
    particular).

    To put this into context, consider what it actually means to have an array
    with more than 4 billion elements (which is the point where the different
    size type becomes relevant). With hashtables as implemented in master with
    144 bytes per bucket, this would require 2^32 * 144 = 576 GB of memory. If
    we introduced 64bit hashtable sizes in phpng, you'd still need 206 GB of
    memory for to create an array that can make use of those sizes.

    I hope this visualizes that supporting 64bit sizes for hashtables is not
    necessary. We can easily implement a size restriction in
    zend_hash_do_resize. As hashtables go through a centralized API controlling
    the size limitation is a lot more straightforward than for strings.

    Now that we have covered the HashTable changes, we're only left with one
    significant change. The diff for the zend_string structure is as follows:

    zend_refcounted gc;
    - zend_ulong h; /* hash value */
    - int len;
    + zend_uint_t h; /* hash value */
    + zend_size_t len;
    char val[1];

    This means that a zend_string will on average be approximately 4 bytes
    longer. (Approximately because the exact difference is subject to details
    of the allocation process and depends on the string length distribution.)

    I think that this difference might be acceptable, if this is the cost of
    uniform string size usage in the codebase. I tested the impact that adding
    4 bytes on the string structure has on one application and the result was a
    change from 38.2 to 38.5 MiB, which translates to a 0.7% difference. That
    difference is acceptable to me.

    However I don't know how representative that result is and I don't
    currently have time to set up a testing environment for something like
    wordpress on a 64bit vm. I would appreciate it if somebody who already has
    such a setup could run a test for this, to confirm that there is indeed no
    large difference in memory consumption. Just add a "int unused" after the
    len member and compare memory usage.

    So, what I'm suggesting here is the following:

    We implement 64bit integers on LLP64, we implement unsigned sizes, we
    implement size_t for strings lengths. I honestly still don't fully
    understand the necessity for the latter, but if the impact in phpng is
    indeed low (in the 1% range), then I think I can compromise on that.

    What we don't implement is indiscriminate usage of size_t or other 64bit
    types all over the place (like for linenos, argument counts, argument
    lengths etc) and usage of 64bit sizes for hashtables.

    This would be acceptable for me and I hope that it would also be acceptable
    for others.

    While we're at it, I would also like to quickly touch on the topic of
    naming and renaming. I hope it is clear now that we need both 64bit types
    for some things and 32bit types for others (like HT sizes, linenos, etc).
    As such I would suggest to use zend_(u)long_t as the 64bit type (on 64bit
    platforms of course) and zend_(u)int_t as the 32bit type. This more or less
    matches our current naming and hopefully avoids confusion.

    With that naming convention, we should also continue using IS_LONG and
    Z_LVAL instead of the suggested IS_INT and Z_IVAL. I'd also keep using
    Z_STRLEN instead of Z_STRSIZE and wouldn't do changes to zpp, especially as
    they conflict with other zpp changes in phpng. I understand why these
    changes were beneficial in the original 64bit patch, however considering
    that the usage of many APIs changed in phpng and extensions need to be
    carefully reviewed anyway, I don't think these renames make a lot of sense
    anymore.

    So, that's my opinion on how we should proceed with the 64bit patch. I very
    much hope that we can reach some consensus about this.

    Credits: This mail is the result of a discussion between Anthony, Bob and
    myself.

    Nikita

    [1]: https://gist.github.com/weltling/a941d8cf6c731640b51f
    First of all: thank you for bringing back the discussion to the technical
    level.
    I would really like if Pierre and Anatol could reply what do they think
    about this, as based on the previous discussion, the changes proposed here
    would be acceptable for most people voting no for the original rfc, and it
    is only sacrifices parts which was already considered as a "side-effect"
    not a goal of the rfc anyways.

    --
    Ferenc Kovács
    @Tyr43l - http://tyrael.hu
  • Dmitry Stogov at May 17, 2014 at 8:12 pm
    Hi Nikita,

    Thanks for this deep analyzes.
    I hope, this will lead us to agreement.

    I see no problems with accepting IS_LONG part of the patch.
    Then we may think about string length.
    As you pointed, it makes no sense to use size_t everywhere, and may be
    using it only in "right" places won't make serious harm.

    Thanks. Dmitry.


    On Sat, May 17, 2014 at 10:27 PM, Nikita Popov wrote:

    Hi internals!

    The discussions around the patch for improved 64bit support have
    deteriorated from a semi-reasonable discussions to pointless accusations
    and name-calling. I'd like to get back to discussing this issue from a
    technical point of view and see which parts of the patch can be salvaged
    and which can not.

    As the current voting results show, the 64bit changes, if accepted, will be
    integrated into phpng rather than into master. As such I will argue purely
    from a phpng perspective.

    Before going any further, it should be established that two aspects of the
    64 bit patch are, as far as I can see, acceptable to everyone: The
    introduction of 64bit integers on Win64 and other LLP64 architectures and
    the use of an unsigned integer type for lengths and sizes.

    The disputed aspect of the patch is the use of 64 bit lengths and sizes. It
    has been argued that the use of 64bit sizes will significantly increase
    memory consumption of the phpng branch and may also negatively impact
    performance.

    If you take a look at an early patch for porting the 64bit changes for
    phpng [1] you'll find that these are very real concerns, as the changes
    increase the sizes of many commonly used data structures.

    However, this is only the case if 64bit sizes are blindly used in every
    possible place. In the following I'll argue how a more careful choice of
    the used size type in key places can make this patch a lot more realistic.

    First of all it should be noted that the patch introduces size_t usage in
    several places where, as far as I can see, it is not necessary. For example
    it changes the storage type of line numbers from zend_uint to zend_size_t:

    From zend_opline:
    - uint lineno;
    + zend_size_t lineno;

    From zend_class_entry:
    - zend_uint line_start;
    - zend_uint line_end;
    + zend_size_t line_start;
    + zend_size_t line_end;

    I don't think we need to concern ourselves about files with more than 4
    billion lines.

    Similarly the patch also changes the length of argument names and class
    typehints to 64bit:

    From zend_arg_info:
    - zend_uint name_len;
    + zend_size_t name_len;
    const char *class_name;
    - zend_uint class_name_len;
    + zend_size_t class_name_len;

    This once again seem rather unnecessary and wasteful. Another case are the
    number of arguments of a function:

    From zend_op_array:
    - zend_uint num_args;
    - zend_uint required_num_args;
    + zend_size_t num_args;
    + zend_size_t required_num_args;

    Again it seems doubtful whether functions with more than 4 billion
    arguments are quite relevant.

    While there are more examples than these, I'll not go into this point any
    further, as the pattern should be clear. Instead we'll be moving on to the
    zend_string and HashTable structures.

    For the HashTable structure, the diff currently looks as follows:

    - zend_uint nTableSize;
    - zend_uint nTableMask;
    - zend_uint nNumUsed;
    - zend_uint nNumOfElements;
    - long nNextFreeElement;
    + zend_uint_t nTableSize;
    + zend_uint_t nTableMask;
    + zend_uint_t nNumUsed;
    + zend_uint_t nNumOfElements;
    + zend_int_t nNextFreeElement;
    Bucket *arData;
    - zend_uint *arHash;
    + zend_uint_t *arHash;
    dtor_func_t pDestructor;
    zend_uint nInternalPointer;

    This actually misses changing nInternalPointer. If we change that one as
    well and account for differences in alignment, the total increase for the
    HashTable structure is 6*4 = 24 bytes. For a heavily used structure, that's
    a lot. However, even worse are the per-bucket memory increases:

    From the diff above you can already see that the arHash array will be twice
    as large, i.e. you'll get an additional 4 bytes per bucket. However - and
    this is currently not represented in the patch stub - changing hashtables
    to 64bit lengths will make the use of Z_NEXT(bucket->val) to store the next
    collision resolution index impossible. As such another 8 bytes per bucket
    will be needed.

    This brings us to a total increase of 24 bytes per hashtable and 12 bytes
    per bucket. Especially the latter seems unacceptable to me.

    As such, I would suggest to *not* change sizes for the hashtable structure
    and limit these changes to other parts of the engine (strings in
    particular).

    To put this into context, consider what it actually means to have an array
    with more than 4 billion elements (which is the point where the different
    size type becomes relevant). With hashtables as implemented in master with
    144 bytes per bucket, this would require 2^32 * 144 = 576 GB of memory. If
    we introduced 64bit hashtable sizes in phpng, you'd still need 206 GB of
    memory for to create an array that can make use of those sizes.

    I hope this visualizes that supporting 64bit sizes for hashtables is not
    necessary. We can easily implement a size restriction in
    zend_hash_do_resize. As hashtables go through a centralized API controlling
    the size limitation is a lot more straightforward than for strings.

    Now that we have covered the HashTable changes, we're only left with one
    significant change. The diff for the zend_string structure is as follows:

    zend_refcounted gc;
    - zend_ulong h; /* hash value */
    - int len;
    + zend_uint_t h; /* hash value */
    + zend_size_t len;
    char val[1];

    This means that a zend_string will on average be approximately 4 bytes
    longer. (Approximately because the exact difference is subject to details
    of the allocation process and depends on the string length distribution.)

    I think that this difference might be acceptable, if this is the cost of
    uniform string size usage in the codebase. I tested the impact that adding
    4 bytes on the string structure has on one application and the result was a
    change from 38.2 to 38.5 MiB, which translates to a 0.7% difference. That
    difference is acceptable to me.

    However I don't know how representative that result is and I don't
    currently have time to set up a testing environment for something like
    wordpress on a 64bit vm. I would appreciate it if somebody who already has
    such a setup could run a test for this, to confirm that there is indeed no
    large difference in memory consumption. Just add a "int unused" after the
    len member and compare memory usage.

    So, what I'm suggesting here is the following:

    We implement 64bit integers on LLP64, we implement unsigned sizes, we
    implement size_t for strings lengths. I honestly still don't fully
    understand the necessity for the latter, but if the impact in phpng is
    indeed low (in the 1% range), then I think I can compromise on that.

    What we don't implement is indiscriminate usage of size_t or other 64bit
    types all over the place (like for linenos, argument counts, argument
    lengths etc) and usage of 64bit sizes for hashtables.

    This would be acceptable for me and I hope that it would also be acceptable
    for others.

    While we're at it, I would also like to quickly touch on the topic of
    naming and renaming. I hope it is clear now that we need both 64bit types
    for some things and 32bit types for others (like HT sizes, linenos, etc).
    As such I would suggest to use zend_(u)long_t as the 64bit type (on 64bit
    platforms of course) and zend_(u)int_t as the 32bit type. This more or less
    matches our current naming and hopefully avoids confusion.

    With that naming convention, we should also continue using IS_LONG and
    Z_LVAL instead of the suggested IS_INT and Z_IVAL. I'd also keep using
    Z_STRLEN instead of Z_STRSIZE and wouldn't do changes to zpp, especially as
    they conflict with other zpp changes in phpng. I understand why these
    changes were beneficial in the original 64bit patch, however considering
    that the usage of many APIs changed in phpng and extensions need to be
    carefully reviewed anyway, I don't think these renames make a lot of sense
    anymore.

    So, that's my opinion on how we should proceed with the 64bit patch. I very
    much hope that we can reach some consensus about this.

    Credits: This mail is the result of a discussion between Anthony, Bob and
    myself.

    Nikita

    [1]: https://gist.github.com/weltling/a941d8cf6c731640b51f
  • Anatol Belski at May 17, 2014 at 8:38 pm
    Hi,
    On Sat, May 17, 2014 22:12, Dmitry Stogov wrote:
    Hi Nikita,


    Thanks for this deep analyzes.
    I hope, this will lead us to agreement.


    I see no problems with accepting IS_LONG part of the patch.
    Then we may think about string length.
    As you pointed, it makes no sense to use size_t everywhere, and may be
    using it only in "right" places won't make serious harm.

    Thanks. Dmitry.




    On Sat, May 17, 2014 at 10:27 PM, Nikita Popov wrote:

    Hi internals!


    The discussions around the patch for improved 64bit support have
    deteriorated from a semi-reasonable discussions to pointless accusations
    and name-calling. I'd like to get back to discussing this issue from a
    technical point of view and see which parts of the patch can be
    salvaged and which can not.

    As the current voting results show, the 64bit changes, if accepted,
    will be integrated into phpng rather than into master. As such I will
    argue purely from a phpng perspective.

    Before going any further, it should be established that two aspects of
    the 64 bit patch are, as far as I can see, acceptable to everyone: The
    introduction of 64bit integers on Win64 and other LLP64 architectures
    and the use of an unsigned integer type for lengths and sizes.

    The disputed aspect of the patch is the use of 64 bit lengths and
    sizes. It has been argued that the use of 64bit sizes will significantly
    increase memory consumption of the phpng branch and may also negatively
    impact performance.

    If you take a look at an early patch for porting the 64bit changes for
    phpng [1] you'll find that these are very real concerns, as the changes
    increase the sizes of many commonly used data structures.

    However, this is only the case if 64bit sizes are blindly used in every
    possible place. In the following I'll argue how a more careful choice
    of the used size type in key places can make this patch a lot more
    realistic.

    First of all it should be noted that the patch introduces size_t usage
    in several places where, as far as I can see, it is not necessary. For
    example it changes the storage type of line numbers from zend_uint to
    zend_size_t:


    From zend_opline:
    - uint lineno;
    + zend_size_t lineno;


    From zend_class_entry:
    - zend_uint line_start;
    - zend_uint line_end;
    + zend_size_t line_start;
    + zend_size_t line_end;


    I don't think we need to concern ourselves about files with more than 4
    billion lines.

    Similarly the patch also changes the length of argument names and class
    typehints to 64bit:

    From zend_arg_info:
    - zend_uint name_len;
    + zend_size_t name_len;
    const char *class_name; - zend_uint class_name_len;
    + zend_size_t class_name_len;


    This once again seem rather unnecessary and wasteful. Another case are
    the number of arguments of a function:

    From zend_op_array:
    - zend_uint num_args;
    - zend_uint required_num_args;
    + zend_size_t num_args;
    + zend_size_t required_num_args;


    Again it seems doubtful whether functions with more than 4 billion
    arguments are quite relevant.

    While there are more examples than these, I'll not go into this point
    any further, as the pattern should be clear. Instead we'll be moving on
    to the zend_string and HashTable structures.

    For the HashTable structure, the diff currently looks as follows:


    - zend_uint nTableSize;
    - zend_uint nTableMask;
    - zend_uint nNumUsed;
    - zend_uint nNumOfElements;
    - long nNextFreeElement;
    + zend_uint_t nTableSize;
    + zend_uint_t nTableMask;
    + zend_uint_t nNumUsed;
    + zend_uint_t nNumOfElements;
    + zend_int_t nNextFreeElement;
    Bucket *arData;
    - zend_uint *arHash;
    + zend_uint_t *arHash;
    dtor_func_t pDestructor; zend_uint nInternalPointer;

    This actually misses changing nInternalPointer. If we change that one
    as well and account for differences in alignment, the total increase for
    the HashTable structure is 6*4 = 24 bytes. For a heavily used structure,
    that's a lot. However, even worse are the per-bucket memory increases:

    From the diff above you can already see that the arHash array will be
    twice as large, i.e. you'll get an additional 4 bytes per bucket.
    However - and
    this is currently not represented in the patch stub - changing
    hashtables to 64bit lengths will make the use of Z_NEXT(bucket->val) to
    store the next collision resolution index impossible. As such another 8
    bytes per bucket will be needed.

    This brings us to a total increase of 24 bytes per hashtable and 12
    bytes per bucket. Especially the latter seems unacceptable to me.

    As such, I would suggest to *not* change sizes for the hashtable
    structure and limit these changes to other parts of the engine (strings
    in particular).

    To put this into context, consider what it actually means to have an
    array with more than 4 billion elements (which is the point where the
    different size type becomes relevant). With hashtables as implemented in
    master with 144 bytes per bucket, this would require 2^32 * 144 = 576 GB
    of memory. If we introduced 64bit hashtable sizes in phpng, you'd still
    need 206 GB of memory for to create an array that can make use of those
    sizes.

    I hope this visualizes that supporting 64bit sizes for hashtables is
    not necessary. We can easily implement a size restriction in
    zend_hash_do_resize. As hashtables go through a centralized API
    controlling the size limitation is a lot more straightforward than for
    strings.

    Now that we have covered the HashTable changes, we're only left with
    one significant change. The diff for the zend_string structure is as
    follows:


    zend_refcounted gc; - zend_ulong h; /* hash
    value */ - int len;
    + zend_uint_t h; /* hash value */
    + zend_size_t len;
    char val[1];

    This means that a zend_string will on average be approximately 4 bytes
    longer. (Approximately because the exact difference is subject to
    details of the allocation process and depends on the string length
    distribution.)

    I think that this difference might be acceptable, if this is the cost
    of uniform string size usage in the codebase. I tested the impact that
    adding 4 bytes on the string structure has on one application and the
    result was a change from 38.2 to 38.5 MiB, which translates to a 0.7%
    difference. That difference is acceptable to me.

    However I don't know how representative that result is and I don't
    currently have time to set up a testing environment for something like
    wordpress on a 64bit vm. I would appreciate it if somebody who already
    has such a setup could run a test for this, to confirm that there is
    indeed no large difference in memory consumption. Just add a "int
    unused" after the len member and compare memory usage.

    So, what I'm suggesting here is the following:


    We implement 64bit integers on LLP64, we implement unsigned sizes, we
    implement size_t for strings lengths. I honestly still don't fully
    understand the necessity for the latter, but if the impact in phpng is
    indeed low (in the 1% range), then I think I can compromise on that.

    What we don't implement is indiscriminate usage of size_t or other
    64bit
    types all over the place (like for linenos, argument counts, argument
    lengths etc) and usage of 64bit sizes for hashtables.

    This would be acceptable for me and I hope that it would also be
    acceptable for others.

    While we're at it, I would also like to quickly touch on the topic of
    naming and renaming. I hope it is clear now that we need both 64bit
    types for some things and 32bit types for others (like HT sizes,
    linenos, etc). As such I would suggest to use zend_(u)long_t as the
    64bit type (on 64bit
    platforms of course) and zend_(u)int_t as the 32bit type. This more or
    less matches our current naming and hopefully avoids confusion.

    With that naming convention, we should also continue using IS_LONG and
    Z_LVAL instead of the suggested IS_INT and Z_IVAL. I'd also keep using
    Z_STRLEN instead of Z_STRSIZE and wouldn't do changes to zpp, especially
    as they conflict with other zpp changes in phpng. I understand why these
    changes were beneficial in the original 64bit patch, however
    considering that the usage of many APIs changed in phpng and extensions
    need to be carefully reviewed anyway, I don't think these renames make a
    lot of sense anymore.

    So, that's my opinion on how we should proceed with the 64bit patch. I
    very much hope that we can reach some consensus about this.

    Credits: This mail is the result of a discussion between Anthony, Bob
    and myself.

    Nikita
    I really appreciate you guys have looked inside the str_size_and_int64
    patch. Dmitry even has compiled and tested it.

    While the Nikitas notes sound pretty meaningful, let me ask you a one
    simple question before proceeding with any details - how much time you
    personally would spend on the integration?

    Thanks

    Anatol
  • Dmitry Stogov at May 17, 2014 at 8:43 pm
    I may take a part of the work. It's not a problem.

    Thanks. Dmitry.



    On Sun, May 18, 2014 at 12:37 AM, Anatol Belski wrote:

    Hi,
    On Sat, May 17, 2014 22:12, Dmitry Stogov wrote:
    Hi Nikita,


    Thanks for this deep analyzes.
    I hope, this will lead us to agreement.


    I see no problems with accepting IS_LONG part of the patch.
    Then we may think about string length.
    As you pointed, it makes no sense to use size_t everywhere, and may be
    using it only in "right" places won't make serious harm.

    Thanks. Dmitry.




    On Sat, May 17, 2014 at 10:27 PM, Nikita Popov <nikita.ppv@gmail.com>
    wrote:

    Hi internals!


    The discussions around the patch for improved 64bit support have
    deteriorated from a semi-reasonable discussions to pointless accusations
    and name-calling. I'd like to get back to discussing this issue from a
    technical point of view and see which parts of the patch can be
    salvaged and which can not.

    As the current voting results show, the 64bit changes, if accepted,
    will be integrated into phpng rather than into master. As such I will
    argue purely from a phpng perspective.

    Before going any further, it should be established that two aspects of
    the 64 bit patch are, as far as I can see, acceptable to everyone: The
    introduction of 64bit integers on Win64 and other LLP64 architectures
    and the use of an unsigned integer type for lengths and sizes.

    The disputed aspect of the patch is the use of 64 bit lengths and
    sizes. It has been argued that the use of 64bit sizes will significantly
    increase memory consumption of the phpng branch and may also negatively
    impact performance.

    If you take a look at an early patch for porting the 64bit changes for
    phpng [1] you'll find that these are very real concerns, as the changes
    increase the sizes of many commonly used data structures.

    However, this is only the case if 64bit sizes are blindly used in every
    possible place. In the following I'll argue how a more careful choice
    of the used size type in key places can make this patch a lot more
    realistic.

    First of all it should be noted that the patch introduces size_t usage
    in several places where, as far as I can see, it is not necessary. For
    example it changes the storage type of line numbers from zend_uint to
    zend_size_t:


    From zend_opline:
    - uint lineno;
    + zend_size_t lineno;


    From zend_class_entry:
    - zend_uint line_start;
    - zend_uint line_end;
    + zend_size_t line_start;
    + zend_size_t line_end;


    I don't think we need to concern ourselves about files with more than 4
    billion lines.

    Similarly the patch also changes the length of argument names and class
    typehints to 64bit:

    From zend_arg_info:
    - zend_uint name_len;
    + zend_size_t name_len;
    const char *class_name; - zend_uint class_name_len;
    + zend_size_t class_name_len;


    This once again seem rather unnecessary and wasteful. Another case are
    the number of arguments of a function:

    From zend_op_array:
    - zend_uint num_args;
    - zend_uint required_num_args;
    + zend_size_t num_args;
    + zend_size_t required_num_args;


    Again it seems doubtful whether functions with more than 4 billion
    arguments are quite relevant.

    While there are more examples than these, I'll not go into this point
    any further, as the pattern should be clear. Instead we'll be moving on
    to the zend_string and HashTable structures.

    For the HashTable structure, the diff currently looks as follows:


    - zend_uint nTableSize;
    - zend_uint nTableMask;
    - zend_uint nNumUsed;
    - zend_uint nNumOfElements;
    - long nNextFreeElement;
    + zend_uint_t nTableSize;
    + zend_uint_t nTableMask;
    + zend_uint_t nNumUsed;
    + zend_uint_t nNumOfElements;
    + zend_int_t nNextFreeElement;
    Bucket *arData;
    - zend_uint *arHash;
    + zend_uint_t *arHash;
    dtor_func_t pDestructor; zend_uint nInternalPointer;

    This actually misses changing nInternalPointer. If we change that one
    as well and account for differences in alignment, the total increase for
    the HashTable structure is 6*4 = 24 bytes. For a heavily used structure,
    that's a lot. However, even worse are the per-bucket memory increases:

    From the diff above you can already see that the arHash array will be
    twice as large, i.e. you'll get an additional 4 bytes per bucket.
    However - and
    this is currently not represented in the patch stub - changing
    hashtables to 64bit lengths will make the use of Z_NEXT(bucket->val) to
    store the next collision resolution index impossible. As such another 8
    bytes per bucket will be needed.

    This brings us to a total increase of 24 bytes per hashtable and 12
    bytes per bucket. Especially the latter seems unacceptable to me.

    As such, I would suggest to *not* change sizes for the hashtable
    structure and limit these changes to other parts of the engine (strings
    in particular).

    To put this into context, consider what it actually means to have an
    array with more than 4 billion elements (which is the point where the
    different size type becomes relevant). With hashtables as implemented in
    master with 144 bytes per bucket, this would require 2^32 * 144 = 576 GB
    of memory. If we introduced 64bit hashtable sizes in phpng, you'd still
    need 206 GB of memory for to create an array that can make use of those
    sizes.

    I hope this visualizes that supporting 64bit sizes for hashtables is
    not necessary. We can easily implement a size restriction in
    zend_hash_do_resize. As hashtables go through a centralized API
    controlling the size limitation is a lot more straightforward than for
    strings.

    Now that we have covered the HashTable changes, we're only left with
    one significant change. The diff for the zend_string structure is as
    follows:


    zend_refcounted gc; - zend_ulong h; /* hash
    value */ - int len;
    + zend_uint_t h; /* hash value */
    + zend_size_t len;
    char val[1];

    This means that a zend_string will on average be approximately 4 bytes
    longer. (Approximately because the exact difference is subject to
    details of the allocation process and depends on the string length
    distribution.)

    I think that this difference might be acceptable, if this is the cost
    of uniform string size usage in the codebase. I tested the impact that
    adding 4 bytes on the string structure has on one application and the
    result was a change from 38.2 to 38.5 MiB, which translates to a 0.7%
    difference. That difference is acceptable to me.

    However I don't know how representative that result is and I don't
    currently have time to set up a testing environment for something like
    wordpress on a 64bit vm. I would appreciate it if somebody who already
    has such a setup could run a test for this, to confirm that there is
    indeed no large difference in memory consumption. Just add a "int
    unused" after the len member and compare memory usage.

    So, what I'm suggesting here is the following:


    We implement 64bit integers on LLP64, we implement unsigned sizes, we
    implement size_t for strings lengths. I honestly still don't fully
    understand the necessity for the latter, but if the impact in phpng is
    indeed low (in the 1% range), then I think I can compromise on that.

    What we don't implement is indiscriminate usage of size_t or other
    64bit
    types all over the place (like for linenos, argument counts, argument
    lengths etc) and usage of 64bit sizes for hashtables.

    This would be acceptable for me and I hope that it would also be
    acceptable for others.

    While we're at it, I would also like to quickly touch on the topic of
    naming and renaming. I hope it is clear now that we need both 64bit
    types for some things and 32bit types for others (like HT sizes,
    linenos, etc). As such I would suggest to use zend_(u)long_t as the
    64bit type (on 64bit
    platforms of course) and zend_(u)int_t as the 32bit type. This more or
    less matches our current naming and hopefully avoids confusion.

    With that naming convention, we should also continue using IS_LONG and
    Z_LVAL instead of the suggested IS_INT and Z_IVAL. I'd also keep using
    Z_STRLEN instead of Z_STRSIZE and wouldn't do changes to zpp, especially
    as they conflict with other zpp changes in phpng. I understand why these
    changes were beneficial in the original 64bit patch, however
    considering that the usage of many APIs changed in phpng and extensions
    need to be carefully reviewed anyway, I don't think these renames make a
    lot of sense anymore.

    So, that's my opinion on how we should proceed with the 64bit patch. I
    very much hope that we can reach some consensus about this.

    Credits: This mail is the result of a discussion between Anthony, Bob
    and myself.

    Nikita
    I really appreciate you guys have looked inside the str_size_and_int64
    patch. Dmitry even has compiled and tested it.

    While the Nikitas notes sound pretty meaningful, let me ask you a one
    simple question before proceeding with any details - how much time you
    personally would spend on the integration?

    Thanks

    Anatol
  • Stas Malyshev at May 17, 2014 at 9:09 pm
    Hi!
    Before going any further, it should be established that two aspects of the
    64 bit patch are, as far as I can see, acceptable to everyone: The
    introduction of 64bit integers on Win64 and other LLP64 architectures and
    the use of an unsigned integer type for lengths and sizes.
    I would add a third aspect - using a dedicated set of types to signify
    those, instead of relying on (potentially unportable) mishmash of
    standard ones. This is being overlooked, but I'd like to emphasize that
    I actually like the idea very much - with all the disruption that it
    brings - as a chance to weed out all these false assumptions and sloppy
    types still lurking in the guts of the engine and the extensions. I
    wonder if we can also make clang to tell us on such type conversions
    even if they match on specific system (I know there are hacks to do this
    but they are way uglier than I'd like to use).

    As for the rest, I think this is an excellent proposal and I agree with
    pretty much all of it. I am still not convinced of the need for 64-bit
    string sizes, but if we will be able to deal with much more important
    issues - such as hashtables, internal name lengths (function/class),
    etc. - I don't care, the difference is too small. I am not sure how the
    technical details with zend_string will work out but if we agree that's
    what we're doing and try to find the way to make it happen I'd think it
    worth a serious try.
    --
    Stanislav Malyshev, Software Architect
    SugarCRM: http://www.sugarcrm.com/
    (408)454-6900 ext. 227
  • Guilhermeblanco at May 18, 2014 at 5:22 am
    Hi Nikita,

    I really like the way you write because you put realistic approaches when
    considering facts. I do miss a lot of this in here.
    Like you said, it is completely feasible to add 64bit types where
    applicable and not degrade performance that much as some people are flaming
    internals@ lately.
    As Pierre said on emails earlier on other threads, that initial patch was a
    possible initial approach, not the actual final one since lots of tweaks on
    memory improvement could/would be required to make it optimized as phpng.

    For this exact reason I voted +1 on the initial idea, but not as a final
    patch.

    Cheers,

    On Sat, May 17, 2014 at 5:09 PM, Stas Malyshev wrote:

    Hi!
    Before going any further, it should be established that two aspects of the
    64 bit patch are, as far as I can see, acceptable to everyone: The
    introduction of 64bit integers on Win64 and other LLP64 architectures and
    the use of an unsigned integer type for lengths and sizes.
    I would add a third aspect - using a dedicated set of types to signify
    those, instead of relying on (potentially unportable) mishmash of
    standard ones. This is being overlooked, but I'd like to emphasize that
    I actually like the idea very much - with all the disruption that it
    brings - as a chance to weed out all these false assumptions and sloppy
    types still lurking in the guts of the engine and the extensions. I
    wonder if we can also make clang to tell us on such type conversions
    even if they match on specific system (I know there are hacks to do this
    but they are way uglier than I'd like to use).

    As for the rest, I think this is an excellent proposal and I agree with
    pretty much all of it. I am still not convinced of the need for 64-bit
    string sizes, but if we will be able to deal with much more important
    issues - such as hashtables, internal name lengths (function/class),
    etc. - I don't care, the difference is too small. I am not sure how the
    technical details with zend_string will work out but if we agree that's
    what we're doing and try to find the way to make it happen I'd think it
    worth a serious try.
    --
    Stanislav Malyshev, Software Architect
    SugarCRM: http://www.sugarcrm.com/
    (408)454-6900 ext. 227

    --
    PHP Internals - PHP Runtime Development Mailing List
    To unsubscribe, visit: http://www.php.net/unsub.php

    --
    Guilherme Blanco
    MSN: guilhermeblanco@hotmail.com
    GTalk: guilhermeblanco
    Toronto - ON/Canada
  • Rasmus Lerdorf at May 18, 2014 at 9:54 am

    On 05/17/2014 08:27 PM, Nikita Popov wrote:
    So, that's my opinion on how we should proceed with the 64bit patch. I very
    much hope that we can reach some consensus about this.
    Thanks for pulling this together Nikita. People have been asking me
    about this issue and this was exactly what I told them we would likely
    end up with as it is a sane and reasonable implementation of the two
    efforts. I would guess an rfc vote between the alternatives, as I see
    them, would favour this one by a wide margin.

    As for >4G strings, it does seem unlikely, on current hardware, that you
    would stick that much data in a variable. You might be able to get one
    in there, but if you then do anything with it, the tmp copy is going to
    make things fall over pretty quickly. But who knows in 4-5 years maybe
    having a TB of ram in servers will be the norm, IO channels have become
    much wider and we are manipulating 4K video files directly in PHP so
    perhaps you can make a future-proofing argument there.

    -Rasmus
  • Pierre Joye at May 18, 2014 at 10:13 am

    On Sun, May 18, 2014 at 11:53 AM, Rasmus Lerdorf wrote:
    On 05/17/2014 08:27 PM, Nikita Popov wrote:
    So, that's my opinion on how we should proceed with the 64bit patch. I very
    much hope that we can reach some consensus about this.
    Thanks for pulling this together Nikita. People have been asking me
    about this issue and this was exactly what I told them we would likely
    end up with as it is a sane and reasonable implementation of the two
    efforts. I would guess an rfc vote between the alternatives, as I see
    them, would favour this one by a wide margin.
    It is exactly what we have been advocating during the whole process.
    More about that in a mail I'm writing.

    As for >4G strings, it does seem unlikely, on current hardware, that you
    would stick that much data in a variable. You might be able to get one
    in there, but if you then do anything with it, the tmp copy is going to
    make things fall over pretty quickly. But who knows in 4-5 years maybe
    having a TB of ram in servers will be the norm, IO channels have become
    much wider and we are manipulating 4K video files directly in PHP so
    perhaps you can make a future-proofing argument there.


    The string length is a side effect, not a goal per se, never was. But
    variables, buffers dealing with external libraries or functions are
    sensible areas. A safe implementation will prevent many issues we
    already had, many times. Also correct typing and less (if not none)
    magic casting guessed by the compiler will also drastically increase
    PHP code quality and safety. These two points, and 64bit integer in a
    portable manner, are the top goals of this RFC.

    Cheers,
    --
    Pierre

    @pierrejoye | http://www.libgd.org
  • Rasmus Lerdorf at May 18, 2014 at 11:34 am

    On 05/18/2014 12:13 PM, Pierre Joye wrote:
    On Sun, May 18, 2014 at 11:53 AM, Rasmus Lerdorf wrote:
    On 05/17/2014 08:27 PM, Nikita Popov wrote:
    So, that's my opinion on how we should proceed with the 64bit patch. I very
    much hope that we can reach some consensus about this.
    Thanks for pulling this together Nikita. People have been asking me
    about this issue and this was exactly what I told them we would likely
    end up with as it is a sane and reasonable implementation of the two
    efforts. I would guess an rfc vote between the alternatives, as I see
    them, would favour this one by a wide margin.
    It is exactly what we have been advocating during the whole process.
    More about that in a mail I'm writing.
    As for >4G strings, it does seem unlikely, on current hardware, that you
    would stick that much data in a variable. You might be able to get one
    in there, but if you then do anything with it, the tmp copy is going to
    make things fall over pretty quickly. But who knows in 4-5 years maybe
    having a TB of ram in servers will be the norm, IO channels have become
    much wider and we are manipulating 4K video files directly in PHP so
    perhaps you can make a future-proofing argument there.
    The string length is a side effect, not a goal per se, never was. But
    variables, buffers dealing with external libraries or functions are
    sensible areas. A safe implementation will prevent many issues we
    already had, many times. Also correct typing and less (if not none)
    magic casting guessed by the compiler will also drastically increase
    PHP code quality and safety. These two points, and 64bit integer in a
    portable manner, are the top goals of this RFC.
    You keep talking about side effects of the patch. It is these side
    effects that we have a problem with. We need a version of the patch that
    doesn't have all these undesired side effects.

    You put the patch as it stands up for a vote and it is this patch we
    have to vote on which is why I am voting no on it. It isn't some future
    version with some unknown number of side effects removed.

    -Rasmus
  • Pierre Joye at May 18, 2014 at 4:40 pm

    On Sun, May 18, 2014 at 1:34 PM, Rasmus Lerdorf wrote:

    You keep talking about side effects of the patch.
    No, what I have to keep saying, because this argument keeps coming, is
    that the ability to store >2GB string is a side effect and not a goal
    of this patch.
    It is these side
    effects that we have a problem with. We need a version of the patch that
    doesn't have all these undesired side effects.
    The only one, and it is disputable, is the memory usage, which is
    around 4% under real world usage (WP, Symfony, Drupal, and some other
    apps). It even performs better in many common situations.
    You put the patch as it stands up for a vote and it is this patch we
    have to vote on which is why I am voting no on it. It isn't some future
    version with some unknown number of side effects removed.
    I am not sure how to formulate it in a better way but let me try again.

    - 64bit patch has been worked on and stabilized since months
    - It is continuously tested using master, 5.5 and 5.6, using our full
    tests suite (which is much more than just running phpt in cli if you
    remember)
    - the patch is stable

    Given these points, and as it is the case with every new
    features/addition/improvements we have made, tweaks, optimization,
    etc. will happen after it gets approved and applied. Stability and
    correctness have the priority.

    Asking to take care of phpng was hardly possible given its sudden
    public availability. However, we know that phpng can bring PHP a lot
    and that is why we gave the option to merge the 64bit patch to phpng,
    once it is gets somehow usable in common situations (estimation: 1-2
    months if not critical bugs exist in common extensions, but can't test
    them in a good way as of now).

    I will post a mail about what we plan to do and how, this pretty much
    matches what Nikita said and what I have been said since the very day
    after we proposed the 64 bit patch again. Most likely tonight or
    tomorrow morning.

    Cheers,
    --
    Pierre

    @pierrejoye | http://www.libgd.org
  • Ferenc Kovacs at May 18, 2014 at 6:17 pm

    On Sun, May 18, 2014 at 6:39 PM, Pierre Joye wrote:
    On Sun, May 18, 2014 at 1:34 PM, Rasmus Lerdorf wrote:

    You keep talking about side effects of the patch.
    No, what I have to keep saying, because this argument keeps coming, is
    that the ability to store >2GB string is a side effect and not a goal
    of this patch.
    It is these side
    effects that we have a problem with. We need a version of the patch that
    doesn't have all these undesired side effects.
    The only one, and it is disputable, is the memory usage, which is
    around 4% under real world usage (WP, Symfony, Drupal, and some other
    apps). It even performs better in many common situations.
    You put the patch as it stands up for a vote and it is this patch we
    have to vote on which is why I am voting no on it. It isn't some future
    version with some unknown number of side effects removed.
    I am not sure how to formulate it in a better way but let me try again.

    - 64bit patch has been worked on and stabilized since months
    - It is continuously tested using master, 5.5 and 5.6, using our full
    tests suite (which is much more than just running phpt in cli if you
    remember)
    - the patch is stable

    Given these points, and as it is the case with every new
    features/addition/improvements we have made, tweaks, optimization,
    etc. will happen after it gets approved and applied. Stability and
    correctness have the priority.

    Asking to take care of phpng was hardly possible given its sudden
    public availability. However, we know that phpng can bring PHP a lot
    and that is why we gave the option to merge the 64bit patch to phpng,
    once it is gets somehow usable in common situations (estimation: 1-2
    months if not critical bugs exist in common extensions, but can't test
    them in a good way as of now).

    I will post a mail about what we plan to do and how, this pretty much
    matches what Nikita said and what I have been said since the very day
    after we proposed the 64 bit patch again. Most likely tonight or
    tomorrow morning.

    Cheers,
    --
    Pierre

    @pierrejoye | http://www.libgd.org

    --
    PHP Internals - PHP Runtime Development Mailing List
    To unsubscribe, visit: http://www.php.net/unsub.php
    Hi Pierre,

    Thanks for the reply, based on your promise, I've voted yes to the RFC.
    I think that we could have saved a bunch of arguing back and forth if we
    would have sorted out the side-effects first (or at least explicitly state
    that you guys are ok with dropping/willing to drop those for measurable
    performance gains), but I'm glad that we were able to reach an agreement
    eventually.

    --
    Ferenc Kovács
    @Tyr43l - http://tyrael.hu
  • Rasmus Lerdorf at May 19, 2014 at 4:48 pm

    On 5/18/14, 9:39 AM, Pierre Joye wrote:
    On Sun, May 18, 2014 at 1:34 PM, Rasmus Lerdorf wrote:

    You keep talking about side effects of the patch.
    No, what I have to keep saying, because this argument keeps coming, is
    that the ability to store >2GB string is a side effect and not a goal
    of this patch.
    It is these side
    effects that we have a problem with. We need a version of the patch that
    doesn't have all these undesired side effects.
    The only one, and it is disputable, is the memory usage, which is
    around 4% under real world usage (WP, Symfony, Drupal, and some other
    apps). It even performs better in many common situations.
    You put the patch as it stands up for a vote and it is this patch we
    have to vote on which is why I am voting no on it. It isn't some future
    version with some unknown number of side effects removed.
    I am not sure how to formulate it in a better way but let me try again.

    - 64bit patch has been worked on and stabilized since months
    - It is continuously tested using master, 5.5 and 5.6, using our full
    tests suite (which is much more than just running phpt in cli if you
    remember)
    - the patch is stable

    Given these points, and as it is the case with every new
    features/addition/improvements we have made, tweaks, optimization,
    etc. will happen after it gets approved and applied. Stability and
    correctness have the priority.
    But that is for minor tweaks and optimizations. In this case the way to
    optimize the patch is to undo the 64-bitness in a number of places where
    it doesn't make sense. Putting in a software-imposed limit on class size
    names while still keeping it a 64-bit value in the struct makes no
    sense, for example. Same goes for lineno, line_start, line_end, num_args
    and a couple more that Nikita pointed out.

    And as far as I am concerned this has nothing to do with phpng. I'd
    still be voting no on it as a 4% memory increase, which, by the way, you
    don't even mention in the impacts section, is still too high for me when
    I know parts of the 4% are completely unnecessary.

    -Rasmus
  • Pierre Joye at May 19, 2014 at 4:52 pm

    On Mon, May 19, 2014 at 6:48 PM, Rasmus Lerdorf wrote:

    But that is for minor tweaks and optimizations. In this case the way to
    optimize the patch is to undo the 64-bitness in a number of places where
    it doesn't make sense. Putting in a software-imposed limit on class size
    names while still keeping it a 64-bit value in the struct makes no
    sense, for example. Same goes for lineno, line_start, line_end, num_args
    and a couple more that Nikita pointed out.
    That's not what we discussed.
    And as far as I am concerned this has nothing to do with phpng. I'd
    still be voting no on it as a 4% memory increase, which, by the way, you
    don't even mention in the impacts section, is still too high for me when
    I know parts of the 4% are completely unnecessary.
    We answer that already, be from Nikita, Dmitry or I. And yes, we agree
    on these points already.

    Cheers,
    --
    Pierre

    @pierrejoye | http://www.libgd.org
  • Rasmus Lerdorf at May 19, 2014 at 4:54 pm

    On 5/19/14, 9:52 AM, Pierre Joye wrote:
    On Mon, May 19, 2014 at 6:48 PM, Rasmus Lerdorf wrote:

    But that is for minor tweaks and optimizations. In this case the way to
    optimize the patch is to undo the 64-bitness in a number of places where
    it doesn't make sense. Putting in a software-imposed limit on class size
    names while still keeping it a 64-bit value in the struct makes no
    sense, for example. Same goes for lineno, line_start, line_end, num_args
    and a couple more that Nikita pointed out.
    That's not what we discussed.
    And as far as I am concerned this has nothing to do with phpng. I'd
    still be voting no on it as a 4% memory increase, which, by the way, you
    don't even mention in the impacts section, is still too high for me when
    I know parts of the 4% are completely unnecessary.
    We answer that already, be from Nikita, Dmitry or I. And yes, we agree
    on these points already.
    Ok, then please update the RFC with what you see as the way forward,
    including adding actual memory impacts to it and restart the vote when
    the RFC is ready.

    -Rasmus
  • Pierre Joye at May 19, 2014 at 5:03 pm

    On Mon, May 19, 2014 at 6:54 PM, Rasmus Lerdorf wrote:

    Ok, then please update the RFC with what you see as the way forward,
    including adding actual memory impacts to it and restart the vote when
    the RFC is ready.
    Asking us, after months of discussions, work, tests, publications,
    etc. to do change that could be done post acceptation easily, as it is
    always the case for any change affecting more than a couple of LoC.
    And with which guarantee? So far none, even the contrary, all
    possible ways are used too block this RFC, while we keep trying to
    find compromises and keep helping phpng moving forward in the
    meantime. I hope you can understand that I have hard time to justify
    more efforts on this patch at this stage, we are not an endless free
    resource.

    At some point trust, compromise and common sense must be followed. And
    we are at this point here. We said numerous time on this list and IRC
    what are the goals and what are the possibilities of improving both
    the 64bit patch and phpng in the coming months/years. Let end this
    unproductive debate and let us back to code, for the good of PHP.

    Cheers,
    --
    Pierre

    @pierrejoye | http://www.libgd.org
  • Anatol Belski at May 19, 2014 at 8:56 pm

    On Mon, May 19, 2014 18:54, Rasmus Lerdorf wrote:
    On 5/19/14, 9:52 AM, Pierre Joye wrote:

    On Mon, May 19, 2014 at 6:48 PM, Rasmus Lerdorf <rasmus@lerdorf.com>
    wrote:

    But that is for minor tweaks and optimizations. In this case the way
    to optimize the patch is to undo the 64-bitness in a number of places
    where it doesn't make sense. Putting in a software-imposed limit on
    class size names while still keeping it a 64-bit value in the struct
    makes no sense, for example. Same goes for lineno, line_start,
    line_end, num_args and a couple more that Nikita pointed out.
    That's not what we discussed.

    And as far as I am concerned this has nothing to do with phpng. I'd
    still be voting no on it as a 4% memory increase, which, by the way,
    you don't even mention in the impacts section, is still too high for
    me when I know parts of the 4% are completely unnecessary.
    We answer that already, be from Nikita, Dmitry or I. And yes, we agree
    on these points already.
    Ok, then please update the RFC with what you see as the way forward,
    including adding actual memory impacts to it and restart the vote when the
    RFC is ready.


    -Rasmus
    Confused about what is happening. I thought we reached the agreement based
    on what Nikita has suggested, which is pretty simple.

    The points of the current patch which have to stay

    1. 64 bit int in zval(no mem issue, no perf issue)
    2. size_t in zend_string (.8% mem issue, no perf issue)
    3. use 32 bit string length as much as possible everywhere else

    for myself i'd add also
    - several ini options (like content, post, etc length, memory_limit and
    several other)
    - file cursor and stream positions (off_t family)
    That doesn't affect mem consumption much as Dmitry already mentioned.

    Everything else, say

    literal
    ini
    stack
    header
    lineno
    class names in structs
    path length in structs
    hash
    etc.

    has to be adjusted with 32 bit string length while merging with the phpng
    branch.

    So question - why there are voices like "you have to implement it for
    phpng and rewrite the RFC"? If there's the consent to take this RFC with
    regard to improvement suggestion listed in short above, so what is wrong
    again? Wasn't it said we do it collectively?

    Have a nice day.

    Anatol
  • Rasmus Lerdorf at May 19, 2014 at 9:45 pm

    On 5/19/14, 1:55 PM, Anatol Belski wrote:
    On Mon, May 19, 2014 18:54, Rasmus Lerdorf wrote:
    On 5/19/14, 9:52 AM, Pierre Joye wrote:

    On Mon, May 19, 2014 at 6:48 PM, Rasmus Lerdorf <rasmus@lerdorf.com>
    wrote:

    But that is for minor tweaks and optimizations. In this case the way
    to optimize the patch is to undo the 64-bitness in a number of places
    where it doesn't make sense. Putting in a software-imposed limit on
    class size names while still keeping it a 64-bit value in the struct
    makes no sense, for example. Same goes for lineno, line_start,
    line_end, num_args and a couple more that Nikita pointed out.
    That's not what we discussed.

    And as far as I am concerned this has nothing to do with phpng. I'd
    still be voting no on it as a 4% memory increase, which, by the way,
    you don't even mention in the impacts section, is still too high for
    me when I know parts of the 4% are completely unnecessary.
    We answer that already, be from Nikita, Dmitry or I. And yes, we agree
    on these points already.
    Ok, then please update the RFC with what you see as the way forward,
    including adding actual memory impacts to it and restart the vote when the
    RFC is ready.


    -Rasmus
    Confused about what is happening. I thought we reached the agreement based
    on what Nikita has suggested, which is pretty simple.

    The points of the current patch which have to stay

    1. 64 bit int in zval(no mem issue, no perf issue)
    2. size_t in zend_string (.8% mem issue, no perf issue)
    3. use 32 bit string length as much as possible everywhere else

    for myself i'd add also
    - several ini options (like content, post, etc length, memory_limit and
    several other)
    - file cursor and stream positions (off_t family)
    That doesn't affect mem consumption much as Dmitry already mentioned.

    Everything else, say

    literal
    ini
    stack
    header
    lineno
    class names in structs
    path length in structs
    hash
    etc.

    has to be adjusted with 32 bit string length while merging with the phpng
    branch.

    So question - why there are voices like "you have to implement it for
    phpng and rewrite the RFC"? If there's the consent to take this RFC with
    regard to improvement suggestion listed in short above, so what is wrong
    again? Wasn't it said we do it collectively?
    I am fine saying we have a consensus on doing it this way, but it is
    quite different from what the current RFC says. At least make a note of
    this in the RFC then since many people use accepted RFCs as a resource
    later on to figure out what has changed.

    -Rasmus
  • Derick Rethans at May 20, 2014 at 4:17 pm

    On Mon, 19 May 2014, Anatol Belski wrote:

    Confused about what is happening. I thought we reached the agreement
    based on what Nikita has suggested, which is pretty simple.
    I'm confused too, mostly because with the ML problems, I missed a lot of
    the discussion. I think it would be best to close the current RFC and
    voting, make sure there is a new "correct" RFC and restart the vote. If
    all the concerns that are raised by various people have been included,
    then, I think that's a much clearer way to figure out what we'd be
    voting for.

    cheers,
    Derick

    --
    http://derickrethans.nl | http://xdebug.org
    Like Xdebug? Consider a donation: http://xdebug.org/donate.php
    twitter: @derickr and @xdebug
    Posted with an email client that doesn't mangle email: alpine
  • David Soria Parra at May 20, 2014 at 6:18 pm

    Derick Rethans schrieb:
    On Mon, 19 May 2014, Anatol Belski wrote:

    Confused about what is happening. I thought we reached the agreement
    based on what Nikita has suggested, which is pretty simple.
    I'm confused too, mostly because with the ML problems, I missed a lot of
    the discussion. I think it would be best to close the current RFC and
    voting, make sure there is a new "correct" RFC and restart the vote. If
    all the concerns that are raised by various people have been included,
    then, I think that's a much clearer way to figure out what we'd be
    voting for.
    I agree. It is rather confusing and I have a hard time keeping up with what
    the current agreement is. Can we please stop the votes on the current RFC
    work on it and vote again. I might want to remind people that it is a
    Request for Comments. Comments we got and now it's time to integrate them
    before rushing into a vote, which is why I haven't voted neither yes or no
    so far. However I cannot vote "yes" on the current state of the RFC.
  • Zeev Suraski at May 20, 2014 at 6:25 pm

    On 20 במאי 2014, at 19:18, Derick Rethans wrote:
    On Mon, 19 May 2014, Anatol Belski wrote:

    Confused about what is happening. I thought we reached the agreement
    based on what Nikita has suggested, which is pretty simple.
    I'm confused too, mostly because with the ML problems, I missed a lot of
    the discussion. I think it would be best to close the current RFC and
    voting, make sure there is a new "correct" RFC and restart the vote. If
    all the concerns that are raised by various people have been included,
    then, I think that's a much clearer way to figure out what we'd be
    voting for.
    +1

    I think we have pretty much consensus but not around the current RFC text.
  • Derick Rethans at May 28, 2014 at 11:38 am

    On Tue, 20 May 2014, Derick Rethans wrote:
    On Mon, 19 May 2014, Anatol Belski wrote:

    Confused about what is happening. I thought we reached the agreement
    based on what Nikita has suggested, which is pretty simple.
    I'm confused too, mostly because with the ML problems, I missed a lot
    of the discussion. I think it would be best to close the current RFC
    and voting, make sure there is a new "correct" RFC and restart the
    vote. If all the concerns that are raised by various people have been
    included, then, I think that's a much clearer way to figure out what
    we'd be voting for.
    Did anything happen with this?

    cheers,
    Derick

    --
    http://derickrethans.nl | http://xdebug.org
    Like Xdebug? Consider a donation: http://xdebug.org/donate.php
    twitter: @derickr and @xdebug
    Posted with an email client that doesn't mangle email: alpine
  • Kris Craig at May 28, 2014 at 11:55 am


    Did anything happen with this?
    Looks like it passed, 24 votes in favor to 21 against (53%). Voting
    expired on May 26th.

    --Kris
  • Pierre Joye at May 28, 2014 at 12:04 pm

    On May 28, 2014 1:38 PM, "Derick Rethans" wrote:
    On Tue, 20 May 2014, Derick Rethans wrote:
    On Mon, 19 May 2014, Anatol Belski wrote:

    Confused about what is happening. I thought we reached the agreement
    based on what Nikita has suggested, which is pretty simple.
    I'm confused too, mostly because with the ML problems, I missed a lot
    of the discussion. I think it would be best to close the current RFC
    and voting, make sure there is a new "correct" RFC and restart the
    vote. If all the concerns that are raised by various people have been
    included, then, I think that's a much clearer way to figure out what
    we'd be voting for.
    Did anything happen with this?
    Waiting that the ML is fixed to push an updated rfc and patch.
  • Derick Rethans at May 28, 2014 at 12:07 pm

    On Wed, 28 May 2014, Pierre Joye wrote:
    On May 28, 2014 1:38 PM, "Derick Rethans" wrote:
    On Tue, 20 May 2014, Derick Rethans wrote:
    On Mon, 19 May 2014, Anatol Belski wrote:

    Confused about what is happening. I thought we reached the
    agreement based on what Nikita has suggested, which is pretty
    simple.
    I'm confused too, mostly because with the ML problems, I missed a
    lot of the discussion. I think it would be best to close the
    current RFC and voting, make sure there is a new "correct" RFC and
    restart the vote. If all the concerns that are raised by various
    people have been included, then, I think that's a much clearer way
    to figure out what we'd be voting for.
    Did anything happen with this?
    Waiting that the ML is fixed to push an updated rfc and patch.
    That makes sense :-) I pinged Wez, lets see if he can help fixing the
    ML.

    cheers,
    Derick

    --
    http://derickrethans.nl | http://xdebug.org
    Like Xdebug? Consider a donation: http://xdebug.org/donate.php
    twitter: @derickr and @xdebug
    Posted with an email client that doesn't mangle email: alpine
  • Michael Wallner at May 28, 2014 at 12:14 pm

    On 28 May 2014 14:07, Derick Rethans wrote:
    On Wed, 28 May 2014, Pierre Joye wrote:

    Waiting that the ML is fixed to push an updated rfc and patch.
    That makes sense :-) I pinged Wez, lets see if he can help fixing the
    ML.
    Is it the ML or the MTA? I just got an RDNS failure back from a test
    message I sent a few days back.


    --
    Regards,
    Mike
  • Derick Rethans at May 28, 2014 at 12:50 pm

    On Wed, 28 May 2014, Michael Wallner wrote:
    On 28 May 2014 14:07, Derick Rethans wrote:
    On Wed, 28 May 2014, Pierre Joye wrote:

    Waiting that the ML is fixed to push an updated rfc and patch.
    That makes sense :-) I pinged Wez, lets see if he can help fixing the
    ML.
    Is it the ML or the MTA? I just got an RDNS failure back from a test
    message I sent a few days back.
    Not sure if it's ML or MTA—sorry.

    cheers,
    Derick

    --
    http://derickrethans.nl | http://xdebug.org
    Like Xdebug? Consider a donation: http://xdebug.org/donate.php
    twitter: @derickr and @xdebug
    Posted with an email client that doesn't mangle email: alpine
  • Ferenc Kovacs at May 28, 2014 at 12:54 pm

    On Wed, May 28, 2014 at 2:49 PM, Derick Rethans wrote:
    On Wed, 28 May 2014, Michael Wallner wrote:
    On 28 May 2014 14:07, Derick Rethans wrote:
    On Wed, 28 May 2014, Pierre Joye wrote:

    Waiting that the ML is fixed to push an updated rfc and patch.
    That makes sense :-) I pinged Wez, lets see if he can help fixing the
    ML.
    Is it the ML or the MTA? I just got an RDNS failure back from a test
    message I sent a few days back.
    Not sure if it's ML or MTA—sorry.

    cheers,
    Derick

    --
    http://derickrethans.nl | http://xdebug.org
    Like Xdebug? Consider a donation: http://xdebug.org/donate.php
    twitter: @derickr and @xdebug
    Posted with an email client that doesn't mangle email: alpine

    --
    PHP Internals - PHP Runtime Development Mailing List
    To unsubscribe, visit: http://www.php.net/unsub.php

    the last info was that the mailing list on lists.php.net tries a local
    delivery for @php.net addresses instead of sending it back to the php.netMX.


    --
    Ferenc Kovács
    @Tyr43l - http://tyrael.hu
  • Zeev Suraski at May 18, 2014 at 7:02 pm

    On 18 במאי 2014, at 14:35, Rasmus Lerdorf wrote:

    You keep talking about side effects of the patch. It is these side
    effects that we have a problem with. We need a version of the patch that
    doesn't have all these undesired side effects.

    You put the patch as it stands up for a vote and it is this patch we
    have to vote on which is why I am voting no on it. It isn't some future
    version with some unknown number of side effects removed.
    Well put. In the strangeness of voting on an implementation RFC, I
    don't see how it makes sense to talk about "side effects". They're
    the very essence of the patch and the reason it's being opposed.

    If there's a better way to so it, let's see it. The sensible thing to
    do would be implementing it for phpng, seeing a *final* version of the
    patch and it's consequences, and then making an informed decision.

    Even without it, Nikita's proposal seems as a much better baseline
    everyone can agree on.

    Zeev
  • Pierre Joye at May 19, 2014 at 9:05 am

    On Sun, May 18, 2014 at 9:01 PM, Zeev Suraski wrote:

    Well put. In the strangeness of voting on an implementation RFC, I
    don't see how it makes sense to talk about "side effects". They're
    the very essence of the patch and the reason it's being opposed.

    If there's a better way to so it, let's see it. The sensible thing to
    do would be implementing it for phpng, seeing a *final* version of the
    patch and it's consequences, and then making an informed decision.
    If I get your point correctly, you are asking to provide the final
    version of a patch against phpng, which is not even in alpha stage,
    mvoing target and APIs, etc. That's not possible.
    Even without it, Nikita's proposal seems as a much better baseline
    everyone can agree on.
    It would help if you actually read replies instead of systematically
    negate this proposal and how things are supposed to work here. Let
    alone the calls for -1 based on wrong numbers and facts. This is wrong
    in so many ways.

    Now, what would have happened if we did what you are saying for
    opcache? It would most likely not be in the voting phase if we
    consider all supported platforms or changes happening since the votes
    began. It would be time to be a little bit serious and consistent if
    you really want to agree on something.

    Cheers,
    --
    Pierre

    @pierrejoye | http://www.libgd.org

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupphp-internals @
categoriesphp
postedMay 17, '14 at 6:27p
activeMay 28, '14 at 12:54p
posts34
users14
websitephp.net

People

Translate

site design / logo © 2022 Grokbase