Fwd: [ABANDONED] Remove some excessive log formatting
l.lunak at collabora.com
Fri Nov 22 11:13:21 UTC 2019
On Thursday 21 of November 2019, Stephan Bergmann wrote:
> My concerns are:
> * We may eventually want to enable SAL_WARN/SAL_INFO for production
> builds, at which point the space cost of log message strings might
> become an issue.
Space cost? The size of /usr/lib64/libreoffice/program/*.so here is about
200MiB. Even if we had 2MiB of log messages code, which would be probably an
insane amount, that'd be still a negligible 1%.
> * Too much trivial SAL_INFO noise (that was once added ad-hoc and should
> arguably have only been in a developer's local build) can make it
> unnecessarily hard to see the more relevant SAL_INFOs (and log area
> granularity is limited in practice). Of course, it is hard to tell how
> much of the existing SAL_INFO use falls under that noise category.
If I'm not mistaken, all SAL_INFO is disabled by default, which trivially
solves that. In case you meant SAL_WARN, that indeed can be a problem, but
rather a problem of either there being too many warnings that nobody cares
about, or those warnings not actually being important enough to be warnings.
> When I had originally designed SAL_WARN/SAL_INFO, I had anticipated use
> along the lines of:
> * Use SAL_WARN if it is an obviously erroneous unusual event. Like
> > SAL_WARN_IF(
> > *ppLibraryUrl == nullptr, "sal.osl", "rtl_string2UString
> > failed");
> in osl_getModuleURLFromAddress (sal/osl/unx/module.cxx), where it warns
> if the passed-in ppLibraryUrl cannot be converted to an 8-bit string.
I think that if something is an obvious error, then the code should just
plain assert. IMO the codebase is way too defensive and it's one of the
reasons why there are so many warnings that nobody cares about - because they
do not have to.
> I do not claim that this is the only reasonable approach to using
> SAL_WARN/SAL_INFO, and I have seen more liberal use effectively from day
> one. But I'm always a little concerned, for the reasons outlined above,
> when I come across what looks like overly liberal use of SAL_INFO.
One way to address your concerns might be by adding SAL_TRACE (or whatever
it'd be named), which would be even one level lower than SAL_INFO and it
wouldn't even be compiled in release builds if we get to the point of keeping
SAL_INFO for those. But when we got to looking at the actual issue, how this
would actually matter. The purpose of keeping SAL_INFO in release builds
would be presumably that users could provide such output when asked for e.g.
in bugzilla, but then how to decide where to draw the line? If I want the
debug output, I probably want all of it.
And of course all of this matters only if it actually matters in practice.
I'm still fairly convinced that both the space and time cost can be
negligible, you have designed it to be pretty cheap if not used (with the
possible exception of sal_detail_log_report(), but that could be improved if
l.lunak at collabora.com
More information about the LibreOffice