Re: [PHP-DEV] Re: 64 bit string offsets

On Mon, September 1, 2014 19:07, Anatol Belski wrote:
> Hi Dmitry,
>
>
> On Mon, September 1, 2014 16:17, Dmitry Stogov wrote:
>
>> Hi Anatol,
>>
>>
>>
>> what do you mean? heap allocated structure? I think, it's not a good
>> option :(
>>
>> I didn't have time to think about this yet.
>>
>>
> I thought about creating an extended zend_string struct with an appended
> offset member. Pointer to that structure could be injected into _zend_value
> and were compatible with the normal zend_string and would be used only for
> the offset case. But then, we could get rid of u2.str_offset and change
> the meaning of the IS_STR_OFFSET related macros.
>
> That struct could be initialized the same way as any other in an opcode.
> However I have yet to figure out where in the compiler this has to happen
> (somewhere near the zend_compile_assign() i suppose).
>
>
So, the approach described here isn't worky. While trying to implement it,
I saw that it's not possible to isolate the exact case on compile time. A
syntax like $s[0] = 's' can be related to too many things and on
compilation time are abstract. Say $s = 'abc' is put somewhere in the
script, but $s[0] = 'd' can be right the next statement or x lines later.
This kind of things can be only catched on execution time.

And, while working on that, I've realized that usage of
zend_assign_to_string_offset() in the ASSIGN opcode is a dead case. So
effectively, the only opcode where it has to be handled is ASSIGN_DIM.
Also, I've rethought the solution - previously I was trying to integrate
some overloaded struct to carry the correct offset. After I saw that's
only one place where the actualy offset assignment can happen, I've
changed the tactics. Now, reading the offset directly on execution time
and passing to zend_assign_to_string_offset(). This one has no impact
neither in perf nor in memory. No new structs or heap allocations, just a
slightly modified flow.

http://belski.net/phpz/64_str_offset_better.patch

Please take a look. I'd suggest to give this last patch a try and would
apply this WE if there's no objection.

Regards

anatol