Last modified: 2012-11-07 04:28:14 UTC
Looking through a lot of the SMW code you're customly building function names What's wrong with __METHOD__/__FUNCTION__ Simiarily, wfProfileIn( "SMWSQLStore2::getInProperties (SMW)" ); Do you need to add the "(SMW)"?? And why are you hard coding those anyway?
And if the rest of the SM* extensions do the same, please clean those up aswell
I had a quick scan over the select() calls, and only found one not passing anything Added in r90200 Can you evaluate any other DB methods that you're not passing them, and please add them too
The reason why "(SMW)" was added was that this simplifies getting an overview of all SMW-related code blocks among the large amount of profiling data. There does not seem to be another way of filtering profiling by extension, and this is important for a big extension like SMW. In general, I am inclined to close this as invalid or at least wontfix. Clearly, we are discussing a detail of coding style here that should not have any impact on observable program behavior. Moreover, it is not so clear that the proposed change is an improvement at all. I the can see that the use of __METHOD__ etc. could be considered preferable regarding code style, but OTOH the use of string construction functions in frequent profiling calls (to add "(SMW)") could be considered harmful for performance (since parameter construction costs apply even if profiling is disabled). I appreciate efforts to implement clean coding conventions, but looking at the code of SMW and MW, I wonder if this issue is anywhere near the top of things to bother about in this respect. (This does not affect the unrelated comment on select; these should obviously have provenance information attached in all cases.)
(In reply to comment #3) > The reason why "(SMW)" was added was that this simplifies getting an overview > of all SMW-related code blocks among the large amount of profiling data. There > does not seem to be another way of filtering profiling by extension, and this > is important for a big extension like SMW. If you want filterability, prefix with SMW- instead of suffixing with (SMW). It adds a nice expando-box on profileinfo.php. This will mess up sorting, though (they're all grouped together), so it may or may not actually be a good idea. > In general, I am inclined to close this as invalid or at least wontfix. > Clearly, we are discussing a detail of coding style here that should not have > any impact on observable program behavior. I tend to agree. > Moreover, it is not so clear that > the proposed change is an improvement at all. I the can see that the use of > __METHOD__ etc. could be considered preferable regarding code style, but OTOH > the use of string construction functions in frequent profiling calls (to add > "(SMW)") could be considered harmful for performance (since parameter > construction costs apply even if profiling is disabled). That performance cost is minuscule and can be safely ignored. There's no need to prematurely optimise that.
It is over a year that this request was discussed, can we close this one?
