Initial version of ConcurrentDict container via TBB hashmap by kozlov-alexey · Pull Request #972 · IntelPython/sdc

kozlov-alexey · 2021-04-12T17:59:10Z

Motivation: SDC relies on typed.Dict implementation in many core
pandas algorithms, and it doesn't support concurrent read/writes.
To fill this gap we add ConcurrentDict type which will be used if
threading layer is TBB.

Motivation: SDC relies on typed.Dict implementation in many core pandas algorithms, and it doesn't support concurrent read/writes. To fill this gap we add ConcurrentDict type which will be used if threading layer is TBB.

pep8speaks · 2021-04-12T17:59:15Z

Hello @kozlov-alexey! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-05-14 14:58:21 UTC

kozlov-alexey · 2021-04-13T19:16:47Z

/AzurePipelines Run

azure-pipelines · 2021-04-13T19:16:57Z

Azure Pipelines successfully started running 1 pipeline(s).

sdc/native/hashmap.hpp

Hardcode84 · 2021-04-15T13:57:46Z

sdc/native/hashmap.hpp

+
+{
+    auto ksize = this->key_info.size;
+    void* _key = malloc(ksize);


Why not new? Can we wrap these explicit mallocs/frees into unique_ptr?

It's problematic. I've tried a bit, but using unique_ptr<char[ksize]> is not possible since ksize is not compile-time const. Also the fact that TBB hashmap has to store the raw pointer (since HashCompare is actually numba functions that expect key/value to be pointers to Numba structs) kind of decrease the value of it (i.e. the ownership of allocated memory should go to hashmap itself and this function frees this memory only if insertion failed).

You can use unique_ptr with dynamically-sized arrays. Like this :

auto _key = std::unique_ptr<char[]>(new char[ksize])

Or like this:

auto _key = std::make_unique<char[]>(ksize);

Hardcode84 · 2021-04-20T12:19:35Z

sdc/native/hashmap.hpp

+        found = this->p_map->find(result, key);
+        if (found)
+        {
+            memcpy(res, result->second, this->val_info.size);


Clokwork will aggro on memcpy

I double checked and it appears it doesn't (see report below - all three issues are known and should be fixed by #967). Not sure if memcpy_s will enhance something, but I can still change if you think it's better.

Klocwork 20.1.0.97 ----- May 4, 2021 2:01:17 PM CDT: Running build preparation stage... May 4, 2021 2:01:17 PM CDT: Build preparation stage completed May 4, 2021 2:01:18 PM CDT: Running compilation stage... 1> CLASSIC: Compiling /localdisk/work/akozlov/miniconda3/hpat/sdc/_distributed.cpp 2> CLASSIC: Compiling /localdisk/work/akozlov/miniconda3/hpat/sdc/_hiframes.cpp 9> CLASSIC: Compiling /localdisk/work/akozlov/miniconda3/hpat/sdc/native/sort.cpp 8> CLASSIC: Compiling /localdisk/work/akozlov/miniconda3/hpat/sdc/transport/hpat_transport_single_process.cpp 7> CLASSIC: Compiling /localdisk/work/akozlov/miniconda3/hpat/sdc/io/_io.cpp 6> CLASSIC: Compiling /localdisk/work/akozlov/miniconda3/hpat/sdc/_datetime_ext.cpp 4> CLASSIC: Compiling /localdisk/work/akozlov/miniconda3/hpat/sdc/_str_ext.cpp 3> CLASSIC: Compiling /localdisk/work/akozlov/miniconda3/hpat/sdc/_set_ext.cpp Warning: /localdisk/work/akozlov/miniconda3/kwrun_6/.kwlp/buildspec.txt:24: 'compile' line for object file /localdisk/work/akozlov/miniconda3/hpat/build/temp.linux-x86_64-3.8/sdc/native/utils.o is already defined (ignored) 15> CLASSIC: Compiling /localdisk/work/akozlov/miniconda3/hpat/sdc/native/utils.cpp 14> CLASSIC: Compiling /localdisk/work/akozlov/miniconda3/hpat/sdc/native/module.cpp 11> CLASSIC: Compiling /localdisk/work/akozlov/miniconda3/hpat/sdc/native/stable_sort.cpp 5> CLASSIC: Compiling /localdisk/work/akozlov/miniconda3/hpat/sdc/native/conc_dict_module.cpp May 4, 2021 2:01:29 PM CDT: Compilation stage completed May 4, 2021 2:01:29 PM CDT: Running C/C++ defects detection stage... 1> Analyzing _distributed.cpp_0.o 2> Analyzing _hiframes.cpp_1.o 3> Analyzing _set_ext.cpp_2.o 4> Analyzing _str_ext.cpp_3.o 5> Analyzing _datetime_ext.cpp_4.o 6> Analyzing _io.cpp_5.o 7> Analyzing hpat_transport_single_process.cpp_6.o 8> Analyzing utils.cpp_10.o 9> Analyzing sort.cpp_7.o 10> Analyzing stable_sort.cpp_8.o 11> Analyzing module.cpp_9.o 12> Analyzing conc_dict_module.cpp_12.o May 4, 2021 2:02:21 PM CDT: C/C++ defects detection stage completed May 4, 2021 2:02:21 PM CDT: Running linking stage... May 4, 2021 2:02:22 PM CDT: Linking stage completed 1 (Local) /localdisk/work/akozlov/miniconda3/hpat/sdc/native/sort.cpp:151 VOIDRET (2:Error) Analyze void function returns value 2 (Local) /localdisk/work/akozlov/miniconda3/hpat/sdc/native/utils.cpp:89 NPD.CHECK.MUST (1:Critical) Analyze Pointer 'this->arena._M_t' checked for NULL at line 86 will be dereferenced at line 89. 3 (Local) /localdisk/work/akozlov/miniconda3/hpat/sdc/native/stable_sort.cpp:340 VOIDRET (2:Error) Analyze void function returns value Summary: 3 Local 3 Total Issue(s)

sdc/native/conc_dict_module.cpp

AlexanderKalistratov · 2021-04-26T22:00:01Z

sdc/extensions/sdc_hashmap_ext.py

+        if skip_check and skip_check(suffix):
+            continue
+        full_func_name = f'{fname}_{suffix}'
+        ll.add_symbol(full_func_name,


Is there a reason, we are using ll.add_symbol and not ctypes binding, so we can directly call functions from jit code?

Like it is done here:
https://github.com/IntelPython/sdc/blob/master/sdc/functions/sort.py#L55

No, there's no particular reason for this (except it was easier to write LLVM codegen that way - the function call inserted there directly refers to the function name being loaded, so there's 1:1 mapping and it's easier to navigate the code). I think we may refactor this and re-use common ctypes/bind approach later.

AlexanderKalistratov · 2021-04-26T22:09:16Z

sdc/extensions/sdc_hashmap_ext.py

+
+
+@intrinsic
+def hashmap_clear(typingctx, dict_type):


why do we need this to be as intrinsic and not as an overload?

@overload(hashmap_clear) def hashmap_clear_ovld(conc_dict): ty_key, ty_val = dict_type.key_type, dict_type.value_type key_type_postfix, value_type_postfix = _get_types_postfixes(ty_key, ty_val) hasmap_clear_func = hashmap_clear_map[(key_type_postfix, value_type_postfix)] def hashmap_clear_impl(conc_dict): return hashmap_clear_func(conc_dict._data_ptr) return hashmap_clear_impl

Am I missing something?

ctypes binding is required for this

Thanks! I didn't consider this way of doing things. However it may be more difficult to read for methods like set and pop which make key transformations during codegen or allocate the result (e.g.

sdc/sdc/extensions/sdc_hashmap_ext.py

Line 434 in 1177495

key_val, lir_key_type = transform_input_arg(context, builder, key_type, key_val)

), since the logic of generated llvm code will be split across functions (and also it would be much harder to debug this if you need to make some changed in the codegen). So if you don't mind I will keep it initial way for now.

AlexanderKalistratov · 2021-04-26T22:49:15Z

sdc/native/hashmap.hpp

+        {
+            typename map_type::const_accessor result;
+            found = this->p_map->find(result, key);
+            result.release();


Do we really need both - inner scope and call to release?

No of course, but it's (I guess) usual practice to release immediately after you don't need the lock (i.e. I copy-pasted it from TBB examples :)).

Hardcode84 · 2021-05-13T13:12:30Z

sdc/native/hashmap.hpp

+
+
+template<typename key_type, typename val_type>
+void hashmap_dump(void* p_hash_map)


Can we make hashmap_dump and corresponding #include <iostream> optional and wrap in some ifdefs? I would really prefer release libraries to be without iostream dependency?

Initial version of ConcurrentDict container via TBB hashmap

84f251d

Motivation: SDC relies on typed.Dict implementation in many core pandas algorithms, and it doesn't support concurrent read/writes. To fill this gap we add ConcurrentDict type which will be used if threading layer is TBB.

kozlov-alexey added 2 commits April 14, 2021 04:30

Fixing PEP and updating failing import

1010e6f

Fixing builds, warnings and complying to C++11 syntax

1177495

kozlov-alexey added this to the 0.39 milestone Apr 14, 2021

kozlov-alexey added the Ready for Review label Apr 14, 2021

kozlov-alexey requested review from AlexanderKalistratov and Hardcode84 April 14, 2021 19:04

Hardcode84 reviewed Apr 15, 2021

View reviewed changes

sdc/native/hashmap.hpp Outdated Show resolved Hide resolved

Hardcode84 reviewed Apr 15, 2021

View reviewed changes

Hardcode84 reviewed Apr 20, 2021

View reviewed changes

AlexanderKalistratov reviewed Apr 26, 2021

View reviewed changes

kozlov-alexey added Waiting on author and removed Ready for Review labels Apr 28, 2021

Fixing PEP and review comments #1

cd93560

kozlov-alexey added Ready for Review and removed Waiting on author labels May 5, 2021

kozlov-alexey requested review from AlexanderKalistratov and Hardcode84 May 6, 2021 12:04

kozlov-alexey added 2 commits May 11, 2021 19:51

Merge branch 'master' into feature/add_conc_dict

16b9ea8

Fixing remarks #2

d7ca42c

Hardcode84 reviewed May 13, 2021

View reviewed changes

Hardcode84 approved these changes May 13, 2021

View reviewed changes

kozlov-alexey added 2 commits May 14, 2021 17:57

Applying remarks #3

5de5d34

Merge branch 'master' into feature/add_conc_dict

78ff0fa

kozlov-alexey merged commit 1574682 into IntelPython:master May 14, 2021



		template<typename key_type, typename val_type>
		void hashmap_dump(void* p_hash_map)

Conversation

kozlov-alexey commented Apr 12, 2021

Uh oh!

pep8speaks commented Apr 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Comment last updated at 2021-05-14 14:58:21 UTC

Uh oh!

kozlov-alexey commented Apr 13, 2021

Uh oh!

azure-pipelines bot commented Apr 13, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hardcode84 May 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

pep8speaks commented Apr 12, 2021 •

edited

Loading

Hardcode84 May 13, 2021 •

edited

Loading