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 <krin...@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-le...@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.
_______________________________________________ Wikitech-l mailing list -- wikitech-l@lists.wikimedia.org To unsubscribe send an email to wikitech-l-le...@lists.wikimedia.org https://lists.wikimedia.org/postorius/lists/wikitech-l.lists.wikimedia.org/
