Remove multi-thread synchronization from CRT logger wrapper #2885
Conversation
| { | ||
| // SDK CRT logger is terminated, fallback to an original/previous CRT logger |
There was a problem hiding this comment.
I just thought about it more and realized that we don't control / own "original" logger, so better to not try to over-smart ourselves.
| { | ||
| Array<char> outputBuff(requiredLength); | ||
| #ifdef _WIN32 | ||
| vsnprintf_s(outputBuff.GetUnderlyingData(), requiredLength, _TRUNCATE, formatStr, args); |
There was a problem hiding this comment.
exactly the same logic is in the main SDK logger, so just call it instead.
65bf755 to
349407b
Compare
| ss << DateTime::Now().CalculateGmtTimeWithMsPrecision() << " " << tag << " [" << std::this_thread::get_id() << "] "; | ||
|
|
||
| return ss.str(); | ||
| static Aws::String CreateLogPrefixLine(LogLevel logLevel, const char* tag, const size_t statementSize = 80) |
There was a problem hiding this comment.
so what happens here is the attempt to reduce number of memory allocations to 1 (from many) per logging statement processing.
There are still additional allocations from CalculateGmtTimeWithMsPrecision and <<std::this_thread::get_id but the rest of allocations (and stack-allocated Aws::Array) are eliminated.
349407b to
b56403a
Compare
| static const size_t THREAD_ID_LEN = [](){ | ||
| Aws::StringStream strStr; | ||
| strStr << std::this_thread::get_id(); | ||
| return strStr.str().size(); |
There was a problem hiding this comment.
this runs only once per application instance, right?
Is it guaranteed that every thread's ID is the same length? If this were just an integer on some platform, I could imagine different threads having different length integers. I guess it's still safe, since this is just for a reserve() call...
Simpler to just have a hard-coded high guess? Or overestimate somewhere and just like, + 10 to prefixLen?
b56403a to
b25c617
Compare
b25c617 to
c509267
Compare
| [&](){ return m_logsProcessed.load() == 0; }); | ||
| void DefaultCRTLogSystem::SetLogLevel(LogLevel logLevel) | ||
| { | ||
| AWS_UNREFERENCED_PARAM(logLevel); // will use one from the main SDK logger |
There was a problem hiding this comment.
should we set the log level in main SDK logger instead of no-op?
| const size_t oldStatementSz = statement.size(); | ||
| const size_t newStatementSz = oldStatementSz + requiredLength; | ||
| // assert(statement.capacity() >= newStatementSz); | ||
| // assert(statement.capacity() < 3 * newStatementSz / 2); |
There was a problem hiding this comment.
nit: commented out asserts
c509267 to
73ba9a8
Compare
73ba9a8 to
b321e05
Compare
| // GetLogSystem returns a raw pointer | ||
| // so this is a hack to let all other threads finish their log statement after getting a LogSystem pointer | ||
| // otherwise we would need to perform ref-counting on each logging statement | ||
| auto tmpLogger = std::move(CRTLogSystem); |
There was a problem hiding this comment.
This approach would not be necessary if the logger's lifetime always exceeded the CRT. We should be able to shutdown the CRT and then call aws_logger_set(nullptr) and have no need for time-based quiesces which are ultimately a flawed approach (someone will always exceed the sleep time). We would then get all the log messages from CRT shutdown. We're shutting down CRT logging before CRT is stopped, and hooking CRT logging in after CRT is started. There's a more fundamental problem here.
Issue #, if available:
Description of changes:
Check all that applies:
Check which platforms you have built SDK on to verify the correctness of this PR.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.