|
|
Boost : |
Subject: Re: [boost] [log] Release candidate 1
From: Andrey Semashev (andrey.semashev_at_[hidden])
Date: 2013-03-10 16:40:49
On Sunday 10 March 2013 19:56:19 Vicente J. Botet Escriba wrote:
>
> The review result requested to take in account some critical issues. I'm
> sure that we could found some of the answers to these question on the
> documentation, but it would be easier for the mini-review if you give
> some explicit answers to these critical issues. Telling if you have
> taken in account some of the non-critical points could help also.
Sure.
> Critical issues
> ===============
>
> These issues must be addressed, and be passed through informal mini-
> review,
> before the library can be added to SVN.
>
> - It appears that the 'trivial' logging is not sufficiently good to just
> take and use. The library should provide an out-of-the-box mechanism
> that:
>
> - supports channel/severity attributes, without creating new logger for
> each severity.
The point about different severity levels requiring different loggers is not
accurate. This was never the case. But it was so for channels. In v2, it is
now possible to change channel for an existing logger without creating a new
one. A new macro BOOST_LOG_CHANNEL is introduced which allows to set channel
name for a specific log record.
However, I still don't recommend doing this in performance critical places.
> It also should be possible to filter out specific
> channel by essentially poking at some "map", and without manually
> composing a new filter that checks for a set of allowed channels
> sequentally. I believe that at least Christian, Roland, Robert and
> myself
> requested this.
This is implemented with the channel_severity filter.
> - has no observable effect on compile time. One second (or more) that
> trivial.hpp
> imposes is not acceptable.
I've done my best to reduce compile times, although I didn't purposefully
measure the gain. Compiling libs/log/example/trivial takes ~1.75 second on my
setup. Commenting out the expressions part of the example reduces the time to
~0.42 s. I'm open to suggestions for further improvements.
> - outputs to console by default, but permits changing that
Done. The default sink outputs to console. When other sinks are added they
override the default sink.
> - outputs only component/severity/message by default, but probably
> permits
> changing that.
The default sink format is the same as before:
timestamp [thread id] [severity] message
I believe, all these components are essential for common logging purposes. You
can always set up your own format when you register your sinks.
> - By default, the library should try hard to continue working no
> matter what errors happen during processign a log record -- including
> things like invalid attribute names or types, or other exceptions.
Done, filters and formatters don't throw by default.
> - The library should not reinvent wheels. In particular, custom
> implementations
> of TSS and RW mutex seems like a very bad idea. Use of a custom lambda
> implementation was pointed by many as not great -- we already have a
> couple,
> so let's not add more to the mix.
The lambda expressions (filters and formatters) are now based on
Boost.Phoenix. Custom implementations of TSS and RW mutex are preserved as I'm
not satisfied with the existing implementations in Boost.Thread. We discussed
this with Vladimir after the review report was published and agreed that this
was not a hard requirement.
> - Library interface is too geared towards lambda. In particular,
> implementing
> formatters/filters as free-standing functions appears to require
> considerably
> more code. In particular, it should be possible to access an
> attribute using a
> single function call. Also, logging::extract taking a lambda for
> callback
> complicates matters, it's desirable to have a more direct variant.
The previous extraction API has been reworked and divided into new extraction
and visitation APIs. In general, the former one allows to get a reference to
the stored value, and the latter one is for applying a visitor to it.
Also, with addition of attribute keywords it is now possible to extract
attribute values with operator[] applied to records and attribute value sets.
http://boost-log.sourceforge.net/libs/log/doc/html/log/detailed.html#log.detailed.core
As for other issues in the report:
Important issues
================
These issues are important enough to be mentioned here, but can be
addressed as convenient.
- The reported run-time performance can be improved. In particular the
performance with date attributes seems scary and 2x slowdown relative
to some other library in the 'most records are filtered out' case is
also concerning.
I have implemented a custom version of the date/time formatter that does not
use facets provided by Boost.DateTime. I did not measure the performance in
numbers but it should be much faster.
- I don't think it was satisfactory explained why 'attr' exists in two
namespaces, as opposed to being a single type that is used for both
formatting and filtering.
This cross-references the custom lambda expressions that were in v1. Now there
is a common boost::log::expressions namespace and a common 'attr' placeholder
in it.
Other notes
===========
These issues seem important and should be considered, but might not even
be fixable within the current design.
- Several reviewers said the library is "too flexible" and this makes it
harder to understand. Probably nothing can be done about that at this
point, but interesting data point notwithstanding.
Well, flexibility comes from multitude of requirements to the library. I don't
think reducing flexibility would serve the library for the better, but I admit
that some things are more complicated than I'd like.
- It was suggested that the library include an API that another Boost
log
may use in a way that permits plugging another logging library should
end user of a Boost so desire. This seems a good idea, but I think
it's
up to Andrey to implement such API or not.
Any logging library can use the regular logging interfaces Boost.Log provides.
There's no need for a special interface.
- Alternative design approaches were proposed, in particular (i)
permitting
user log records and templating/basing other components on a user-
defined
record type and (ii) using compile-time indexing to access attributes.
Also, it was proposed that named parameters not be necessary to use
the
library, and a more conventional interface be available.
It does not seem that implementing those suggestions is fully possible
without writing a different library from scratch, so it's up to Andrey
to consider those proposals.
This would take to write another library for other purposes.
- It was suggested to use newer Spirit to address poor compile times for
the library itself.
I've ported the code to Boost.Spirit v2 although I don't think this improved
library compile times.
Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk