[LOGCXX-546] Prevent unnecessary locking when determining the logging level #90
Conversation
…en using disabled logging statements
…g in throughputtests
…eds to hold its setting
|
Looks good to me. I also get rather dramatic increases in the number of log messages/second when logging is disabled(approx. 10x on my machine). Other stats are about 5-10% better. |
|
Do you think this performance fix warrants a release (0.12.3) before next_stable is ready? |
|
My current thought is that we'll do a 0.13.0 release in a few weeks; there are a number of fixes for that already in master. next_stable probably won't be for several months at least, since that contains bigger changes(e.g. API breaking, other assorted fixes that are currently hard to fix with the current state of the code without touching pretty much everything). |
|
Before: After: |
|
Is this good to merge? |
|
Merged using https://gitbox.apache.org |
|
|
||
| /** | ||
| Return the the LoggerRepository where this | ||
| <code>Logger</code> is attached. | ||
| */ | ||
| log4cxx::spi::LoggerRepositoryWeakPtr getLoggerRepository() const; | ||
| log4cxx::spi::LoggerRepository* getLoggerRepository() const; |
There was a problem hiding this comment.
After thinking about this for a few days, would it be better to lave this as LoggerRepositoryWeakPtr? The signature of this method did change about a year ago(so some people have started fixing their code to use it), and I would hate to ping-pong back and forth over the signature of the method.
If we just change the Logger::setHierarchy to lock the pointer that is passed in(and store the raw pointer as a new member), I think that would be less of a change and less likely to break things.
I don't have a problem changing it in the next_stable branch, since my hope would be that we can clean up the initialization sequence of things to make it more clean as well.
Thoughts?
There was a problem hiding this comment.
I will measure the impact with 'm->priv->repository' a weak pointer.
With this PR included, LOG4CXX_322 is still a probem. I still eventually get a SegFault in multithreadtest using the following:
while ctest -R multi; do i=`expr $i + 1`; echo Done $i; done;
However, with 'm->priv->repository' a weak pointer, getLoggerRepository() should return a spi::LoggerRepositoryPtr (i.e. getLoggerRepository() should call lock()), so the caller does not need to call lock().
There was a problem hiding this comment.
The per thread throughput is less half the single thread throughput when using a std::weak_ptr in Logger (Results are in #94)
Also, LOGCXX-322 is still a problem when using a std::weak_ptr in Logger.
There was a problem hiding this comment.
I agree that using the raw pointer would be faster, but what I'm saying is why don't we store the raw pointer and the weak pointer? That lets the API stay stable, plus gives us the benefits of speed since we only use the raw pointer.
pseudo-code:
Logger.h:
private:
log4cxx::spi::LoggerRepositoryWeakPtr repository;
log4cxx::spi::LoggerRepository* repository_raw;
Logger.cpp:
void Logger::setHierarchy(spi::LoggerRepositoryWeakPtr repository1)
{
this->repository = repository1;
this->repository_raw = repository1.lock().get();
}
bool Logger::isTraceEnabled() const
{
if (!repository_raw || repository_raw->isDisabled(Level::TRACE_INT))
{
return false;
}
return true;
}
There was a problem hiding this comment.
Given the std::weak_ptr does not fix LOGCXX-322, why is it is useful?
There was a problem hiding this comment.
Even if it doesn't completely fix it, there are at least two reasons that I can think of:
- Since it is part of the public API, it forces clients to use smart pointers(better expressing the intent) and keeps it similar to other parts of the API
- Keeps things stable for users who need to get the repository from the logger
There was a problem hiding this comment.
I expect that there's also a better way to fix this that requires some bigger architecture changes; I don't know what those are at the moment, but hopefully we can fix it for next_stable.
There was a problem hiding this comment.
OK, I agree forceing clients to use smart pointers better expresses the intent.
So, I will change getLoggerRepository() to
log4cxx::spi::LoggerRepositoryPtr getLoggerRepository() const
OK?
throughputtests after these changes:
throughput output before these changes: