Fix prange utils #615

1e-to · 2020-02-17T15:08:45Z

No description provided.

pep8speaks · 2020-02-17T15:08:48Z

Hello @1e-to! 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 2020-02-18 15:11:39 UTC

PokhodenkoSA · 2020-02-17T15:11:30Z

sdc/utilities/prange_utils.py

        return chunks

-    return get_chunks_impl
+    return get_chunks_impl


Suggested change

return get_chunks_impl

return get_chunks_impl

densmirn

Let's hang @sdc_register_jitable on get_chunks and call the function inside of the overload implementation to avoid code duplication.

densmirn · 2020-02-17T15:14:38Z

Let's hang @sdc_register_jitable on get_chunks and call the function inside of the overload implementation to avoid code duplication.

@sdc_overload(get_chunks)
def get_chunks_overload(size, pool_size=0):
    def get_chunks_impl(size, pool_size=0):
        return get_chunks(size, pool_size=pool_size)

    return get_chunks_impl

densmirn · 2020-02-17T15:18:03Z

sdc/utilities/prange_utils.py

+        if i == pool_size - 1:
+            rest = size - size // pool_size
+            stop = min((i + 1) * chunk_size + rest, size)


Maybe to limit the loop for i in range(pool_size - 1) and handle the latest step outside of the loop.

densmirn · 2020-02-17T16:09:47Z

sdc/utilities/prange_utils.py

+# *****************************************************************************
+# Copyright (c) 2020, Intel Corporation All rights reserved.
+#
+# Redistribution and use in source and binary forms, with or without
+# modification, are permitted provided that the following conditions are met:
+#
+#     Redistributions of source code must retain the above copyright notice,
+#     this list of conditions and the following disclaimer.
+#
+#     Redistributions in binary form must reproduce the above copyright notice,
+#     this list of conditions and the following disclaimer in the documentation
+#     and/or other materials provided with the distribution.
+#
+# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
+# AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
+# THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
+# PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR
+# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,
+# EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
+# PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS;
+# OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY,
+# WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR
+# OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE,
+# EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+# *****************************************************************************
+
+
+import numba
+import sdc
+
+from typing import NamedTuple
+from sdc.utilities.utils import sdc_overload
+from sdc.utilities.utils import sdc_register_jitable
+
+
+class Chunk(NamedTuple):
+    start: int
+    stop: int
+
+
+def get_pool_size():
+    if sdc.config.config_use_parallel_overloads:
+        return numba.config.NUMBA_NUM_THREADS
+    else:
+        return 1
+
+
+@sdc_overload(get_pool_size)
+def get_pool_size_overload():
+    pool_size = get_pool_size()
+
+    def get_pool_size_impl():
+        return pool_size
+
+    return get_pool_size_impl


Duplicated code.

densmirn · 2020-02-17T16:11:03Z

sdc/utilities/prange_utils.py

 from sdc.utilities.utils import sdc_overload
+from sdc.utilities.utils import sdc_register_jitable


These lines could be combined to a single one.

AlexanderKalistratov · 2020-02-17T17:52:19Z

sdc/utilities/prange_utils.py

        pool_size = get_pool_size()

-    chunk_size = (size - 1) // pool_size + 1
+    chunk_size = size // pool_size


What's not valid.

What are you doing?

After closer look, it is valid, but a bad idea.

Let's assume, you have 56 threads and task size 55. In your implementation it wouldn't be paralleled at all. All 55 would be computed by the last chunk

It was my idea :) After some thinking I have the same conclusion. We should do the last thread to take lesser work.

I have implemented my vision in #618.

AlexanderKalistratov · 2020-02-17T17:52:41Z

sdc/utilities/prange_utils.py

        return numba.config.NUMBA_NUM_THREADS
-
-    return 1
+    else:


AlexanderKalistratov · 2020-02-17T19:12:48Z

In it's current (not this one) implementation number of chunks is equal to the pool size.

In case task size is less than pool size first chunks would contain valid information, while the last ones would have start outside of task size.
e.g. if task size is 5 and number of threads 16 first five chunks would have valid start and stop ([0,1], [1, 2], [2,3], [3,4], [4,5]). But the rest will have start and stop outside of task size ([5,5])

This is not a problem if we a doing something like this:

for i in prange(len(chunks):
    chunk = chunks[i]
    for j in range(chunk.start, chunk.stop):
        ...

In this case range(chunk.start, chunk.stop) is zero for extra chunks.
But if we need to do something like this:

for i in prange(len(chunks):
    chunk = chunks[i]
    first_item = items[chunk.start]
    for j in range(chunk.start, chunk.stop):
        ...

This could be a problem, since chunk.start could be out of range.
So, probably it is better for the case task size is less than pool size return number of chunks equal to task size. I.e. in case of task size 5 and pool size 16 return only first 5 chunks.

What is your thoughts?

PokhodenkoSA · 2020-02-17T19:16:16Z

What is your thoughts?

I agree.

pool_size = min(pool_size, size)

AlexanderKalistratov · 2020-02-18T08:00:36Z

sdc/utilities/prange_utils.py


-    chunk_size = (size - 1) // pool_size + 1
+    pool_size = min(pool_size, size)
+    chunk_size = size // pool_size


Please return the initial formula (size - 1)//pool_size + 1

This is not working properly.
With an array of 5 elements and 4 threads, chunks are divided into 0-2, 2-4, 4-6, 6-8

No, it divided into chunks [0,2), [2,4), [4,5), [5,5). And that's fine.
I think you should add code to discard last chunk in such case

In this module or in the function code that use this?

In this module. Something like this:

pool_size = min(pool_size, size) chunk_size = (size - 1)//pool_size + 1 chunks = [] for i in range(pool_size): start = i*chunk_size stop = min((i+1)*chunk_size, size) if start >= size: break chunks.append(Chunk(start, stop))

densmirn · 2020-02-18T13:04:12Z

sdc/utilities/prange_utils.py

-        start = min(i * chunk_size, size)
-        stop = min((i + 1) * chunk_size, size)
+        start = i*chunk_size
+        stop = min((i+1)*chunk_size, size)


You could move calculating of stop after if-block.

densmirn · 2020-02-18T13:05:58Z

sdc/utilities/prange_utils.py


-    chunk_size = (size - 1) // pool_size + 1
+    pool_size = min(pool_size, size)
+    chunk_size = (size - 1)//pool_size + 1


Why don't you round all the operations by white spaces?

Fix prange utils

0135066

1e-to requested review from PokhodenkoSA and densmirn February 17, 2020 15:08

PokhodenkoSA reviewed Feb 17, 2020

View reviewed changes

densmirn reviewed Feb 17, 2020

View reviewed changes

elena.totmenina added 3 commits February 17, 2020 19:04

wip

6f6815b

fix

007971b

fix

02a6f09

densmirn reviewed Feb 17, 2020

View reviewed changes

AlexanderKalistratov reviewed Feb 17, 2020

View reviewed changes

sdc/utilities/prange_utils.py Outdated

return numba.config.NUMBA_NUM_THREADS

return 1

else:

Copy link

Collaborator

AlexanderKalistratov Feb 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

small fixes

bb8880a

AlexanderKalistratov reviewed Feb 18, 2020

View reviewed changes

elena.totmenina added 2 commits February 18, 2020 12:44

fix chunk divide

767c329

pep

2f4fd50

densmirn reviewed Feb 18, 2020

View reviewed changes

codestyle fix

c777fa6

1e-to closed this Feb 19, 2020

1e-to deleted the fix branch February 19, 2020 15:48

		from sdc.utilities.utils import sdc_overload
		from sdc.utilities.utils import sdc_register_jitable

Fix prange utils #615

Fix prange utils #615

Uh oh!

Conversation

1e-to commented Feb 17, 2020

Uh oh!

pep8speaks commented Feb 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Comment last updated at 2020-02-18 15:11:39 UTC

Uh oh!

Choose a reason for hiding this comment

Uh oh!

densmirn left a comment

Choose a reason for hiding this comment

Uh oh!

densmirn commented Feb 17, 2020

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!

PokhodenkoSA Feb 17, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

AlexanderKalistratov commented Feb 17, 2020

Uh oh!

PokhodenkoSA commented Feb 17, 2020

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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

pep8speaks commented Feb 17, 2020 •

edited

Loading

PokhodenkoSA Feb 17, 2020 •

edited

Loading