Hi Larry Garfield,
> >> Returning $this would resolve that, I think. (Making it return a new,
> >> immutable copy of the Deque would be even nicer, but I realize that's
> >> probably not an argument I'm going to win at this point on this RFC.)
> >
> > Technically, you still can have single-expression usages in
> > readable/unreadable ways
> >
> > - `[$deque->shift('value'), $deque][1]`, or
> > - `($deque->shift('value') ?: $deque)`, or
> > - `my_userland_helper_shift_and_return($deque, 'value')`
>
> ... Those are all grotesque.
True, I wasn't familiar with that event and whether your goal was to write as
few lines as possible or as quickly as possible vs in a readable way to people
familiar with php.
That clears that up.
> > My personal preference is against making this fluent.
> > I'd rather expose an efficient datastructure that's consistent with the
> > rest of PHP's functionality to the extent possible,
> > which userland can use to write their own fluent/non-fluent classes.
> > There's drawbacks to returning `$this`, including:
> >
> > 1. Inconsistency with existing APIs making remembering what does what
> > harder. Barely anything in php that I remember returns $this.
> >
> > https://www.php.net/manual/en/arrayobject.append.php returns void.
> >
> > https://www.php.net/manual/en/function.array-push returns an int.
>
> So it's already inconsistent, and frankly neither of those returns is useful.
> If the existing convention is "inconsistently unhelpful", then let's go
> ahead and make it helpful since "it's lame but at least it's consistent" is
> not an argument here. (Sometimes that is a valid argument, but not in this
> case.)
I won't know if anyone is strongly against fluent interfaces belonging in core,
or strongly against adopting fluent interfaces in an ad-hoc way that would make
it unclear if the vote was for/against the overall functionality or the new
design choice.
I'd rather not do this without a policy RFC such as the one that passed for
namespaces https://wiki.php.net/rfc/namespaces_in_bundled_extensions
If you expect this to be widely considered a better design php should adopt,
please create an RFC after the vote finishes (if it passes) before the feature
freeze outlining which methods of `Collection\Deque` would change and how.
> > 2. Inconsistency with possible new datastructures/methods
> >
> > If a `Map`/`Set` function were to be added, then methods for
> > add/remove would return booleans (or the old value), not $this
>
> Removal methods need to return the value being removed, sure. That makes
> sense across the board. But I don't see why Map::add($k, $v) or Set::add($v)
> shouldn't also return $this. I would make the exact same ask there, for the
> exact same reason: To aid functional code, which (when the syntax is
> conducive to it) can be more readable in many cases than procedural
> approaches.
>
> > 3. Slight additional performance overhead for functionality I assume
> > will be used relatively infrequently
>
> I would assume the opposite, at least in my own code. I'm writing a lot of
> recursive or reduction-based routines these days, so if I had reason to use a
> queue/stack in them in the first place, they'd almost certainly get used in
> this fashion.
>
> > (php has to increment reference counts and opcache can't eliminate
> > the opcode to decrease reference counts and possibly free the return
> > value of `$deque->shift()` with the return type info being an object)
>
> I cannot speak to this one; how much of a difference is it in practice?
Adding `RETURN_OBJ_COPY(Z_OBJ_P(ZEND_THIS));` and a different method with the
`: Deque` return type:
Around 6-8% for large arrays where push is the most frequent method call and
the array reads are taken out since they'd be the same?
(and similar for shift, I'd expect)
(Intel CPU security mitigations/power management and so on have made
benchmarking less convenient, I ran the functions twice to see if it was
consistent)
I'd still object to this for other stated reasons even if you think 6-8% is
worth it for your use cases for this or future additions to php in general.
```
Results for php 8.2.0-dev debug=false with opcache enabled=true
Appending to Collections\Deque []= : n= 4 iterations=10000000,
create+destroy time=1.204 result=0
Appending to Collections\Deque push : n= 4 iterations=10000000,
create+destroy time=1.551 result=0
Appending to Collections\Deque fluent: n= 4 iterations=10000000,
create+destroy time=1.640 result=0
Appending to Collections\Deque []= : n= 4 iterations=10000000,
create+destroy time=1.213 result=0
Appending to Collections\Deque push : n= 4 iterations=10000000,
create+destroy time=1.549 result=0
Appending to Collections\Deque fluent: n= 4 iterations=10000000,
create+destroy time=1.636 result=0
Appending to Collections\Deque []= : n= 8 iterations= 5000000,
create+destroy time=0.923 result=0
Appending to Collections\Deque push : n= 8 iterations= 5000000,
create+destroy time=1.263 result=0
Appending to Collections\Deque fluent: n= 8 iterations= 5000000,
create+destroy time=1.335 result=0
Appending to Collections\Deque []= : n= 8 iterations= 5000000,
create+destroy time=0.921 result=0
Appending to Collections\Deque push : n= 8 iterations= 5000000,
create+destroy time=1.261 result=0
Appending to Collections\Deque fluent: n= 8 iterations= 5000000,
create+destroy time=1.335 result=0
Appending to Collections\Deque []= : n= 1024 iterations= 100000,
create+destroy time=1.246 result=0
Appending to Collections\Deque push : n= 1024 iterations= 100000,
create+destroy time=1.972 result=0
Appending to Collections\Deque fluent: n= 1024 iterations= 100000,
create+destroy time=2.150 result=0
Appending to Collections\Deque []= : n= 1024 iterations= 100000,
create+destroy time=1.251 result=0
Appending to Collections\Deque push : n= 1024 iterations= 100000,
create+destroy time=1.969 result=0
Appending to Collections\Deque fluent: n= 1024 iterations= 100000,
create+destroy time=2.146 result=0
Appending to Collections\Deque []= : n= 1048576 iterations= 100,
create+destroy time=2.450 result=0
Appending to Collections\Deque push : n= 1048576 iterations= 100,
create+destroy time=3.172 result=0
Appending to Collections\Deque fluent: n= 1048576 iterations= 100,
create+destroy time=3.371 result=0
Appending to Collections\Deque []= : n= 1048576 iterations= 100,
create+destroy time=2.455 result=0
Appending to Collections\Deque push : n= 1048576 iterations= 100,
create+destroy time=3.183 result=0
Appending to Collections\Deque fluent: n= 1048576 iterations= 100,
create+destroy time=3.376 result=0
```
> > 4. Returning $this makes code easier to write at some cost to
> > readability - Developers new to php or using `Collections\Deque` for
> > the first time would not immediately know what the code they're reading
> > is doing.
> > (less of a problem with a good IDE, typechecker, and a typed
> > codebase, but this isn't universal)
> > Having it return void, `return $deque->push()` would be less common
> > and this would force the meaning to be clear.
>
> I disagree that it's less readable. Compare it with the alternatives you
> showed at the start of your reply. None of those would qualify as
> "readable", IMO.
True, I wasn't sure if your goal was readability (for php developers deeply
familiar with libraries) or to write as few lines as possible.
Suggesting using `function` and adding several lines seemed like something you
were trying to avoid.
> > Developers might have several guesses/assumptions based on their
> > experience with other methods in php/elsewhere
> >
> > - It returns the new count (JavaScript Array.push, array_push)
> > - It returns $this (Ruby)
> > - It returns a lazy copy, like you'd wanted, not modifying the
> > original
> > - It's returning void and the code in question is shorthand for
> > `return null`.
> > (Python, C++
> > https://www.cplusplus.com/reference/vector/vector/push_back/ ,
> > offsetSet and spl push()/shift() methods)
>
> Once again, the lack of a consistent pattern means we don't need to follow
> the pattern. As with the first point, "it's silly but at least it's
> consistent" is a valid argument in many cases, and we've changed the language
> accordingly before. (Ternary associativity order comes to mind.) That's not
> the case here. There is no broad-based ingrained training that people have
> that would lead them to expect a given return, so we can and should go for
> whichever one is most powerful/flexible/conducive to good code. IMO, that's
> returning $this.
I continue to disagree for my previously stated reasons.
> (Again, since a purely functional version is apparently off the table.)
I'd be in favor of immutable datastructures in core, but I don't think I'd have
much success in an RFC.
I've only heard from a few voters that are passionately for them, I'm not sure
if that translates to widespread interest.
Though interestingly (and unrelated to this RFC https://wiki.php.net/rfc/deque
), they do seem possible to implement with better asymptotic performance than
I'd have expected.
Google searching mentioned Relaxed Radix Binary Trees, an immutable
datastructure surprisingly claiming performance near arrays in most operations.
https://hypirion.com/musings/thesis https://github.com/hyPiRion/c-rrb
Aside: An Immutable Map/Set would probably have a stronger argument than
immutable sequeunces/deques/linked lists (arrays support copy on write already
so the original array is immutable, but arbitrary keys are something arrays
don't support),
but voters that didn't see a use case for immutables in core might be
unconvinced.
Thanks,
Tyson
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php
