[prev in list] [next in list] [prev in thread] [next in thread]
List: wikitech-l
Subject: [Wikitech-l] Re: Feedback wanted: PHPCS in a static types world
From: Lucas Werkmeister <lucas.werkmeister () wikimedia ! de>
Date: 2022-11-16 16:17:19
Message-ID: CAFbwPjfrR-hAs9W17_1unmaVh4btZgW-hCKvR8yU4kt+govQ5A () mail ! gmail ! com
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
What Timo writes sounds good to me. I don't think we need to enforce
removal of existing code blocks – that can happen gradually as the code is
touched for other reasons instead.
Regarding performance, I think we should limit micro-optimizations like
omitting static types to code that we know is hot; for most of our code, I
think the benefit of avoiding bugs that can be caught or prevented by
static types is more important. We already have some hot code that uses a
more performance-oriented code style (e.g. RemexHtml "sometimes use[s]
direct member access instead of going through accessors, and manually
inline[s] some performance-sensitive code"), but we don't use that code
style everywhere.
Aside: I would also hope that PHP will eventually learn some optimizations
that are taken for granted in many other languages, such as inlining
setters/getters and other short methods, or (more relevant to this thread)
eliminating runtime type checks when they can be statically proven. But I
definitely don't have the skills to push that forward, so I won't complain
about it too much.
I'll try to find some time soon-ish to look into MediaWiki CodeSniffer and
see if some improvements can be implemented without too much trouble.
Am Mi., 16. Nov. 2022 um 01:00 Uhr schrieb Krinkle <krinkle@fastmail.com>:
>
>
> On Tue, 15 Nov 2022, at 11:41, Daniel Kinzler wrote:
>
> Am 10.11.2022 um 03:08 schrieb Tim Starling:
>
> Clutter, because it's redundant to add a return type declaration when the
> return type is already in the doc comment. If we stop requiring doc
> comments as you propose, then fine, add a return type declaration to
> methods with no doc comment. But if there is a doc comment, an additional
> return type declaration just pads out the file for no reason.
>
> I agree that we shouldn't have redundant doc tags and return type
> declarations. I would suggest that all methods should have a return type
> declaration, but should not have a @return doc tag unless there is
> additional info […]
>
>
> The performance impact is measurable for hot functions. In gerrit 820244
> <https://gerrit.wikimedia.org/r/c/mediawiki/core/+/820244> I removed
> parameter type declarations from a private method for a benchmark
> improvement of 2%.
>
> This raises an interesting issue, one that has bitten me before: How do we
> know that a given method is "hot"? Maybe we should establish a @hot or
> @performance tag to indicate that a given method should be optimized for
> speed. […]
>
>
> I think the enforced and automated codesniffer could remain fairly simple:
> As today, the sniff encourages all methods to have parameter and return
> types documented in a way that humans, Phan, and IDEs can understand for
> static analysis to avoid and catch mistakes.
>
> What I propose we change is that instead of enforcing this solely through
> a mandatory doc comment, enforce it by requiring at least one of them to be
> present. Either parameters and returns are typed, or a doc block exists.
> Both may exist, of course.
>
> We've established in this email thread that it can be cluttering (and
> waste of effort) to require repeating of information when doing so adds no
> value. It is also my understanding that Phan and IDEs already understand
> either and both so we don't need them to be aware of which "should" exist.
>
> Is there value in enforcing removal of existing doc blocks after someone
> has written it? This seems to me like potentially a significant time sink
> with no return on that other because we enforced it as a new rule. If we
> agree there is no urgency in removing existing doc blocks or actively
> blocking CI when someone choose to write a doc block, then afaik we do not
> need new annotations like "hot" or "performance" or some other tag to
> surpress warnings about doc blocks.
>
> I do think it is important to preserve author intent when it comes to
> performance optimisations. However these are by no means limited to this
> new notion of saving native type overhead. There are all sorts of code
> optimisations. I believe we typically document these through an inline
> comment like "Optimization: ..." next to the code in question, in which the
> need for optimisation and sometimes (if non-obvious) how that optimisation
> is achieved, are mentioend. That should suffice I think in preserving the
> use case and e.g. prevent someone from re-introducing typing where it was
> previously removed for perf reasons.
>
> In other words: Codesniffer helps us avoid unknown types (in docblock
> and/or native type), and inline comments remind us about past performance
> optimisations. Do we need more? If so, what is the benefit/usecase for
> more? What do we risk if we don't?
>
> -- Timo
>
> _______________________________________________
> Wikitech-l mailing list -- wikitech-l@lists.wikimedia.org
> To unsubscribe send an email to wikitech-l-leave@lists.wikimedia.org
> https://lists.wikimedia.org/postorius/lists/wikitech-l.lists.wikimedia.org/
>
--
Lucas Werkmeister (he/er)
Software Engineer
Wikimedia Deutschland e. V. | Tempelhofer Ufer 23-24 | 10963 Berlin
Phone: +49 (0)30-577 11 62-0
https://wikimedia.de
Imagine a world in which every single human being can freely share in the
sum of all knowledge. Help us to achieve our vision!
https://spenden.wikimedia.de
Wikimedia Deutschland - Gesellschaft zur Förderung Freien Wissens e. V.
Eingetragen im Vereinsregister des Amtsgerichts Berlin-Charlottenburg unter
der Nummer 23855 B. Als gemeinnützig anerkannt durch das Finanzamt für
Körperschaften I Berlin, Steuernummer 27/029/42207.
[Attachment #5 (text/html)]
<div dir="ltr"><div>What Timo writes sounds good to me. I don't think we need to \
enforce removal of existing code blocks – that can happen gradually as the code is \
touched for other reasons instead.</div><div><br></div><div>Regarding performance, I \
think we should limit micro-optimizations like omitting static types to code that we \
know is hot; for most of our code, I think the benefit of avoiding bugs that can be \
caught or prevented by static types is more important. We already have some hot code \
that uses a more performance-oriented code style (e.g. RemexHtml "sometimes use[s] \
direct member access instead of going through accessors, and manually inline[s] some \
performance-sensitive code"), but we don't use that code style \
everywhere.</div><div><br></div><div style="margin-left:40px">Aside: I would also \
hope that PHP will eventually learn some optimizations that are taken for granted in \
many other languages, such as inlining setters/getters and other short methods, or \
(more relevant to this thread) eliminating runtime type checks when they can be \
statically proven. But I definitely don't have the skills to push that forward, so I \
won't complain about it too much.</div><div><br></div><div>I'll try to find some time \
soon-ish to look into MediaWiki CodeSniffer and see if some improvements can be \
implemented without too much trouble.<br></div><div><br></div></div><br><div \
class="gmail_quote"><div dir="ltr" class="gmail_attr">Am Mi., 16. Nov. 2022 um 01:00 \
Uhr schrieb Krinkle <<a \
href="mailto:krinkle@fastmail.com">krinkle@fastmail.com</a>>:<br></div><blockquote \
class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid \
rgb(204,204,204);padding-left:1ex"><div \
class="msg5004372682503488465"><u></u><div><div><br></div><div><br></div><div>On Tue, \
15 Nov 2022, at 11:41, Daniel Kinzler wrote:<br></div><blockquote type="cite" \
id="m_5004372682503488465qt"><div>Am 10.11.2022 um 03:08 schrieb Tim
Starling:<br></div><blockquote type="cite">Clutter, because it's redundant \
to add a return type declaration when the return type is already in the doc comment. \
If we stop requiring doc comments as you propose, then fine, add a return
type declaration to methods with no doc comment. But if there is a
doc comment, an additional return type declaration just pads out
the file for no reason.<br></blockquote><p>I agree that we shouldn't have \
redundant doc tags and return type declarations. I would suggest that all methods \
should have a return type declaration, but should not have a @return doc tag
unless there is additional info […]<br></p><p><br></p><blockquote \
type="cite"><p>The performance impact is measurable for hot functions. In <a \
href="https://gerrit.wikimedia.org/r/c/mediawiki/core/+/820244" \
target="_blank">gerrit 820244</a> I removed parameter type declarations from a \
private method for a benchmark
improvement of 2%. <br></p></blockquote><p>This raises an interesting issue, \
one that has bitten me before:
How do we know that a given method is "hot"? Maybe we should
establish a @hot or @performance tag to indicate that a given
method should be optimized for speed. \
[…]<br></p></blockquote><div><br></div><div>I think the enforced and automated \
codesniffer could remain fairly simple: As today, the sniff encourages all methods to \
have parameter and return types documented in a way that humans, Phan, and IDEs can \
understand for static analysis to avoid and catch \
mistakes.<br></div><div><br></div><div>What I propose we change is that instead of \
enforcing this solely through a mandatory doc comment, enforce it by requiring at \
least one of them to be present. Either parameters and returns are typed, or a doc \
block exists. Both may exist, of course.<br></div><div><br></div><div>We've \
established in this email thread that it can be cluttering (and waste of effort) to \
require repeating of information when doing so adds no value. It is also my \
understanding that Phan and IDEs already understand either and both so we don't \
need them to be aware of which "should" \
exist.<br></div><div><br></div><div>Is there value in enforcing removal of existing \
doc blocks after someone has written it? This seems to me like potentially a \
significant time sink with no return on that other because we enforced it as a new \
rule. If we agree there is no urgency in removing existing doc blocks or actively \
blocking CI when someone choose to write a doc block, then afaik we do not need new \
annotations like "hot" or "performance" or some other tag to \
surpress warnings about doc blocks.<br></div><div><br></div><div>I do think it is \
important to preserve author intent when it comes to performance optimisations. \
However these are by no means limited to this new notion of saving native type \
overhead. There are all sorts of code optimisations. I believe we typically document \
these through an inline comment like "Optimization: ..." next to the code \
in question, in which the need for optimisation and sometimes (if non-obvious) how \
that optimisation is achieved, are mentioend. That should suffice I think in \
preserving the use case and e.g. prevent someone from re-introducing typing where it \
was previously removed for perf reasons.<br></div><div><br></div><div>In other words: \
Codesniffer helps us avoid unknown types (in docblock and/or native type), and inline \
comments remind us about past performance optimisations. Do we need more? If so, what \
is the benefit/usecase for more? What do we risk if we \
don't?<br></div><div><br></div><div>-- \
Timo<br></div><div><br></div></div>_______________________________________________<br>
Wikitech-l mailing list -- <a href="mailto:wikitech-l@lists.wikimedia.org" \
target="_blank">wikitech-l@lists.wikimedia.org</a><br> To unsubscribe send an email \
to <a href="mailto:wikitech-l-leave@lists.wikimedia.org" \
target="_blank">wikitech-l-leave@lists.wikimedia.org</a><br> <a \
href="https://lists.wikimedia.org/postorius/lists/wikitech-l.lists.wikimedia.org/" \
rel="noreferrer" target="_blank">https://lists.wikimedia.org/postorius/lists/wikitech-l.lists.wikimedia.org/</a></div></blockquote></div><br \
clear="all"><br>-- <br><div dir="ltr" class="gmail_signature"><div \
dir="ltr"><div><div dir="ltr"><div><div dir="ltr"><div><div dir="ltr"><div><div \
dir="ltr">Lucas Werkmeister (he/er)<br>Software Engineer<br><br>Wikimedia Deutschland \
e. V. | Tempelhofer Ufer 23-24 | 10963 Berlin<br>Phone: +49 (0)30-577 11 62-0<br><a \
href="https://wikimedia.de" target="_blank">https://wikimedia.de</a><br><br>Imagine a \
world in which every single human being can freely share in the sum of all knowledge. \
Help us to achieve our vision!<br><a href="https://spenden.wikimedia.de" \
target="_blank">https://spenden.wikimedia.de</a><br><br>Wikimedia Deutschland - \
Gesellschaft zur Förderung Freien Wissens e. V. Eingetragen im Vereinsregister des \
Amtsgerichts Berlin-Charlottenburg unter der Nummer 23855 B. Als gemeinnützig \
anerkannt durch das Finanzamt für Körperschaften I Berlin, Steuernummer \
27/029/42207.</div></div></div></div></div></div></div></div></div></div>
_______________________________________________
Wikitech-l mailing list -- wikitech-l@lists.wikimedia.org
To unsubscribe send an email to wikitech-l-leave@lists.wikimedia.org
https://lists.wikimedia.org/postorius/lists/wikitech-l.lists.wikimedia.org/
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic