[log] Boost.Log formal review result
First of all, I would like to thank Andrey for this submission and the active discussion during the review. I also would like to thank everybody who participated in the review. The comments on the library were generally positive. Several reviewers have tried it in practical projects already, and found it superior to other libraries. It clearly will benefit the users of Boost right away. Therefore, the library is accepted. Congratulations, Andrey! On the other hand, some concerns about the ease of use and performance and implementation details were raised. Of those, some can be easily addressed as part of normal maintenance, while some seem to require significant rework and may change the library quite consirably. Therefore, the library is accepted subject to several conditions -- listed below under "Critical issues". Because the changes can be large, I think a mini-require is required when those issues are addressed. To avoid delays, it's probably not necessary to formally schedule and conduct a mini-review -- just posting a email asking for discussion will be sufficient. Statistics ========== There were roughly 300 emails during formal review (I have a mbox archive if anybody needs it). The following persons submitted a review with a vote. Some of votes are "provisional", this is not reflected below, but affects the list of critical issues below. - Boris Schaeling (yes) - Roland Bock (yes) - Ralf Globisch (yes) - Sean Chittenden (yes) - Darryl Green (no) - Tom Brinkman (no) - Barend (yes) - Sam Gundry (yes) - Vicente Botet (yes) - Robert Stewart (yes) - Alexander Arhipenko (yes) - "M-Square" (yes) 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. 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. - has no observable effect on compile time. One second (or more) that trivial.hpp imposes is not acceptable. - outputs to console by default, but permits changing that - outputs only component/severity/message by default, but probably permits changing that. - 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. - 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. - 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. 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 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. 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. - 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. - 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. - It was suggested to use newer Spirit to address poor compile times for the library itself. Thanks, Volodya
On 03/25/2010 01:28 AM, Vladimir Prus wrote:
First of all, I would like to thank Andrey for this submission and the active discussion during the review. I also would like to thank everybody who participated in the review.
The comments on the library were generally positive. Several reviewers have tried it in practical projects already, and found it superior to other libraries. It clearly will benefit the users of Boost right away. Therefore, the library is accepted. Congratulations, Andrey!
Thank you, Vladimir. You did a really great job both as a review manager and as a reviewer. I would also like to thank all the reviewers, I needed this kind of feedback. My special appreciation goes to Steven Watanabe, who gave thorough reading to the documentation and provided the needed corrections, and also gave many fruitful suggestions about the code. I agree to most prerequisites needed for the final inclusion, with a sole exception of this one:
Critical issues ===============
These issues must be addressed, and be passed through informal mini-review, before the library can be added to SVN.
- The library should not reinvent wheels. In particular, custom implementations of TSS and RW mutex seems like a very bad idea.
I have noted that during the review that these tools were implemented for a reason. They make the library faster and lighter in terms of memory footprint. They allow to avoid including Boost.Thread headers in public headers of Boost.Log, which also speeds up the compilation. I think these matters are more important than having the code dispersion they add. I remember that it was you who expressed the concern about these tools. Do you think this issue should really be a hard requirement for the library to be included in Boost?
Andrey Semashev wrote:
On 03/25/2010 01:28 AM, Vladimir Prus wrote:
I agree to most prerequisites needed for the final inclusion, with a sole exception of this one:
Critical issues ===============
These issues must be addressed, and be passed through informal mini-review, before the library can be added to SVN.
- The library should not reinvent wheels. In particular, custom implementations of TSS and RW mutex seems like a very bad idea.
I have noted that during the review that these tools were implemented for a reason. They make the library faster and lighter in terms of memory footprint. They allow to avoid including Boost.Thread headers in public headers of Boost.Log, which also speeds up the compilation. I think these matters are more important than having the code dispersion they add.
I'd like to know the real effect of using off-the-Boost-shelf components rather than your special sauce. That is, I'd like to know the performance and compilation tradeoffs for them before commenting further. _____ Rob Stewart robert.stewart@sig.com Software Engineer, Core Software using std::disclaimer; Susquehanna International Group, LLP http://www.sig.com IMPORTANT: The information contained in this email and/or its attachments is confidential. If you are not the intended recipient, please notify the sender immediately by reply and immediately delete this message and all its attachments. Any review, use, reproduction, disclosure or dissemination of this message or any attachment by an unintended recipient is strictly prohibited. Neither this message nor any attachment is intended as or should be construed as an offer, solicitation or recommendation to buy or sell any security or other financial instrument. Neither the sender, his or her employer nor any of their respective affiliates makes any warranties as to the completeness or accuracy of any of the information contained herein or that this message or any of its attachments is free of viruses.
On 25.03.2010 19:15, Stewart, Robert wrote:
- The library should not reinvent wheels. In particular, custom implementations of TSS and RW mutex seems like a very bad idea.
I have noted that during the review that these tools were implemented for a reason. They make the library faster and lighter in terms of memory footprint. They allow to avoid including Boost.Thread headers in public headers of Boost.Log, which also speeds up the compilation. I think these matters are more important than having the code dispersion they add.
I'd like to know the real effect of using off-the-Boost-shelf components rather than your special sauce. That is, I'd like to know the performance and compilation tradeoffs for them before commenting further.
Here are the numbers regarding the memory footprint and the lightness: On Linux x86_64, GCC 4.4: ========================= $ g++ -I. -E ./boost/log/detail/thread_specific.hpp >bl_tss.pp $ g++ -I. -E ./boost/thread/tss.hpp >bt_tss.pp $ g++ -I. -E ./boost/log/detail/light_rw_mutex.hpp >bl_rw.pp $ g++ -I. -E ./boost/thread/shared_mutex.hpp >bt_rw.pp $ ls -la 82157 2010-03-25 19:26 bl_rw.pp 157431 2010-03-25 19:24 bl_tss.pp 1205108 2010-03-25 19:26 bt_rw.pp 347240 2010-03-25 19:24 bt_tss.pp sizeof(shared_mutex) = 192, sizeof(light_rw_mutex) = 56 On Windows, x64, MSVC 9: ======================== E:\Sources\_SVN\boost-release>cl -I. /TP /EHsc -E boost\log\detail\thread_specific.hpp >bl_tss.pp Microsoft (R) C/C++ Optimizing Compiler Version 15.00.30729.01 for x64 Copyright (C) Microsoft Corporation. All rights reserved. thread_specific.hpp E:\Sources\_SVN\boost-release>cl -I. /TP /EHsc -E boost\thread\tss.hpp
bt_tss.pp Microsoft (R) C/C++ Optimizing Compiler Version 15.00.30729.01 for x64 Copyright (C) Microsoft Corporation. All rights reserved.
tss.hpp E:\Sources\_SVN\boost-release>cl -I. /TP /EHsc -DBOOST_LOG_USE_WINNT6_API -E boost\log\detail\light_rw_mutex.hpp >bl_rw.pp Microsoft (R) C/C++ Optimizing Compiler Version 15.00.30729.01 for x64 Copyright (C) Microsoft Corporation. All rights reserved. light_rw_mutex.hpp E:\Sources\_SVN\boost-release>cl -I. /TP /EHsc -E boost\thread\shared_mutex.hpp >bt_rw.pp Microsoft (R) C/C++ Optimizing Compiler Version 15.00.30729.01 for x64 Copyright (C) Microsoft Corporation. All rights reserved. shared_mutex.hpp E:\Sources\_SVN\boost-release>dir Volume in drive E is Dev Volume Serial Number is E050-708C Directory of E:\Sources\_SVN\boost-release 25.03.2010 19:52 394 755 bl_rw.pp 25.03.2010 19:55 776 216 bl_tss.pp 25.03.2010 19:51 4 360 402 bt_rw.pp 25.03.2010 19:49 3 823 468 bt_tss.pp E:\Sources\_SVN\boost-release>cl -I. /TP /EHsc -DBOOST_LOG_USE_WINNT6_API -DBOOST_ALL_NO_LIB size_test.cpp Microsoft (R) C/C++ Optimizing Compiler Version 15.00.30729.01 for x64 Copyright (C) Microsoft Corporation. All rights reserved. size_test.cpp Microsoft (R) Incremental Linker Version 9.00.30729.01 Copyright (C) Microsoft Corporation. All rights reserved. /out:size_test.exe size_test.obj E:\Sources\_SVN\boost-release>size_test.exe sizeof(shared_mutex) = 48, sizeof(light_rw_mutex) = 8 I draw your attention to that a typical MT logger (at least, ones provided by the library) contains a rw mutex, which makes it a significant addition if loggers are used as class members. Regarding performance, Boost.Log TSS relies on raw pthread/WinAPI calls, while Boost.Thread TSS adds to that a logarithmic lookup along all thread_specific_ptrs in the application, so it definitely cannot be faster. Also, using thread_specific_ptr to manage a tiny thing like an integer (the severity level) is an overkill since it would require several memory allocations per thread, one of which is for shared_ptr, IIRC. That's opposed to no allocations at all for Boost.Log TSS.
Andrey Semashev wrote:
On 03/25/2010 01:28 AM, Vladimir Prus wrote:
First of all, I would like to thank Andrey for this submission and the active discussion during the review. I also would like to thank everybody who participated in the review.
The comments on the library were generally positive. Several reviewers have tried it in practical projects already, and found it superior to other libraries. It clearly will benefit the users of Boost right away. Therefore, the library is accepted. Congratulations, Andrey!
Thank you, Vladimir. You did a really great job both as a review manager and as a reviewer.
I would also like to thank all the reviewers, I needed this kind of feedback. My special appreciation goes to Steven Watanabe, who gave thorough reading to the documentation and provided the needed corrections, and also gave many fruitful suggestions about the code.
I agree to most prerequisites needed for the final inclusion, with a sole exception of this one:
Critical issues ===============
These issues must be addressed, and be passed through informal mini-review, before the library can be added to SVN.
- The library should not reinvent wheels. In particular, custom implementations of TSS and RW mutex seems like a very bad idea.
I have noted that during the review that these tools were implemented for a reason. They make the library faster and lighter in terms of memory footprint. They allow to avoid including Boost.Thread headers in public headers of Boost.Log, which also speeds up the compilation. I think these matters are more important than having the code dispersion they add.
I remember that it was you who expressed the concern about these tools. Do you think this issue should really be a hard requirement for the library to be included in Boost?
I don't think it's hard requirement to have Boost.Log use specific current implementation of TSS and RW mutex from Boost.Thread. Rather, this point, as well as all other "critical issues" suggest something that looks really bad, and will be a focus of the follow-up mini review. If you demonstrate that your implementation is better, and it significantly affects users of libraries, and you have attempted to get Boost.Thread improved (either by changing current implementation, or by adding yours as alternative) and that failed, then I don't think anybody is going to suggest you still use Boost.Thread facilities. But I do hope that it will be possible to make Boost.Thread meet your requirements. Thanks, Volodya
participants (4)
-
Andrey Semashev -
Stewart, Robert -
Vladimir Prus -
Vladimir Prus