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(a)fastmail.com>om>:
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(a)lists.wikimedia.org
To unsubscribe send an email to wikitech-l-leave(a)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.