[RFC] Union Types v2

Le jeudi 5 septembre 2019, 12:50:48 CEST Joe Watkins a écrit :

Because the PHP project should avoid depending on a privately owned centralized service for its technical discussions

This is obviously your opinion, but you haven't actually told us why this
is the case, and it's not at all obvious.

I thought it was obvious, sorry.
So, the problem is PHP have no control over github, and cannot know how it will evolve.

GAFAM organizations have closed people accounts before, sometimes without giving reasons.
I think PHP project should not let other organisations choose who can or cannot participate in PHP development.

should not encourage (some would say force) people to use such platforms.

In any case it's not a choice for the contributor, internals chooses the
medium and the contributor has to use it. Whether we force them to use a
mailing list from last century or something from this century makes no
difference with regard to choice for the contributor.

With a mailing list the contributor can choose its email server and email client. He does not have to agree to term of service of a third party.
https://tosdr.org/#github
With github, internal chooses to delegate to github, and github (currently microsoft) chooses who can take part in the discussion and how the discussion works. (they can change the platform tomorrow and internal won’t be able to do anything about it).

Côme

Because the PHP project should avoid depending on a privately owned
centralized service for its technical discussions

This is obviously your opinion, but you haven't actually told us why this
is the case, and it's not at all obvious.

should not encourage (some would say force) people to use such platforms.

In any case it's not a choice for the contributor, internals chooses the
medium and the contributor has to use it. Whether we force them to use a
mailing list from last century or something from this century makes no
difference with regard to choice for the contributor.

I am not worrying when one uses it for draft. However anything after that
should happen in the wiki and on our git as it is the correct process.

I really like github, or gitlab, however int he current context, almost all
contributors may lose access to github (or other US based) companies based
on the US government policies or directives. Without starting a political
discussion whether or not this is valid, it is definitely a big risk. A
risk I am not really willing to take for php itself, if I may say.

best,

but at least views
like externals.io and news.php.net can let you navigate the tree.

The lack of a full tree-like structure isn't the worst thing in the
world. If only because it discourages certain types of individual from
wanting to reply to every single sub-branch individually to get the
final word.

Something I am finding hard on Github, and maybe it's just because I
haven't found the option yet, is finding new posts.

Mark Randall

GitHub usage is inevitable.

Did you use the wrong word here, or are you saying that, of all the
hundreds of different platforms we could investigate, there is no chance
that we would end up using something other than github?

The interface is so good with clear discussion and review options

As my previous message, and those of several other people, show, that is
far from an established consensus. The power of an e-mail list is that
different users can use different interfaces - I've yet to see a forum
suggested that I would find easier than Thunderbird's tree view. There are
certainly downsides to e-mail, and upsides to GitHub, but let's stay calm
and evaluate our options rather than jumping at the first thing we see.

Regards,

Rowan Tommins
[IMSoP]

Happy to read your thoughts on this Nikita, I think you've drawn some good conclusions.

If I may add a thought or two:

I wouldn't make any final decisions based on one experiment, especially a big RFC as this one. I think the GitHub discussion got extra attention because it was the first one, and because of the scope of the RFC. I would try to have two or three more RFCs discussed on GitHub, maybe smaller ones?

Second, there are more things we can do in order to keep the main thread on topic. Three things come to mind:

 - We could add community guidelines, clearly stating that RFC comments should stay on topic
 - People could be appointed to moderate the comments, allowing contributors to focus on the code instead of community management
 - Conversations on GitHub can be locked as a last measurement. Repository members can still comment.

I fear that separating the main discussion from the PR will cause unnecessary confusion: important, generals remarks could be made on the "main thread", and I think there's value in keeping these remarks together with everything else.

Kind regards
Brent

Here are my own thoughts on how the pull request discussion for union types
went...

I think the main takeaway for me is that inline comments (on specific lines
in the RFC) were really invaluable. Each comment thread discussed a
specific issue and most of them have resulted in a direct improvement to
the RFC.

Generally there was a lot of discussion of specific technical details that
we very rarely see in RFC discussions. Current RFC discussions on the
mailing list tend to be rather high level (which is fine in itself), with
nobody ever discussing the details (which is very bad).

Thinking back to https://wiki.php.net/rfc/engine_warnings, I think that RFC
could have really benefited from this discussion medium. While the mailing
list discussion ended up talking circles around more or less one single
question (undefined variables), pretty much none of the other parts of the
RFC have seen so much as a comment. I'm sure that there would be a lot more
discussion regarding specific classifications if this went up as a pull
request.

Another nice thing is that it's possible to mark a comment thread as
resolved, once the RFC has been adjusted to address the comments. That way
you don't have to see issues that were already addressed (though you can if
you like).

Having thumbs-up and thumbs-down reactions to comments was also helpful to
judge whether some comment represents a minority opinion or not, something
that is notoriously hard with current mailing list discussions (which are
almost dominated by "negative" opinions which mysteriously don't show up in
voting results).

However, while the inline comments were pleasantly insightful, the same
cannot be said for the top-level comments on the pull request. The majority
of them was borderline off-topic. While some in principle interesting
discussion happened there, it simply didn't belong in the RFC thread for
union types. The top-level comments also suffered from a lack of threading
-- I would have been less bothered about tangential discussions if they
were threaded. (To be fair: I use gmail, so I don't get threading on the
mailing list either.)

If this kind of discussion behavior is representative, then I would suggest
a workflow alone the following lines...

• RFCs are submitted as PRs on GitHub, but must be announced on the mailing
  list.
• The PR description should have a fat warning that top-level comments
  belong on the mailing list. We can mark all top-level comments on PRs as
  "off-topic" as a matter of general policy.
• Top-level commentary stays on the mailing list.

This is a shift from what I originally had in mind (completely moving the
RFC process to GitHub), towards providing a way for more detailed and
specific feedback on the RFC text.

Regarding GitHub as a 3rd party. I think there are a few things to
considered:

• We're already very heavily reliant on GitHub. Most of my day-to-day
  interaction with PHP core development is via GitHub and most of the
  day-to-day decisions also happen there. Only the major stuff everhits this
  mailing list.
• The RFC repo would of course be hosted on git.php.net as usual and only
  be mirrored to GitHub.
• GitHub would not be the exclusive venue for RFC discussion. All RFCs are
  still announced on internals and the top-level discussion can and should
  still happen here.

Disclaimer: I'm trying to draw conclusions here from an experiment with a
sample size of 1, which may not be representative. Union types are a pretty
significant proposal (and also the first one to be on GH), and other,
smaller proposals might well have different discussion dynamics.

Regards,
Nikita

Le jeudi 5 septembre 2019, 12:04:55 CEST Brent a écrit :

Huge "no" from me on using github for discussing RFCs.

Care to elaborate why? The majority seems to like it. Though I am also
curious about Nikita's experience with it, as he is the one having to
process the feedback.

Because the PHP project should avoid depending on a privately owned
centralized service for its technical discussions, and should not encourage
(some would say force) people to use such platforms.

PHP is already on github but it’s only a mirror, the main git repository
is at git.php.net .

Côme

Le vendredi 6 septembre 2019, 16:47:52 CEST Nikita Popov a écrit :

• GitHub would not be the exclusive venue for RFC discussion. All RFCs are
  still announced on internals and the top-level discussion can and should
  still happen here.

My remark on the mailing list regarding string|true was unanswered and I had to go over to github to see that the concern was discussed there.
This is the kind of problems splitting the discussion will cause, people will have to check both places or miss things.

Regarding the github PR as a practical point of view, I find it hard to check if there are new messages since last time I visited the page. Is there any way to browse messages by time? (backwards ideally).

Côme

On Tue, Sep 24, 2019 at 12:24 PM Claude Pache claude.pache@gmail.com
wrote:

The choice of supporting precisely the two literal values null and
false
is not arbitrary: They are the two values that are the most often used as
sentinel values (for indicating failure or absence). It is true that
true is
also sometimes used as sentinel value (more rarely and not among the
internal functions), but the same can be said of other literal values
(one of your examples includes 0).

While I personally think false makes sense as an allowed "type", I also
don't want to see the union types RFC get held up on such a tiny detail.

I would propose either of the following alternatives:
1/ Remove false from the proposal. It can always be added at a later
time, but not taken away.
2/ Make this detail a sub-vote. I would suggest that this sub-vote should
also be subject to a 2/3 majority in order to pass.

-Sara

I would tend to agree with Sara. That seems to be the only issue of contention and it is (AFAIK) reasonably straightforward to add "later" without blocking the rest of Union types. It would mean some functions wouldn't be able to get a fully accurate return type yet but... they've survived this long without them, they can wait a bit longer while this gets settled and/or some even more robust alternative is found.

--Larry Garfield

On Tue, Sep 24, 2019 at 12:24 PM Claude Pache claude.pache@gmail.com
wrote:

The choice of supporting precisely the two literal values null and
false
is not arbitrary: They are the two values that are the most often used as
sentinel values (for indicating failure or absence). It is true that
true is
also sometimes used as sentinel value (more rarely and not among the
internal functions), but the same can be said of other literal values
(one of your examples includes 0).

While I personally think false makes sense as an allowed "type", I also
don't want to see the union types RFC get held up on such a tiny detail.

I would propose either of the following alternatives:
1/ Remove false from the proposal. It can always be added at a later
time, but not taken away.
2/ Make this detail a sub-vote. I would suggest that this sub-vote should
also be subject to a 2/3 majority in order to pass.

-Sara

This RFC is currently held up by a lack of implementation. Once that is
done, the RFC will go forward as-is (barring any novel concerns). Because I
consider it an important part of the overall proposal (*), I will neither
remove the false type, nor split it into a separate vote. People may vote
against the whole RFC if they disagree with this aspect, or any other
aspect of the proposal.

Regards,
Nikita

(*) While certainly not the primary reason for why we should support union
types, the reason why I brought this proposal forward at this time
specifically, is that the lack of union types is a blocker for my pet
project of providing comprehensive type annotations for internal functions.
Supporting "false" is strictly necessary for this purpose, because it is
part of nearly all unions as far as internal functions are concerned.

The simpler idea I had in this area was caching what type checks a value
had passed, either on each zval or perhaps just at the class level, so that
checking the same type again would be much faster, even if it was a complex
union with multiple interfaces.

Last night I was playing about with caching the last passed interface as
part of the instanceof_function.

https://gist.github.com/marandall/38d7dba6600889d897f2c8fc57532f31

When used in the likes of loops which are performing the same check over
and over, it yields about a 18% increase when using a 3 layer deep
nested interface.

It currently just stores a cache of the last match in the class entry so
it would hit as many uses as possible.

For T_INSTANCEOF it could potentially use an extended cache slot to
store a reference to it (would need expanding out into a separate struct
as the cache slot is already used for converting the static name to a ce).

I'm less clear about how we could handle it for everything else, like
parameter type validation. The underlying implementation already seems
pretty well optimized so I'm not sure where a performance penalty is to
be found.

--
Mark Randall

I think this performance impact is a real concern; PHP is the only language
I know of which implements type checks entirely at run-time in production
code, and we should ask ourselves if that's definitely the right approach.

As they are runtime checks, would an ini setting where they can be
completely disabled be feasible? So during development and in production
when the performance decrease doesn't matter, I can have the full runtime
type checking. But when absolutely needed, the checking can be disabled.

Peter

I think this performance impact is a real concern; PHP is the only language
I know of which implements type checks entirely at run-time in production
code, and we should ask ourselves if that's definitely the right approach.

Would it be possible, at least in principle, to build a static analysis

tool which could prove that certain type checks would never fail, and prime
OpCache with code that leaves them out? As I understand it, this is how
Dart works: the compiler only emits run-time checks for assertions it can't
prove true by static analysis.

I was wondering the same thing, especially in the light of preloading,
where a lot of class/function definitions are known on startup.

For example, doing:

function x(): int {}
function y(int $foo) {}
y(x());

Should definitely not redundantly check the type of the y() parameter, if
possible.

Also, in the case of JIT, shouldn't type hints actually improve
performance?

— Benjamin

Hi Dmitry,

Actually, I think PHP already took wrong direction implementing "typed
references" and "type variance".
Introducing more "typing", we suggest using it, but this "typing" comes
with a huge cost of run-time checks.
From 10% (scalar type hint of argument) to 3 times (typed reference
assignment) performance degradation.
Anyone may rerun my benchmarks
https://gist.github.com/dstogov/fb32023e8dd55e58312ae0e5029556a9

I think this performance impact is a real concern; PHP is the only language
I know of which implements type checks entirely at run-time in production
code, and we should ask ourselves if that's definitely the right approach.

Would it be possible, at least in principle, to build a static analysis
tool which could prove that certain type checks would never fail, and prime
OpCache with code that leaves them out? As I understand it, this is how
Dart works: the compiler only emits run-time checks for assertions it can't
prove true by static analysis.

The simpler idea I had in this area was caching what type checks a value
had passed, either on each zval or perhaps just at the class level, so that
checking the same type again would be much faster, even if it was a complex
union with multiple interfaces.

Regards,

Rowan Tommins
[IMSoP]

Hi Nikita,

in your results, assignment to typed reference is ~3 time slower without JIT and ~10 times slower with JIT.
But this is not the worst case. I can simple craft a script where single assignment will lead to hundreds type checks.

<?php
class A {
public ?A $ref;
function __construct(&$ref = null) {
$this->ref =& $ref;
}
}
for ($i = 0; $i < 100; $i++) {
$a[$i] = new A($ref);
}
$ref = new A; // <= 100 type checks on a single assignment
?>

In case we add union types, we can make 200, 300 check...

The worst thing, for me, is variance together with union of multiple class types.
Each such method compatibility check is going to be a puzzle solving, and I imagine people, proud of writing "complex pearls".

My main concern, I don't like to complicate the language with features that shouldn't be often used, and I don't like to complicate implementation and reduce performance without real need.

In my opinion, union of standard types and single class or null should be enough for most typing use cases.

Thanks. Dmitry.

From: Nikita Popov nikita.ppv@gmail.com
Sent: Friday, October 25, 2019 13:22
To: Dmitry Stogov dmitry@zend.com
Cc: PHP internals internals@lists.php.net
Subject: Re: [PHP-DEV] Re: [RFC] Union Types v2

Hi Nikita,

I checked the Union Type implementation, and it more or less good. I mean, it implements the RFC in almost the best way.
However, as I don't like the RFC itself. Especially, unions of multiple classes and interference with type variance, I'll vote against this.

Actually, I think PHP already took wrong direction implementing "typed references" and "type variance".
Introducing more "typing", we suggest using it, but this "typing" comes with a huge cost of run-time checks.
From 10% (scalar type hint of argument) to 3 times (typed reference assignment) performance degradation.
Anyone may rerun my benchmarks https://gist.github.com/dstogov/fb32023e8dd55e58312ae0e5029556a9

Thanks. Dmitry.

For reference, here are the results I get with/without JIT: https://gist.github.com/nikic/2a2d363fffaa3aeb251da976f0edbc33

I think that union types don't really change much in terms of the performance calculus of type checking, because type checking is equally fast (or slow) for a single scalar type, and 5 different scalar types: they are all handled in a single bit check. The only case that is indeed somewhat slow is if multiple class types are used in the union, because we actually have to check each one until we find a match. I do hope that having unions with a large number of classes will not be common.

Generally, this area could use some more optimization from the implementation side. I spent a bit of time yesterday optimizing how we perform instanceof checks (the implementation was quite inefficient, especially for interfaces), which had a large positive impact on class type checks. There's more low hanging fruit like this, for example verify_return_type has no JIT implementation yet (which is why with JIT simple arg type checks have nearly zero overhead, while return type checks have a large overhead). Assignments to typed properties are also badly optimized, because everything is punted to the slow path, while we should be able to handle the simple cases with just one extra bit check, without going through the complex implementation that deals with things like weak typing coercions.

I do think that performance considerations are important when it comes to new typing features (which is why, for example, we have categorically rejected a "typed arrays" implementation that has to check all array elements), but don't see union types are particular problematic in that regard, beyond what we already have.

Nikita

I do think that performance considerations are important when it comes to
new typing features (which is why, for example, we have categorically
rejected a "typed arrays" implementation that has to check all array
elements), but don't see union types are particular problematic in that
regard, beyond what we already have.

While union types barely seem to add to the cost of type checking, I
also understand the general sentiment: Type checks can be expensive,
should we really add more?

Perhaps it might be an idea to wait a little before starting the vote to
show the cost going down as more optimization work is done. Just to
build confidence with those who are hesitant to give it their approval.

Regards,
Dik Takken

For reference, here are the results I get with/without JIT:
https://gist.github.com/nikic/2a2d363fffaa3aeb251da976f0edbc33

I toyed a bit with the benchmark script (union_bench.php) as well and
wanted to share some observations. First of all I noticed the benchmark
script has a typo on line 90 where it is calling the wrong function. It
should read:

func6(1, 2, 3, 4, 5);

When running the corrected script I see that adding 5 argument type
checks and a return type check cause almost 4x slowdown. My results
(with opcache / jit):

func($a,$b,$c,$d,$e) 0.680 0.583
func(int $a,$b,$c,$d,$e): int 2.106 2.009

However, this appears to be entirely due to the return type check
lacking a JIT implementation, as pointed out by Nikita. Adding one more
test to the benchmark shows this nicely:

func($a,$b,$c,$d,$e) 0.675 0.575
func(int $a,$b,$c,$d,$e) 0.574 0.475
func(int $a,$b,$c,$d,$e): int 2.106 2.009

Now we can see that the argument type hint actually improves
performance, I guess due to it narrowing down the number of possible
types that need to be considered for the function arguments.

Union types allow for more accurate type hinting as well as type hinting
in places where this is currently not possible. As a result union types
can be used to obtain performance gains. As an example, consider the
case where the return type hint matches the type information that
opcache has inferred about the variable that is returned. In that case,
the return type check is optimized away. Let us try and leverage union
types to make this happen. From the benchmark script we take func6:

function func6(int $a, int $b, int $c, int $d, int $e) : int {
return $a + $b + $c + $d + $e;
}

and adjust it to read:

function func6(int $a, int $b, int $c, int $d, int $e) : int|float {
return $a + $b + $c + $d + $e;
}

Now the return type hint matches what opcache infers the result of the
expression will be and the cost of return type checking disappears
completely:

func($a,$b,$c,$d,$e) 0.663 0.568
func(int $a,$b,$c,$d,$e) 0.574 0.475
func(int $a,$b,$c,$d,$e): int|float 0.561 0.466

Then, on to another observation. The SSA forms currently produced by
opcache show union types like string|int. This suggests that opcache
supports union types for type inference already. It explains why opcache
can nicely optimize type checks away even when union types are used.

This is not true for unions of classes though. A union type like int|Foo
copies into the SSA form just fine while Foo|Bar becomes 'object'. Code
like this:

class Foo {}
class Bar {}

function func(): Foo|Bar {
return new Foo();
}

func();

produces the following SSA form:

func: ; (lines=4, args=0, vars=0, tmps=1, ssa_vars=2, no_loops)
; (before dfa pass)
; /php-src/sapi/cli/test.php:6-8
; return [object]
BB0: start exit lines=[0-3]
; level=0
#0.V0 [object (Foo)] = NEW 0 string("Foo")
DO_FCALL
VERIFY_RETURN_TYPE #0.V0 [object (Foo)] -> #1.V0 [object]
RETURN #1.V0 [object]

which will still perform a return type check even though the return type
hint matches the actual type of the variable. Apparently the union type
support in opcache is present but incomplete.

So, while union types can incur higher type checking cost they also
provide more powerful means to help type inference and improve
performance. As opcache improves over time I think we can expect the
cost to decrease while the gain increases. Or am I too optimistic here?

Regards,
Dik Takken