[RFC] Improve language coherence for the behaviour of offsets and containers: object handlers

[Girgias]

Part of https://wiki.php.net/rfc/container-offset-behaviour

This PR mainly deals with the object handler part of the RFC.

[iluuu1994]

The benchmark shows a degradation larger than I expected (0.22% for Symfony). Any idea where this is coming from?

[Girgias]

The benchmark shows a degradation larger than I expected (0.22% for Symfony). Any idea where this is coming from?

Some ideas that might cause this:

• The new algorithm for isset() which means we are calling more userland functions
• Not caching the function pointers in something like the arrayaccess_funcs_ptr struct, so we always need to look them up
• Opcache not really being implemented, as the benchmarking CI pipeline used to segfault prior to me doing the minimal opcache changes

[morrisonlevi]

I've looked at a few places and didn't see any. Does the engine ever call has_dimension without ever calling one of the other handlers? It seems like it's always followed by one.

Potentially, we should stop calling has_dimension and rely on the error handling that still seems to exist in the other dimension handlers. It certainly looks like they cannot assume that they are only called when has_dimension is true. And in the case of hash lookups, the lookup is going to get repeated if we call has_dimension and read_dimension, for example.

[Girgias]

I don't think it's reasonable to "just" call the read_dimension and not call the had_dimension handler, as this would be confusing for users.

I suppose what I could do is what ArrayAccess used to do is cache the zend_function pointers so we don't need to look them up.

[morrisonlevi]

I don't think it's reasonable to "just" call the read_dimension and not call the had_dimension handler, as this would be confusing for users.

Can you expand? What's confusing?

[Girgias]

The following class and code should not cause any warnings (or even exceptions) to be emitted.

I don't see how you'd ensure no warnings (which may or may not be promoted to exceptions) or exceptions are being thrown in this instance.

class Map implements DimensionReadable {
    private array $map = [];

    public function offsetGet(mixed $offset): mixed {
        return $this->map[$offset];
    }

    public function offsetExists(mixed $offset): bool {
        return array_key_exists($offset, $this->map);
    }
}

$map = new Map();
var_dump(isset($map['undef']));

[morrisonlevi]

I don't mean userland, I mean internally. There are C-level calls to has_dimension which get followed up by calls to things like read_dimension. Note that in your example the hash lookup happens twice. Internal calls should be able to avoid that, I think. Or at least, I would try to design the APIs that way. If read_dimension returns NULL, it clearly did not have that dimension. See what I'm saying? Look at zend_weakmap_has_dimension and zend_weakmap_read_dimension for what I mean; it's effectively doing all the same work twice if you call has_dimension before read_dimension. Should be able to coalesce the two, for internal handlers (not userland interface methods).

[Girgias]

The difference between a C handler and the userland function is basically non-existent, having it be different is just confusing.

The hash look_up would have only happened if you were calling the userland version anyway and did not have an optimized C version of the handler.

The API is literally just has_dimension == call to offsetExists(), read_dimension == call to offsetGet(), etc.
But all the internal extensions basically implement an overloaded C version which does not call a userland function.

Also about the hash look-up, this should be a non-issue now as I decided to use a union to either store the zend_function* for userland classes or the C handler function for internal extensions.

[SakiTakamachi]

PDO part looks good to me.