You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@logging.apache.org by GitBox <gi...@apache.org> on 2022/09/12 07:50:19 UTC
[GitHub] [logging-log4cxx] swebb2066 opened a new pull request, #130: Reduce logging overhead by holding references
swebb2066 opened a new pull request, #130:
URL: https://github.com/apache/logging-log4cxx/pull/130
1. Do not copy the (const global) thread name into every LoggingEvent
2. Thread user name was always empty
3. Fix possible compile error with BORLANDC
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4cxx] ams-tschoening commented on a diff in pull request #130: Reduce logging overhead by holding references
Posted by GitBox <gi...@apache.org>.
ams-tschoening commented on code in PR #130:
URL: https://github.com/apache/logging-log4cxx/pull/130#discussion_r972653834
##########
src/main/cpp/loggingevent.cpp:
##########
@@ -297,40 +302,62 @@ LoggingEvent::KeySet LoggingEvent::getPropertyKeySet() const
}
-const LogString LoggingEvent::getCurrentThreadName()
+const LogString& LoggingEvent::getCurrentThreadName()
{
-#if APR_HAS_THREADS
- LOG4CXX_THREAD_LOCAL LogString thread_name;
+#if defined(_WIN32)
+ using ThreadIdType = DWORD;
+ ThreadIdType threadId = GetCurrentThreadId();
+#elif APR_HAS_THREADS
+ using ThreadIdType = apr_os_thread_t;
+ ThreadIdType threadId = apr_os_thread_current();
+#else
+ using ThreadIdType = int;
+ ThreadIdType threadId = 0;
+#endif
- if ( thread_name.size() )
+#if defined(__BORLANDC__)
Review Comment:
From my understanding, `__BORLANDC__` either targets the product as whole including the legacy C++ compiler or the legacy compiler only. Your added code is most likely only compatible with the CLANG based compiler, which can be targeted using `__clang__`. So either the condition needs to be made more explicit to target Borland+Clang without ARM or simply only add such compatibility if someone really asks for it and is then able to test it as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4cxx] swebb2066 commented on a diff in pull request #130: Reduce logging overhead by holding references
Posted by GitBox <gi...@apache.org>.
swebb2066 commented on code in PR #130:
URL: https://github.com/apache/logging-log4cxx/pull/130#discussion_r969007525
##########
src/main/cpp/loggingevent.cpp:
##########
@@ -297,40 +302,62 @@ LoggingEvent::KeySet LoggingEvent::getPropertyKeySet() const
}
-const LogString LoggingEvent::getCurrentThreadName()
+const LogString& LoggingEvent::getCurrentThreadName()
{
-#if APR_HAS_THREADS
- LOG4CXX_THREAD_LOCAL LogString thread_name;
+#if defined(_WIN32)
+ using ThreadIdType = DWORD;
+ ThreadIdType threadId = GetCurrentThreadId();
+#elif APR_HAS_THREADS
+ using ThreadIdType = apr_os_thread_t;
+ ThreadIdType threadId = apr_os_thread_current();
+#else
+ using ThreadIdType = int;
+ ThreadIdType threadId = 0;
+#endif
- if ( thread_name.size() )
+#if defined(__BORLANDC__)
Review Comment:
I do not have a Borland compiler. My testing approach is to change the "#if __BORLANDC__" to "#if !__BORLANDC__", build and run tests
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4cxx] rm5248 commented on a diff in pull request #130: Reduce logging overhead by holding references
Posted by GitBox <gi...@apache.org>.
rm5248 commented on code in PR #130:
URL: https://github.com/apache/logging-log4cxx/pull/130#discussion_r973400713
##########
src/main/cpp/loggingevent.cpp:
##########
@@ -297,16 +302,19 @@ LoggingEvent::KeySet LoggingEvent::getPropertyKeySet() const
}
-const LogString LoggingEvent::getCurrentThreadName()
+const LogString& LoggingEvent::getCurrentThreadName()
{
#if APR_HAS_THREADS
- LOG4CXX_THREAD_LOCAL LogString thread_name;
-
- if ( thread_name.size() )
+ thread_local LogString thread_id_string;
Review Comment:
This is probably going to fail if the compiler does not have `thread_local` support(hence why the `LOG4CXX_THREAD_LOCAL` macro existed). Of course if `thread_local` does not exist, we can't return the `LogString` by reference since it would be declared on the stack and thus go away when the method returns.
Although cppreference does show that C++Builder [supports thread-local storage](https://en.cppreference.com/w/cpp/compiler_support/11)... do we need to test that and see if it actually does even if it claims it doesn't?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4cxx] rm5248 commented on a diff in pull request #130: Reduce logging overhead by holding references
Posted by GitBox <gi...@apache.org>.
rm5248 commented on code in PR #130:
URL: https://github.com/apache/logging-log4cxx/pull/130#discussion_r972542642
##########
src/main/cpp/loggingevent.cpp:
##########
@@ -297,40 +302,62 @@ LoggingEvent::KeySet LoggingEvent::getPropertyKeySet() const
}
-const LogString LoggingEvent::getCurrentThreadName()
+const LogString& LoggingEvent::getCurrentThreadName()
{
-#if APR_HAS_THREADS
- LOG4CXX_THREAD_LOCAL LogString thread_name;
+#if defined(_WIN32)
+ using ThreadIdType = DWORD;
+ ThreadIdType threadId = GetCurrentThreadId();
+#elif APR_HAS_THREADS
+ using ThreadIdType = apr_os_thread_t;
+ ThreadIdType threadId = apr_os_thread_current();
+#else
+ using ThreadIdType = int;
+ ThreadIdType threadId = 0;
+#endif
- if ( thread_name.size() )
+#if defined(__BORLANDC__)
Review Comment:
It seems that `thread_local` is still not supported even in the latest compiler: https://docwiki.embarcadero.com/RADStudio/Alexandria/en/Modern_C%2B%2B_Language_Features_Compliance_Status
This is rather unfortunate, because that is very useful for things like this.
It seems like we should do the following:
- Remove the Borland-specific code that was added here. Changing the log event to hold a reference instead of a copy is a good change and should be kept
- Have a discussion on the mailing as to the level of C++ support and the compilers supported. We should be able to support the Borland compilers without too much trouble as long as we keep away from certain features.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4cxx] swebb2066 commented on a diff in pull request #130: Reduce logging overhead by holding references
Posted by GitBox <gi...@apache.org>.
swebb2066 commented on code in PR #130:
URL: https://github.com/apache/logging-log4cxx/pull/130#discussion_r972549808
##########
src/main/cpp/loggingevent.cpp:
##########
@@ -297,40 +302,62 @@ LoggingEvent::KeySet LoggingEvent::getPropertyKeySet() const
}
-const LogString LoggingEvent::getCurrentThreadName()
+const LogString& LoggingEvent::getCurrentThreadName()
{
-#if APR_HAS_THREADS
- LOG4CXX_THREAD_LOCAL LogString thread_name;
+#if defined(_WIN32)
+ using ThreadIdType = DWORD;
+ ThreadIdType threadId = GetCurrentThreadId();
+#elif APR_HAS_THREADS
+ using ThreadIdType = apr_os_thread_t;
+ ThreadIdType threadId = apr_os_thread_current();
+#else
+ using ThreadIdType = int;
+ ThreadIdType threadId = 0;
+#endif
- if ( thread_name.size() )
+#if defined(__BORLANDC__)
Review Comment:
For the record, the BORLANDC macro was introduced because the following page [BCC compliance statas](http://docwiki.embarcadero.com/RADStudio/Sydney/en/Modern_C%2B%2B_Language_Features_Compliance_Status) states thread_local is (strangly) only supported by [BCCIOSARM64](https://docwiki.embarcadero.com/RADStudio/Sydney/en/BCCIOSARM64).
Any idea why they would remove support thread_local on some architectures?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4cxx] swebb2066 commented on a diff in pull request #130: Reduce logging overhead by holding references
Posted by GitBox <gi...@apache.org>.
swebb2066 commented on code in PR #130:
URL: https://github.com/apache/logging-log4cxx/pull/130#discussion_r969014625
##########
src/main/cpp/loggingevent.cpp:
##########
@@ -297,40 +302,62 @@ LoggingEvent::KeySet LoggingEvent::getPropertyKeySet() const
}
-const LogString LoggingEvent::getCurrentThreadName()
+const LogString& LoggingEvent::getCurrentThreadName()
{
-#if APR_HAS_THREADS
- LOG4CXX_THREAD_LOCAL LogString thread_name;
+#if defined(_WIN32)
+ using ThreadIdType = DWORD;
+ ThreadIdType threadId = GetCurrentThreadId();
+#elif APR_HAS_THREADS
+ using ThreadIdType = apr_os_thread_t;
+ ThreadIdType threadId = apr_os_thread_current();
+#else
+ using ThreadIdType = int;
+ ThreadIdType threadId = 0;
+#endif
- if ( thread_name.size() )
+#if defined(__BORLANDC__)
Review Comment:
A templated class(ThreadLocal<LogString>) may be worthwhile if there is a second need.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4cxx] swebb2066 commented on a diff in pull request #130: Reduce logging overhead by holding references
Posted by GitBox <gi...@apache.org>.
swebb2066 commented on code in PR #130:
URL: https://github.com/apache/logging-log4cxx/pull/130#discussion_r972549808
##########
src/main/cpp/loggingevent.cpp:
##########
@@ -297,40 +302,62 @@ LoggingEvent::KeySet LoggingEvent::getPropertyKeySet() const
}
-const LogString LoggingEvent::getCurrentThreadName()
+const LogString& LoggingEvent::getCurrentThreadName()
{
-#if APR_HAS_THREADS
- LOG4CXX_THREAD_LOCAL LogString thread_name;
+#if defined(_WIN32)
+ using ThreadIdType = DWORD;
+ ThreadIdType threadId = GetCurrentThreadId();
+#elif APR_HAS_THREADS
+ using ThreadIdType = apr_os_thread_t;
+ ThreadIdType threadId = apr_os_thread_current();
+#else
+ using ThreadIdType = int;
+ ThreadIdType threadId = 0;
+#endif
- if ( thread_name.size() )
+#if defined(__BORLANDC__)
Review Comment:
For the record, the BORLANDC macro was introduced because the following page [BCC compliance statas](http://docwiki.embarcadero.com/RADStudio/Sydney/en/Modern_C%2B%2B_Language_Features_Compliance_Status) states thread_local is (strangly) only supported by [BCCIOSARM64](https://docwiki.embarcadero.com/RADStudio/Sydney/en/BCCIOSARM64).
Any idea why they would remove support for thread_local on some architectures?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4cxx] rm5248 commented on a diff in pull request #130: Reduce logging overhead by holding references
Posted by GitBox <gi...@apache.org>.
rm5248 commented on code in PR #130:
URL: https://github.com/apache/logging-log4cxx/pull/130#discussion_r968985287
##########
src/main/cpp/loggingevent.cpp:
##########
@@ -297,40 +302,62 @@ LoggingEvent::KeySet LoggingEvent::getPropertyKeySet() const
}
-const LogString LoggingEvent::getCurrentThreadName()
+const LogString& LoggingEvent::getCurrentThreadName()
{
-#if APR_HAS_THREADS
- LOG4CXX_THREAD_LOCAL LogString thread_name;
+#if defined(_WIN32)
+ using ThreadIdType = DWORD;
+ ThreadIdType threadId = GetCurrentThreadId();
+#elif APR_HAS_THREADS
+ using ThreadIdType = apr_os_thread_t;
+ ThreadIdType threadId = apr_os_thread_current();
+#else
+ using ThreadIdType = int;
+ ThreadIdType threadId = 0;
+#endif
- if ( thread_name.size() )
+#if defined(__BORLANDC__)
Review Comment:
Do you have access to a Borland compiler for this? I know that Thorsten uses a Borland-derived compiler.
I ask because I was thinking about this the other day, there is already a ThreadLocal class in the sources. I was figuring that it was obsolete at this point, but maybe it's still useful for Borland compilers? Maybe make it a templated class(`ThreadLocal<LogString>`)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4cxx] swebb2066 commented on a diff in pull request #130: Reduce logging overhead by holding references
Posted by GitBox <gi...@apache.org>.
swebb2066 commented on code in PR #130:
URL: https://github.com/apache/logging-log4cxx/pull/130#discussion_r972549808
##########
src/main/cpp/loggingevent.cpp:
##########
@@ -297,40 +302,62 @@ LoggingEvent::KeySet LoggingEvent::getPropertyKeySet() const
}
-const LogString LoggingEvent::getCurrentThreadName()
+const LogString& LoggingEvent::getCurrentThreadName()
{
-#if APR_HAS_THREADS
- LOG4CXX_THREAD_LOCAL LogString thread_name;
+#if defined(_WIN32)
+ using ThreadIdType = DWORD;
+ ThreadIdType threadId = GetCurrentThreadId();
+#elif APR_HAS_THREADS
+ using ThreadIdType = apr_os_thread_t;
+ ThreadIdType threadId = apr_os_thread_current();
+#else
+ using ThreadIdType = int;
+ ThreadIdType threadId = 0;
+#endif
- if ( thread_name.size() )
+#if defined(__BORLANDC__)
Review Comment:
For the record, the BORLANDC macro was introduced because the following page [BCC compliance status](http://docwiki.embarcadero.com/RADStudio/Sydney/en/Modern_C%2B%2B_Language_Features_Compliance_Status) states thread_local is (strangly) only supported by [BCCIOSARM64](https://docwiki.embarcadero.com/RADStudio/Sydney/en/BCCIOSARM64).
Any idea why they would remove support for thread_local on some architectures?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4cxx] ams-tschoening commented on a diff in pull request #130: Reduce logging overhead by holding references
Posted by GitBox <gi...@apache.org>.
ams-tschoening commented on code in PR #130:
URL: https://github.com/apache/logging-log4cxx/pull/130#discussion_r969206179
##########
src/main/cpp/loggingevent.cpp:
##########
@@ -297,40 +302,62 @@ LoggingEvent::KeySet LoggingEvent::getPropertyKeySet() const
}
-const LogString LoggingEvent::getCurrentThreadName()
+const LogString& LoggingEvent::getCurrentThreadName()
{
-#if APR_HAS_THREADS
- LOG4CXX_THREAD_LOCAL LogString thread_name;
+#if defined(_WIN32)
+ using ThreadIdType = DWORD;
+ ThreadIdType threadId = GetCurrentThreadId();
+#elif APR_HAS_THREADS
+ using ThreadIdType = apr_os_thread_t;
+ ThreadIdType threadId = apr_os_thread_current();
+#else
+ using ThreadIdType = int;
+ ThreadIdType threadId = 0;
+#endif
- if ( thread_name.size() )
+#if defined(__BORLANDC__)
Review Comment:
Not sure I'm able to follow this discussion: What's the overall reason of introducing the BORLANDC-specific code at all? What are you actually trying to target, the IDE or the 32 Bit legacy compiler or the newer CLANG one or ...? The legacy 32 Bit compiler doesn't even fully support C++11, so I pretty much doubt that the available code works at all. What do you think using `#if !BORLANDC` tell you regarding the limitations of the legacy compiler? :-)
https://docwiki.embarcadero.com/RADStudio/Sydney/en/Predefined_Macros#C.2B.2B_Compiler_Versions_in_Predefined_Macros
https://docwiki.embarcadero.com/RADStudio/Sydney/en/Clang-enhanced_C%2B%2B_Compilers#Predefined_Macros
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4cxx] swebb2066 merged pull request #130: Reduce logging overhead by holding references
Posted by GitBox <gi...@apache.org>.
swebb2066 merged PR #130:
URL: https://github.com/apache/logging-log4cxx/pull/130
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4cxx] rm5248 commented on pull request #130: Reduce logging overhead by holding references
Posted by GitBox <gi...@apache.org>.
rm5248 commented on PR #130:
URL: https://github.com/apache/logging-log4cxx/pull/130#issuecomment-1250065384
Looks pretty good to me at the moment.
I downloaded the community version of C++ builder to test out the `thread_local` stuff, I'll take a look at it this weekend to figure out if it is actually supported or not.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4cxx] rm5248 commented on pull request #130: Reduce logging overhead by holding references
Posted by GitBox <gi...@apache.org>.
rm5248 commented on PR #130:
URL: https://github.com/apache/logging-log4cxx/pull/130#issuecomment-1250069047
I just ran the following code to test the `thread_local` using RADStudio/C++Builder:
```
#pragma hdrstop
#pragma argsused
#ifdef _WIN32
#include <tchar.h>
#else
typedef char _TCHAR;
#define _tmain main
#endif
#include <stdio.h>
#include <conio.h>
#include <thread>
thread_local int test_var;
void printTestVarValue(){
printf( "thread 0x%08X test_var %d\n", std::this_thread::get_id(), test_var );
}
void setTestVar(){
test_var = 5;
printTestVarValue();
std::this_thread::sleep_for(std::chrono::seconds(1));
printTestVarValue();
}
int _tmain(int argc, _TCHAR* argv[])
{
test_var = 1;
printTestVarValue();
std::thread th( setTestVar );
for( int x = 0; x < 10; x++ ){
std::this_thread::sleep_for(std::chrono::milliseconds(100));
printTestVarValue();
}
th.join();
printTestVarValue();
getch();
return 0;
}
```
The output does indicate that the `thread_local` is working:
```
thread 0x00000394 test_var 1
thread 0x000004C8 test_var 5
thread 0x00000394 test_var 1
thread 0x00000394 test_var 1
thread 0x00000394 test_var 1
thread 0x00000394 test_var 1
thread 0x00000394 test_var 1
thread 0x00000394 test_var 1
thread 0x00000394 test_var 1
thread 0x00000394 test_var 1
thread 0x00000394 test_var 1
thread 0x000004C8 test_var 5
thread 0x00000394 test_var 1
thread 0x00000394 test_var 1
```
So it looks like concerns over `thread_local` support may be much ado about nothing. Thorsten, would you be able to run that same program in your environment and see if you get the same results?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4cxx] ams-tschoening commented on a diff in pull request #130: Reduce logging overhead by holding references
Posted by GitBox <gi...@apache.org>.
ams-tschoening commented on code in PR #130:
URL: https://github.com/apache/logging-log4cxx/pull/130#discussion_r969206179
##########
src/main/cpp/loggingevent.cpp:
##########
@@ -297,40 +302,62 @@ LoggingEvent::KeySet LoggingEvent::getPropertyKeySet() const
}
-const LogString LoggingEvent::getCurrentThreadName()
+const LogString& LoggingEvent::getCurrentThreadName()
{
-#if APR_HAS_THREADS
- LOG4CXX_THREAD_LOCAL LogString thread_name;
+#if defined(_WIN32)
+ using ThreadIdType = DWORD;
+ ThreadIdType threadId = GetCurrentThreadId();
+#elif APR_HAS_THREADS
+ using ThreadIdType = apr_os_thread_t;
+ ThreadIdType threadId = apr_os_thread_current();
+#else
+ using ThreadIdType = int;
+ ThreadIdType threadId = 0;
+#endif
- if ( thread_name.size() )
+#if defined(__BORLANDC__)
Review Comment:
Not sure I'm able to follow this discussion: What's the overall reason of introducing the BORLANDC-specific code at all? What are you actually trying to target, the IDE or the 32 Bit legacy compiler or the newer CLANG one or ...? The legacy 32 Bit compiler doesn't even fully support C++11, so I pretty much doubt that the available code works at all. What do you think using `#if !BORLANDC` tell you? :-)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4cxx] swebb2066 commented on a diff in pull request #130: Reduce logging overhead by holding references
Posted by GitBox <gi...@apache.org>.
swebb2066 commented on code in PR #130:
URL: https://github.com/apache/logging-log4cxx/pull/130#discussion_r970144384
##########
src/main/cpp/loggingevent.cpp:
##########
@@ -297,40 +302,62 @@ LoggingEvent::KeySet LoggingEvent::getPropertyKeySet() const
}
-const LogString LoggingEvent::getCurrentThreadName()
+const LogString& LoggingEvent::getCurrentThreadName()
{
-#if APR_HAS_THREADS
- LOG4CXX_THREAD_LOCAL LogString thread_name;
+#if defined(_WIN32)
+ using ThreadIdType = DWORD;
+ ThreadIdType threadId = GetCurrentThreadId();
+#elif APR_HAS_THREADS
+ using ThreadIdType = apr_os_thread_t;
+ ThreadIdType threadId = apr_os_thread_current();
+#else
+ using ThreadIdType = int;
+ ThreadIdType threadId = 0;
+#endif
- if ( thread_name.size() )
+#if defined(__BORLANDC__)
Review Comment:
Using #if !BORLANDC checks that the code compiles and runs - it tells me nothing regarding the limitations of the legacy compiler support for thread local .
Do you suggest we remove the BORLANDC tests?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4cxx] swebb2066 commented on pull request #130: Reduce logging overhead by holding references
Posted by GitBox <gi...@apache.org>.
swebb2066 commented on PR #130:
URL: https://github.com/apache/logging-log4cxx/pull/130#issuecomment-1250211411
When getCurrentThreadName() uses thread_local there is a crash in multithreadtest. The crash does not happen when the alternate code in getCurrentThreadName() is used.
See [gcc thread_local destructor cause use after free](https://github.com/msys2/MINGW-packages/issues/2519)
which has these links:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58142
https://sourceforge.net/p/mingw-w64/bugs/727/
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80816
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4cxx] ams-tschoening commented on a diff in pull request #130: Reduce logging overhead by holding references
Posted by GitBox <gi...@apache.org>.
ams-tschoening commented on code in PR #130:
URL: https://github.com/apache/logging-log4cxx/pull/130#discussion_r970362623
##########
src/main/cpp/loggingevent.cpp:
##########
@@ -297,40 +302,62 @@ LoggingEvent::KeySet LoggingEvent::getPropertyKeySet() const
}
-const LogString LoggingEvent::getCurrentThreadName()
+const LogString& LoggingEvent::getCurrentThreadName()
{
-#if APR_HAS_THREADS
- LOG4CXX_THREAD_LOCAL LogString thread_name;
+#if defined(_WIN32)
+ using ThreadIdType = DWORD;
+ ThreadIdType threadId = GetCurrentThreadId();
+#elif APR_HAS_THREADS
+ using ThreadIdType = apr_os_thread_t;
+ ThreadIdType threadId = apr_os_thread_current();
+#else
+ using ThreadIdType = int;
+ ThreadIdType threadId = 0;
+#endif
- if ( thread_name.size() )
+#if defined(__BORLANDC__)
Review Comment:
> Do you suggest we remove the BORLANDC tests?
Yes. Unless it can be tested and really addresses known limitations of the old legacy compiler, new code targeting that shouldn't be introduced. Changing the condition and testing with some other compiler doesn't mean anything, while the introduced code makes maintenance unnecessary difficult. I wasn't able to successfully build log4cxx using the legacy compiler for a long time already. The following is the docs for supporting the keyword `auto` and it doesn't seem to be supported in BCC32 at all. I very much doubt that the lambda is supported as well.
> The C++98 Definition is supported by previous-generation C++ compilers ([BCC32](https://docwiki.embarcadero.com/RADStudio/Sydney/en/BCC32)).
> The C++11 Definition is supported by [C++11-compatible Clang-enhanced C++ compilers](https://docwiki.embarcadero.com/RADStudio/Sydney/en/Clang-enhanced_C%2B%2B_Compilers).
https://docwiki.embarcadero.com/RADStudio/Sydney/en/Auto
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4cxx] swebb2066 commented on a diff in pull request #130: Reduce logging overhead by holding references
Posted by GitBox <gi...@apache.org>.
swebb2066 commented on code in PR #130:
URL: https://github.com/apache/logging-log4cxx/pull/130#discussion_r970144384
##########
src/main/cpp/loggingevent.cpp:
##########
@@ -297,40 +302,62 @@ LoggingEvent::KeySet LoggingEvent::getPropertyKeySet() const
}
-const LogString LoggingEvent::getCurrentThreadName()
+const LogString& LoggingEvent::getCurrentThreadName()
{
-#if APR_HAS_THREADS
- LOG4CXX_THREAD_LOCAL LogString thread_name;
+#if defined(_WIN32)
+ using ThreadIdType = DWORD;
+ ThreadIdType threadId = GetCurrentThreadId();
+#elif APR_HAS_THREADS
+ using ThreadIdType = apr_os_thread_t;
+ ThreadIdType threadId = apr_os_thread_current();
+#else
+ using ThreadIdType = int;
+ ThreadIdType threadId = 0;
+#endif
- if ( thread_name.size() )
+#if defined(__BORLANDC__)
Review Comment:
Using #if !BORLANDC checks that the code compiles and runs - it tells me nothing regarding the limitations of the legacy compiler support for thread local .
Do yo suggest we remove the BORLANDC tests?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4cxx] ams-tschoening commented on pull request #130: Reduce logging overhead by holding references
Posted by GitBox <gi...@apache.org>.
ams-tschoening commented on PR #130:
URL: https://github.com/apache/logging-log4cxx/pull/130#issuecomment-1250086257
Doesn't work for my RAD Studio 10.2, which defaults to the legacy BCC32 compiler and targets 32 Bit Windows:
```
[bcc32 Error] File2.cpp(13): E2209 Unable to open include file 'thread'
[bcc32 Error] File2.cpp(15): E2141 Declaration syntax error
[bcc32 Error] File2.cpp(18): E2316 'this_thread' is not a member of 'std'
Full parser context
File2.cpp(17): parsing: void printTestVarValue()
[bcc32 Error] File2.cpp(18): E2121 Function call missing )
Full parser context
File2.cpp(17): parsing: void printTestVarValue()
[bcc32 Error] File2.cpp(22): E2451 Undefined symbol 'test_var'
Full parser context
File2.cpp(21): parsing: void setTestVar()
[bcc32 Error] File2.cpp(24): E2316 'this_thread' is not a member of 'std'
Full parser context
File2.cpp(21): parsing: void setTestVar()
[bcc32 Error] File2.cpp(24): E2379 Statement missing ;
Full parser context
File2.cpp(21): parsing: void setTestVar()
[bcc32 Error] File2.cpp(30): E2451 Undefined symbol 'test_var'
Full parser context
File2.cpp(29): parsing: int main(int,char * *)
[bcc32 Error] File2.cpp(34): E2316 'thread' is not a member of 'std'
Full parser context
File2.cpp(29): parsing: int main(int,char * *)
[bcc32 Error] File2.cpp(34): E2379 Statement missing ;
Full parser context
File2.cpp(29): parsing: int main(int,char * *)
[bcc32 Error] File2.cpp(36): E2316 'this_thread' is not a member of 'std'
Full parser context
File2.cpp(29): parsing: int main(int,char * *)
[bcc32 Error] File2.cpp(36): E2379 Statement missing ;
Full parser context
File2.cpp(29): parsing: int main(int,char * *)
[bcc32 Error] File2.cpp(39): E2451 Undefined symbol 'th'
Full parser context
File2.cpp(29): parsing: int main(int,char * *)
Failed
Elapsed time: 00:00:00.9
```
When adding the 64 Bit Windows target, the compiler used should be CLANG, like with your test by default, and the code successfully compiles, but doesn't link. The latter is most likely something wrong with the project setup and as said, we shouldn't care about BCC32 too much. Just use `thread_local` and deal with complains when they occur. BCC32 is VERY legacy and deprecated and Borland altogether not that widespread anymore.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org