Decouple LockManager from engine #8717
Conversation
asfernandes
force-pushed
the
work/lockmanager-decouple
branch
2 times, most recently
from
14610d6 to
f63d26e
Compare
| { | ||
| } | ||
|
|
||
| ISC_STATUS getCancelState() const override; |
There was a problem hiding this comment.
If the class is marked final, perhaps it would be better to mark the methods final as well?
There was a problem hiding this comment.
That can be inferred by the compiler.
|
Can templates be used here instead of callback parameter to resolve interface at compiler-time instead of run-time? |
First, show numbers of what is the performance penalty introduced. |
No. |
|
I don't know what's happening with the android build. Maybe a compiler issue with the host machine compiler. This first phase seems to work locally. |
Is it possible to produce stack backtrace at the compiler host and extract it from there for analysis ? |
Sorry, I wrongly thought it is an engine crashed. Actually, it is clang compiler... weird |
asfernandes
force-pushed
the
work/lockmanager-decouple
branch
from
f63d26e to
3e428e3
Compare
Trying NDK update. |
I think it's not clang (NDK), but GPRE running that is causing the problem. That's why make deletes temp/Release/burp/OdsDetection.cpp. |
src/jrd/tests/LockManagerTest.cpp
Outdated
|
|
||
| ULONG adjustWait(ULONG wait) const | ||
| { | ||
| return 0; |
There was a problem hiding this comment.
| return 0; | |
| return wait; |
I guess templates can be used here by converting this whole thing to header only, which is definitely not worth it, and performance increase here will be non-existent due to other heavy work being done in functions, so overhead of virtual calls would be >0.1%. |
src/jrd/tests/LockManagerTest.cpp
Outdated
| threads.emplace_back([&]() { | ||
| const UCHAR LOCK_KEY[] = {'1'}; | ||
| FbLocalStatus statusVector; | ||
| LOCK_OWNER_T ownerId = threadNum + 1; |
There was a problem hiding this comment.
Here many threads could see the same value of threadNum, in my case all threads sees value 8.
Using an separate atomic_int to generate ownerId and test passed.
There was a problem hiding this comment.
I think the idea was to take threadNum from the loop, and this single loop will increment threadNum, so no race should be here, but it was captured by reference, so yea, it lead to race condition. I guess capture threadNum by value will fix the issue, without atomics.
There was a problem hiding this comment.
Yes @TreeHunter9 . Thanks for catching this, @hvlad and sorry for the noise. It seems to work as well in Linux. I will improve this test and make others.
asfernandes
force-pushed
the
work/lockmanager-decouple
branch
from
3d18860 to
74ed413
Compare
|
The crash on Android build makes me to ask - why it is different from other platforms ? |
|
The crash initially didn't happened when I removed parallel make (-j4). I caught the problem in the core dump. |
No description provided.