Added mutex locks in register_target.cpp and created a multithreading… #2224

bpickrel · 2023-09-20T23:55:14Z

… test.

… test. Fails.

migraphx-bot · 2023-09-21T05:06:01Z

Test	Batch	Rate new d9ad08	Rate old fff12a	Diff	Compare
torchvision-resnet50	64	2,835.60	nan	nan%	❌
torchvision-resnet50_fp16	64	6,502.33	6,505.90	-0.05%	✅
torchvision-densenet121	32	2,096.61	2,080.03	0.80%	✅
torchvision-densenet121_fp16	32	3,664.03	3,662.79	0.03%	✅
torchvision-inceptionv3	32	1,597.74	1,597.57	0.01%	✅
torchvision-inceptionv3_fp16	32	2,556.32	2,562.85	-0.25%	✅
cadene-inceptionv4	16	722.35	722.13	0.03%	✅
cadene-resnext64x4	16	689.74	692.38	-0.38%	✅
slim-mobilenet	64	8,346.46	8,344.82	0.02%	✅
slim-nasnetalarge	64	230.67	230.69	-0.01%	✅
slim-resnet50v2	64	2,663.35	2,665.66	-0.09%	✅
bert-mrpc-onnx	8	812.09	813.21	-0.14%	✅
bert-mrpc-tf	1	388.13	387.28	0.22%	✅
pytorch-examples-wlang-gru	1	302.32	302.68	-0.12%	✅
pytorch-examples-wlang-lstm	1	314.52	318.19	-1.15%	✅
torchvision-resnet50_1	1	607.37	605.99	0.23%	✅
torchvision-inceptionv3_1	1	345.36	341.75	1.06%	✅
cadene-dpn92_1	1	401.36	405.34	-0.98%	✅
cadene-resnext101_1	1	324.08	328.21	-1.26%	✅
slim-vgg16_1	1	459.79	nan	nan%	❌
slim-mobilenet_1	1	2,128.16	2,112.08	0.76%	✅
slim-inceptionv4_1	1	214.31	213.80	0.24%	✅
onnx-taau-downsample	1	305.60	305.91	-0.10%	✅
dlrm-criteoterabyte	1	21.62	21.61	0.02%	✅
dlrm-criteoterabyte_fp16	1	40.65	40.66	-0.02%	✅
agentmodel	1	5,784.19	5,850.96	-1.14%	✅
unet_fp16	2	54.69	54.74	-0.08%	✅
resnet50v1_fp16	1	948.93	944.08	0.51%	✅
bert_base_cased_fp16	64	924.70	924.89	-0.02%	✅
bert_large_uncased_fp16	32	290.71	290.67	0.01%	✅
bert_large_fp16	1	171.80	171.71	0.05%	✅
distilgpt2_fp16	16	1,282.47	1,281.45	0.08%	✅

This build is not recommended to merge 🔴

migraphx-bot · 2023-09-21T05:06:03Z

✅ bert-mrpc-onnx: PASSED: MIGraphX meets tolerance

✅ bert-mrpc-tf: PASSED: MIGraphX meets tolerance

✅ pytorch-examples-wlang-gru: PASSED: MIGraphX meets tolerance

✅ pytorch-examples-wlang-lstm: PASSED: MIGraphX meets tolerance

✅ torchvision-resnet50_1: PASSED: MIGraphX meets tolerance

✅ torchvision-inceptionv3_1: PASSED: MIGraphX meets tolerance

✅ cadene-dpn92_1: PASSED: MIGraphX meets tolerance

✅ cadene-resnext101_1: PASSED: MIGraphX meets tolerance

✅ slim-vgg16_1: PASSED: MIGraphX meets tolerance

✅ slim-mobilenet_1: PASSED: MIGraphX meets tolerance

✅ slim-inceptionv4_1: PASSED: MIGraphX meets tolerance

✅ dlrm-criteoterabyte: PASSED: MIGraphX meets tolerance

✅ agentmodel: PASSED: MIGraphX meets tolerance

✅ unet: PASSED: MIGraphX meets tolerance

✅ resnet50v1: PASSED: MIGraphX meets tolerance

✅ bert_base_cased_fp16: PASSED: MIGraphX meets tolerance

✅ bert_large_uncased_fp16: PASSED: MIGraphX meets tolerance

✅ bert_large: PASSED: MIGraphX meets tolerance

🔴distilgpt2_fp16: FAILED: MIGraphX is not within tolerance - check verbose output

test/targets.cpp

codecov · 2023-09-30T01:08:37Z

Codecov Report

Merging #2224 (0bc7d59) into develop (4188c38) will decrease coverage by 0.04%.
Report is 22 commits behind head on develop.
The diff coverage is 85.71%.

❗ Current head 0bc7d59 differs from pull request most recent head 6befe93. Consider uploading reports for the commit 6befe93 to get more accurate results

@@             Coverage Diff             @@
##           develop    #2224      +/-   ##
===========================================
- Coverage    91.49%   91.46%   -0.04%     
===========================================
  Files          430      433       +3     
  Lines        16129    16184      +55     
===========================================
+ Hits         14758    14803      +45     
- Misses        1371     1381      +10

Files	Coverage Δ
src/compile_src.cpp	`85.00% <100.00%> (ø)`
src/cpp_generator.cpp	`70.83% <100.00%> (ø)`
src/include/migraphx/compile_src.hpp	`100.00% <100.00%> (ø)`
src/include/migraphx/op/allocate.hpp	`100.00% <ø> (ø)`
src/include/migraphx/op/convert.hpp	`100.00% <100.00%> (ø)`
src/include/migraphx/op/isnan.hpp	`100.00% <100.00%> (ø)`
src/onnx/parse_castlike.cpp	`100.00% <100.00%> (ø)`
src/onnx/parse_constant_of_shape.cpp	`92.85% <100.00%> (+1.19%)`	⬆️
src/onnx/parse_depthtospace.cpp	`90.90% <100.00%> (-0.27%)`	⬇️
src/onnx/parse_reshape.cpp	`100.00% <100.00%> (ø)`
... and 9 more

TedThemistokleous · 2023-10-02T15:13:55Z

src/register_target.cpp

+void register_target(const target& t)
+{
+    std::unique_lock<std::mutex> lock(target_mutex());
+    target_map()[t.name()] = t;


You should be unlocking here.

With std::unique_lock, the destructor automatically unlocks it when it goes out of scope. This is supposed not only to be a coding convenience, but gives protection when things are interrupted by an exception. From cplusplus.com:
"This class guarantees an unlocked status on destruction (even if not called explicitly). Therefore it is especially useful as an object with automatic duration, as it guarantees the mutex object is properly unlocked in case an exception is thrown."

TedThemistokleous · 2023-10-02T15:17:34Z

src/register_target.cpp

+    // search for match or return none
+    std::unique_lock<std::mutex> lock(target_mutex());
+    const auto it = target_map().find(name);
+


Add unlock before each return

Don't need to--see above

TedThemistokleous · 2023-10-02T15:20:59Z

src/register_target.cpp

 target make_target(const std::string& name)
 {
-    if(not contains(target_map(), name))
+    //   no lock required here


Are we not concerned with something trying to reregister a target in another thread? If between find_target, we end up doing an additional find()

Is the intent to not expose find_target() outside of register_target? If so, why not just add the locking around here.

The idea is that find_target does contain a lock, so make_target is already protected. The second thread would have to wait for the first thread to finish before trying to register it, then see that it's already there.

umangyadav · 2023-10-03T12:14:05Z

src/register_target.cpp

-        MIGRAPHX_THROW("Requested target '" + name + "' is not loaded or not supported");
-    }
-    return it->second;
+    // at this point we should always have a target


Why is it guaranteed to have a target at this point?
Let's say Target requested is not supported by MIGraphX e.g. some target named "TPU". TPU is not registered with MIGraphX and it will not be found or can not be loaded dynamically either.

Agreed, we should check if (t == nullopt) a second time and throw.

Why is it guaranteed to have a target at this point? Let's say Target requested is not supported by MIGraphX e.g. some target named "TPU". TPU is not registered with MIGraphX and it will not be found or can not be loaded dynamically either.

Because the dynamic_loader constructor will throw an exception. I added a line in the test for this.

Agreed, we should check if (t == nullopt) a second time and throw.

That's redundant since find_target contains that check.

umangyadav · 2023-10-03T12:22:50Z

test/targets.cpp

+    for(auto i = 0u; i < 1000; i++)
+    {
+        auto thread_body = []() {
+            auto ref_target = migraphx::make_target("ref");


target needs to be registered first before it can be made (or alternatively targets can by loaded dynamically). I think it works right now because it was already registered by other tests.

Yes this should try to do make_target("gpu") when gpu is enabled, and do make_target("cpu") when cpu is enabled, and then call unregister_target at the end.

There should also be threads calling get_targets as well.

target needs to be registered first before it can be made (or alternatively targets can by loaded dynamically). I think it works right now because it was already registered by other tests.

It is registered by auto_register.hpp during program initialization, before the test starts. I wouldn't have another way to make a target instance without calling make_target() first.

Recall that the goal of this test is not to make targets correctly; it's to check thread safety.

and then call unregister_target at the end.

That's not allowed. unregister_target is called automatically on exit, and it throws an exception if it's called twice with the same target name. As it is, it's still possible in principle for one holder of a target to unregister it while others are still holding it.

Is there any use at all in having unregister_target? It can only be used at program cleanup.

test/targets.cpp

bpickrel · 2023-10-05T20:47:07Z

test/targets.cpp

+
+            // remove all existing targets, if any
+            std::vector<std::string> target_list = migraphx::get_targets();
+            for(auto tt : target_list)


This new loop breaks the test now. Multiple threads registering and unregistering a target with the same name is not safe. Apparently it's not the target_map that's a problem but the underlying handles. Trying to figure out how to protect against this, or if it's necessary.

Followup: I disabled this extra test but when I tested the existing fixes with amdinfer, the Inference Server, the segmentation violation still occurs. I'm going to recommend we commit this PR as is but continue to search for the root cause of Issue #2208.

…. This code cannot pass the test.

bpickrel · 2023-10-10T21:18:55Z

test/targets.cpp

+    for(auto i = 0u; i < n_threads; i++)
+    {
+        auto thread_body = [&target_name]() {
+            // TODO:  remove all existing targets, if any.


Followup: I disabled this extra test but when I tested the existing fixes with amdinfer, the Inference Server, the segmentation violation still occurs. I'm going to recommend we commit this PR as is but continue to search for the root cause of Issue #2208.

bpickrel · 2023-10-10T21:23:11Z

I'm recommending that we approve this PR so we can get it into the next ROCm release, even though the underlying problem in the Inference Server still occurs. I can continue to look for the root cause. If we can agree that this code is correct as far as it goes then let's merge it.

codecov-commenter · 2023-12-29T14:17:02Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (fff12a2) 91.38% compared to head (d7657d7) 91.39%.
Report is 2 commits behind head on develop.

❗ Current head d7657d7 differs from pull request most recent head d9ad082. Consider uploading reports for the commit d9ad082 to get more accurate results

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #2224      +/-   ##
===========================================
+ Coverage    91.38%   91.39%   +0.01%     
===========================================
  Files          456      456              
  Lines        17241    17252      +11     
===========================================
+ Hits         15756    15768      +12     
+ Misses        1485     1484       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Added mutex locks in register_target.cpp and created a multithreading…

efb502b

… test. Fails.

bpickrel requested a review from pfultz2 September 20, 2023 23:55

bpickrel commented Sep 21, 2023

View reviewed changes

test/targets.cpp Show resolved Hide resolved

bpickrel commented Sep 21, 2023

View reviewed changes

test/targets.cpp Show resolved Hide resolved

TedThemistokleous assigned bpickrel Sep 21, 2023

TedThemistokleous added the bugfix Fixes a bug found in the code. label Sep 21, 2023

bpickrel mentioned this pull request Sep 21, 2023

Make register_target methods thread-safe #2208

Open

bpickrel and others added 4 commits September 26, 2023 11:20

reorganized mutexes and locks; passes test

52d75e0

remove extra includes

bab76d5

change test target type to "ref" since gpu is not available on CI server

d8f1ebb

Merge branch 'develop' into threads_register_target

f6b08c2

bpickrel marked this pull request as ready for review September 30, 2023 00:35

bpickrel requested a review from TedThemistokleous September 30, 2023 00:35

bpickrel linked an issue Oct 2, 2023 that may be closed by this pull request

Make register_target methods thread-safe #2208

Open

TedThemistokleous reviewed Oct 2, 2023

View reviewed changes

umangyadav reviewed Oct 3, 2023

View reviewed changes

pfultz2 reviewed Oct 3, 2023

View reviewed changes

test/targets.cpp Outdated Show resolved Hide resolved

pfultz2 reviewed Oct 3, 2023

View reviewed changes

test/targets.cpp Outdated Show resolved Hide resolved

bpickrel added 2 commits October 3, 2023 13:42

added more details to threading test

55b87a6

updated threading test, new actions fail

81d4516

bpickrel commented Oct 5, 2023

View reviewed changes

bpickrel marked this pull request as draft October 5, 2023 20:55

bpickrel added 2 commits October 5, 2023 14:05

formatting

c61c099

style

3d812cd

bpickrel added 2 commits October 10, 2023 14:06

disable test code to unregister targets concurrently with registering…

d0db0cd

…. This code cannot pass the test.

joinable threads in test

6befe93

bpickrel commented Oct 10, 2023

View reviewed changes

bpickrel marked this pull request as ready for review October 10, 2023 21:20

bpickrel requested review from pfultz2, umangyadav and TedThemistokleous October 10, 2023 21:20

kahmed10 approved these changes Dec 29, 2023

View reviewed changes

Merge branch 'develop' into threads_register_target

fd21950

joinable_thread fix

d9ad082

causten closed this Jan 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added mutex locks in register_target.cpp and created a multithreading… #2224

Added mutex locks in register_target.cpp and created a multithreading… #2224

bpickrel commented Sep 20, 2023 •

edited

Loading

migraphx-bot commented Sep 21, 2023 •

edited

Loading

migraphx-bot commented Sep 21, 2023 •

edited

Loading

codecov bot commented Sep 30, 2023 •

edited

Loading

TedThemistokleous Oct 2, 2023

bpickrel Oct 3, 2023

TedThemistokleous Oct 2, 2023

bpickrel Oct 3, 2023

TedThemistokleous Oct 2, 2023

bpickrel Oct 3, 2023 •

edited

Loading

umangyadav Oct 3, 2023 •

edited

Loading

pfultz2 Oct 3, 2023

bpickrel Oct 3, 2023

bpickrel Oct 3, 2023

umangyadav Oct 3, 2023 •

edited

Loading

pfultz2 Oct 3, 2023

bpickrel Oct 3, 2023

bpickrel Oct 3, 2023

bpickrel Oct 5, 2023

bpickrel Oct 10, 2023

bpickrel Oct 10, 2023

bpickrel commented Oct 10, 2023

codecov-commenter commented Dec 29, 2023 •

edited

Loading

Added mutex locks in register_target.cpp and created a multithreading… #2224

Added mutex locks in register_target.cpp and created a multithreading… #2224

Conversation

bpickrel commented Sep 20, 2023 • edited Loading

migraphx-bot commented Sep 21, 2023 • edited Loading

migraphx-bot commented Sep 21, 2023 • edited Loading

codecov bot commented Sep 30, 2023 • edited Loading

Codecov Report

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

bpickrel Oct 3, 2023 • edited Loading

Choose a reason for hiding this comment

umangyadav Oct 3, 2023 • edited Loading

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

umangyadav Oct 3, 2023 • edited Loading

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

Choose a reason for hiding this comment

bpickrel commented Oct 10, 2023

codecov-commenter commented Dec 29, 2023 • edited Loading

Codecov Report

bpickrel commented Sep 20, 2023 •

edited

Loading

migraphx-bot commented Sep 21, 2023 •

edited

Loading

migraphx-bot commented Sep 21, 2023 •

edited

Loading

codecov bot commented Sep 30, 2023 •

edited

Loading

bpickrel Oct 3, 2023 •

edited

Loading

umangyadav Oct 3, 2023 •

edited

Loading

umangyadav Oct 3, 2023 •

edited

Loading

codecov-commenter commented Dec 29, 2023 •

edited

Loading