HPCC-30156 Instrument CriticalSection #17706

richardkchapman · 2023-08-23T16:31:07Z

Type of change:

This change is a bug fix (non-breaking change which fixes an issue).
This change is a new feature (non-breaking change which adds functionality).
This change improves the code (refactor or other change that does not change the functionality)
This change fixes warnings (the fix does not alter the functionality or the generated code)
This change is a breaking change (fix or feature that will cause existing behavior to change).
This change alters the query API (existing queries will have to be recompiled)

Checklist:

Smoketest:

Send notifications about my Pull Request position in Smoketest queue.
Test my draft Pull Request.

Testing:

github-actions · 2023-08-23T16:41:15Z

https://track.hpccsystems.com/browse/HPCC-30156
Jira updated

ghalliday · 2023-08-24T09:43:25Z

testing/unittests/unittests.cpp

+    inline void enter()
+    {
+        cycle_t start = get_cycles_now();
+#ifdef CHECK_CS_CONTENTION


move this after the getCurrentThreadId() call

ghalliday · 2023-08-24T09:43:57Z

testing/unittests/unittests.cpp

+        holdStart = get_cycles_now();
+        waitCycles += holdStart-start;
+#endif
+        depth++;


move this before the get_cycles_now() - will be trivial
I'm not 100% sure this is correct code (is the compiler allowed to add 1 to the value of depth it got before the mutex_lock?)

ghalliday · 2023-08-24T09:44:56Z

testing/unittests/unittests.cpp

+    inline void leave()
+    {
+        bool isContended = false;
+        depth--;


This is a bit early if the contended test is based on it.

Better to decrement just before the unlock, and check (depth == 1) here.

ghalliday · 2023-08-24T10:20:20Z

testing/unittests/unittests.cpp

+#ifdef _ASSERT_LOCK_SUPPORT
+    ThreadId owner = 0;
+#endif
+    unsigned depth = 0;


Slightly better if this is atomic - then the compiler can't magically move the code (and use fastInc(), fastDec()). Still get very inconsistent results though.

ghalliday · 2023-08-24T10:22:00Z

testing/unittests/unittests.cpp

+
+    inline void enter()
+    {
+        cycle_t start = get_cycles_now();


Should be inside the #ifdef TIME_CRITSECS

ghalliday · 2023-08-24T10:47:51Z

@mckellyln this is curious - the number of reported contentions is much lower than the real value.

I think the reason is that the overhead of the lock calls/returns is larger than the work being done in the critical section.

This may still be useful, but it suggests that quick critical sections will have significantly under-reported contention.

mckellyln

I agree with Gavins comments.
We may need to add compiler fences to ensure statements are not moved around ?

mckellyln · 2023-08-31T12:50:13Z

Should we increase thread count ?
Or add a duty-cycle / spin to test3 ?

Signed-off-by: Richard Chapman <[email protected]>

mckellyln · 2024-05-07T20:41:34Z

system/jlib/jmutex.cpp

+    }
+}
+
+thread_local CriticalBlockInstrumentation *__cbinst = nullptr;


where does this get created ?

ghalliday

Splitting out jtiming looks like a good change.
There are several changes for testing that probably need removing
A few questions about class names etc.
Generally I think it looks good, and I think worth merging with a bit more clean up.

ghalliday · 2024-05-10T09:37:15Z

system/jlib/jmutex.hpp

+#define __glue(a,b) a ## b
+#define glue(a,b) __glue(a,b)
+
+extern thread_local CriticalBlockInstrumentation* __cbinst;


Thread local variables generally generate terrible code. It would be better to keep this hidden within jmutex.cpp and provide an accessor function.

I think this is used to pass an implicit parameter into the CriticalBlock object without having to change the prototype? If so it is worth adding a comment to explain.

That is correct. I can add a comment.

Not really sure how an acessor function helps though - makes the code even more obscure, and less efficient (though smaller) I would have thought.

ghalliday · 2024-05-10T09:39:18Z

system/jlib/jmutex.hpp

+extern thread_local CriticalBlockInstrumentation* __cbinst;
+
+typedef InstrumentedCriticalSection CriticalSection;
+typedef InstrumentedCriticalBlock ICriticalBlock;


I am not clear why you would use ICriticalBlock rather than the other classes. Would it make sense for InstrumentedCrtiticalSection to only be defined if the option was selected, and otherwise be typedefed to the uninistrumented.

ICriticalBlock suggests an interface, which this isn't..

The idea I think is that you can Typedef CriticalBlock == InstrumentedCriticalBlock if you want ALL blocks instrumented, or use InstrumentedCriticalBlock explicitly if you want to investigate the behaviour of individual blocks.

I can add some comments to help explain that.

The reason for ICriticalBlock is that when CriticalBlock is a macro, you can't use it without a parameter. There are some instances where that is necessary. Not sure what the best name for it is.

ghalliday · 2024-05-10T09:41:28Z

cmake_modules/options.cmake

@@ -68,7 +68,7 @@ option(SKIP_ECLWATCH "Skip building ECL Watch" OFF)
 option(USE_ADDRESS_SANITIZER "Use address sanitizer to spot leaks" OFF)
 option(INSTALL_VCPKG_CATALOG "Install vcpkg-catalog.txt" ON)
 option(PORTALURL "Set url to hpccsystems portal download page")
-option(PROFILING "Set to true if planning to profile so stacks are informative" OFF)
+option(PROFILING "Set to true if planning to profile so stacks are informative" ON)


We need a separate flag to control the new functionality. (I set this on in my release builds by default.)

I think I intended the option to be controlled via editing the definition of USE_INSTRUMENTED_CRITSECS in jmutex.hpp - it's something that will be set temporarily while investigating issues rather than left on.

Rename ICriticalBlock Clean up some unwanted changes Add some comments Signed-off-by: Richard Chapman <[email protected]>

ghalliday · 2024-10-17T09:00:52Z

@richardkchapman I just found this branch invaluable for diagnosing a completely confusing set of timings for the stress text example. Please can you rebase, squash and I will rereview and aim to merge.

ghalliday · 2024-10-17T11:32:41Z

dfuserver is currently crashing on startup with this branch enabled
I am also struggling to get the output from roxie - I have having to run as a stand along program currently

ghalliday · 2024-10-17T11:33:07Z

And dali locks up on closedown.

richardkchapman requested a review from ghalliday August 23, 2023 16:31

ghalliday reviewed Aug 24, 2023

View reviewed changes

ghalliday requested a review from mckellyln August 24, 2023 10:33

mckellyln reviewed Aug 31, 2023

View reviewed changes

richardkchapman force-pushed the contendedLock branch 3 times, most recently from 65a8331 to 7bea2af Compare September 6, 2023 15:18

richardkchapman force-pushed the contendedLock branch from 7bea2af to 6642430 Compare September 20, 2023 15:24

HPCC-30156 Instrument CriticalSection

23aeede

Signed-off-by: Richard Chapman <[email protected]>

richardkchapman force-pushed the contendedLock branch from e74cd91 to 23aeede Compare October 11, 2023 13:31

richardkchapman marked this pull request as ready for review May 1, 2024 09:48

richardkchapman requested review from ghalliday and mckellyln May 1, 2024 09:48

mckellyln reviewed May 7, 2024

View reviewed changes

ghalliday reviewed May 10, 2024

View reviewed changes

HPCC-30156 Instrument CriticalSection

44d8b8e

Rename ICriticalBlock Clean up some unwanted changes Add some comments Signed-off-by: Richard Chapman <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HPCC-30156 Instrument CriticalSection #17706

HPCC-30156 Instrument CriticalSection #17706

richardkchapman commented Aug 23, 2023

github-actions bot commented Aug 23, 2023

ghalliday Aug 24, 2023

ghalliday Aug 24, 2023

ghalliday Aug 24, 2023

ghalliday Aug 24, 2023

ghalliday Aug 24, 2023

ghalliday Aug 24, 2023

ghalliday commented Aug 24, 2023

mckellyln left a comment

mckellyln commented Aug 31, 2023 •

edited

Loading

mckellyln May 7, 2024

ghalliday left a comment

ghalliday May 10, 2024

ghalliday May 10, 2024

richardkchapman May 15, 2024

ghalliday May 10, 2024

richardkchapman May 15, 2024 •

edited

Loading

richardkchapman May 15, 2024 •

edited

Loading

ghalliday May 10, 2024

richardkchapman May 15, 2024

ghalliday commented Oct 17, 2024

ghalliday commented Oct 17, 2024

ghalliday commented Oct 17, 2024

HPCC-30156 Instrument CriticalSection #17706

Are you sure you want to change the base?

HPCC-30156 Instrument CriticalSection #17706

Conversation

richardkchapman commented Aug 23, 2023

Type of change:

Checklist:

Smoketest:

Testing:

github-actions bot commented Aug 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghalliday commented Aug 24, 2023

mckellyln left a comment

Choose a reason for hiding this comment

mckellyln commented Aug 31, 2023 • edited Loading

Choose a reason for hiding this comment

ghalliday left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardkchapman May 15, 2024 • edited Loading

Choose a reason for hiding this comment

richardkchapman May 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghalliday commented Oct 17, 2024

ghalliday commented Oct 17, 2024

ghalliday commented Oct 17, 2024

mckellyln commented Aug 31, 2023 •

edited

Loading

richardkchapman May 15, 2024 •

edited

Loading

richardkchapman May 15, 2024 •

edited

Loading