Speed up by releasing gil and using openmp with zerocopy view #36

synodriver · 2022-08-12T14:43:03Z

This pr did:

absolute import
update cython language level to 3
zerocopy view with more typing to speedup
cython inline suport to speedup
cython gc-free class to reduce gc pressure (obviously it's not possible for those types to participate in cycles)
use nogil to allow run in threadpool and bypass gil
use openmp for parallel compute
add ci to build wheels
inject data files to wheels so there's no need to download it when import
reformat using isort and black

feat: add dict in setup time

… speedup

feat: skrink macos brew & flags

fumiama · 2022-08-12T15:17:31Z

This may fix #30 #33 #34 for removing necessity to build it natively.

r9y9

Thank you for PR. There are some interesting new features, but I think there is too much for a single PR. Also, it would be great if you included some more information: what problems you are going to solve, how much performance gain is obtained, etc.

setup.py

r9y9 · 2022-08-14T11:25:42Z

setup.py

+    extra_compile_args = ["-fopenmp"]
+    extra_link_args = ["-fopenmp"]
+elif system == "Darwin":
+    os.system("brew install libomp")


I don't think automatically running brew install in setup.py is a good idea. How about updating the installation guide instead?

Actually the parallel is optional. Without linking to openmp, cython's prange will fallback to a single thread loop.

brew has merely become the default package manager in MacOS, thus in 99% case, installing openmp means run the command brew install libomp. However, we can just remove this line and give a warning of falling back to single thread loop to user.

Giving a warning sounds good to me. It is better to avoid installing binary dependencies to /usr/local/bin or somewhere implicitly.

r9y9 · 2022-08-14T11:25:57Z

setup.py

+elif system == "Darwin":
+    os.system("brew install libomp")
+    extra_compile_args = ["-Xpreprocessor", "-fopenmp"]
+    extra_link_args = ["-L/usr/local/lib", "-lomp"]


Better to avoid hard-code

Maybe we can add an option for it and let user decide where to link against?

Actually the linker will search all path for this lib, so we can just remove "-L/usr/local/lib"

An environmental variable (with default) works okay for example.

pyopenjtalk/setup.py

Line 221 in d4c59cd

if os.getenv("PYOPENJTALK_BUILD_VERSION"):

r9y9 · 2022-08-14T11:27:44Z

setup.py

+
+# extract dic
+filename = "dic.tar.gz"
+print(f"Downloading: {_DICT_URL}")
+urlretrieve(_DICT_URL, filename)
+print("Download complete")
+
+print("Extracting tar file {}".format(filename))
+with tarfile.open(filename, mode="r|gz") as f:
+    f.extractall(path="./")
+os.remove(filename)
+print("Extract complete")
+shutil.copytree(f"./{_dict_folder_name}", f"./pyopenjtalk/{_dict_folder_name}")
+sys.stdout.flush()
+


Code dup with pyopenjtalk/__init__.py

This is to avoid taking a long time when importing, and also for special environments such as cloud functions.

Consider removing the lazy downloader in pyopenjtalk/__init__.py

Sounds good to me. I just wanted to avoid duplication.

fumiama · 2022-08-14T11:54:19Z

but I think there is too much for a single PR

The change seems huge but can be concluded with optimize cython and packaging.

what problems you are going to solve

They are in the first comment.

how much performance gain is obtained

@synodriver will give you a result.

synodriver · 2022-08-14T13:20:04Z

how much performance gain is obtained

Since there is no gil required for those functions, it depends on how many cores your cpu have. And for asyncio apps, just use loop.run_in_executor is okey.

synodriver · 2022-08-15T02:33:14Z

BTW, I think cython should be an optional requirement for this project, just including those cython-generated files is ok if a user don't have cython installed.

r9y9 · 2022-08-15T03:15:16Z

I used to include cython-generated files for cython-dependent projects, but it indeed causes problems in some environments due to a mismatch between Python/Cython/compiler versions. I wish cython-generated code was portable and worked fine on any platform, but it wasn't unfortunately. In addition, to be specific to this repo, users need to generate config.h depending on their platform (ref #21), so it is not easy to make cython dependency optional for any platform.

One possible solution would be to release both pre-built packages for specific platforms (cython dep: optional) and non-built packages for general purposes (cython dep: mandatory), respectively.

fumiama · 2022-08-15T03:21:25Z

One possible solution would be to release both pre-built packages for specific platforms (cython dep: optional) and non-built packages for general purposes (cython dep: mandatory), respectively.

This is surely what we would like to do. We can both reduce the installing complexity and remain its portability by that.

synodriver · 2022-08-15T03:22:34Z

Actually I have already build wheels for almost all platforms here, it took a long time to debug the CI.

feat: remove six and lazy init and tqdm

synodriver · 2022-08-15T06:14:13Z

Ok, let's add some checks before inject datafiles.

r9y9 · 2022-08-15T11:45:34Z

pyopenjtalk/__init__.py

-class _TqdmUpTo(tqdm):  # type: ignore
-    def update_to(self, b=1, bsize=1, tsize=None):
-        if tsize is not None:
-            self.total = tsize
-        return self.update(b * bsize - self.n)


Why is the tqdm-related code removed? If it doesn't cause any problem, could you revert it back?

It is still in steup.py and this removing is part of the

Consider removing the lazy downloader in pyopenjtalk/__init__.py

I said yesterday.

I meant why the following code:

with TqdmUpTo(...): urlretrieve(...)

is reduced to

urlretrieve(...)

I think the former was okay as is.

Well, I just noticed that this code was removed after my changes by @synodriver . In my opinion, since this tqdm is only appears in setup time, whether removing or remaining it is okey. So what is your purpose of removing this?😂

It's just a progress bar and not necessary. Besides, I noticed some wired issue about downloading when runing ci with tqdm. I have no idea what's happening but to remove it. Then the ci works fine.

That may be an acceptable reason of removing this. Or we can pass an env variable to disable it in CI and enable it by default when user want to compile it natively?

I have never had issues with tqdm on CI so far. Is the problem reproducible? If so, it's okay to remove it.

I have run it several times on my fork but it always fail to build with tqdm installed. It's just like... the stdout is hiden, making it difficult to debug. And without tqdm it works fine. BTW, why does the progress bar so important for this project?

Umm, OK. The progress bar was just useful for knowing the estimated time to finish the downloading process.

But with prebuild-wheels, there is no need for users to download. You can check the files I mentioned here, the size of them shows that they already contains data files in the wheel.

r9y9 · 2022-08-15T11:52:31Z

This is surely what we would like to do. We can both reduce the installing complexity and remain its portability by that.

Agreed. I'll upload wheels to pypi for the next release so that users can install pyopenjtalk more easily. Also thanks for debugging through CI. I understand it takes time.

synodriver and others added 19 commits August 11, 2022 23:06

use nogil, prange and typedview to speedup

a17266a

speed up and fix omp

8eed18c

feat: add dict in setup time

5af6b9d

add imports

2364104

fix coredump

94b8b10

add build ci

8bde56c

fix macox build

bcfa63a

fix macox build

d351b35

Merge pull request #2 from fumiama/dict

adec254

feat: add dict in setup time

Merge branch 'speedup' of https://github.com/fumiama/pyopenjtalk into…

1b728e6

… speedup

fix args

b3de875

add ignore

acbebe5

add link arg

a342129

r

13d1d4b

eee

1fc6f63

Update setup.py

1c493f5

add download flag

a9873b8

Merge pull request #4 from fumiama/speedup

b9b0e9d

feat: skrink macos brew & flags

reformat with isort and black

89843b3

Merge upstream

34b008d

synodriver force-pushed the speedup branch from 93c8e1b to 34b008d Compare August 12, 2022 15:55

r9y9 requested changes Aug 14, 2022

View reviewed changes

feat: remove six and lazy init and tqdm

70f37d2

Merge pull request #6 from fumiama/speedup

a928a98

feat: remove six and lazy init and tqdm

ignore exc when datafile exists

bc8699a

r9y9 reviewed Aug 15, 2022

View reviewed changes

r9y9 mentioned this pull request Sep 26, 2022

Suggest: Add a env variable to let user select url to download dict. #41

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Speed up by releasing gil and using openmp with zerocopy view #36

Speed up by releasing gil and using openmp with zerocopy view #36

synodriver commented Aug 12, 2022

fumiama commented Aug 12, 2022

r9y9 left a comment

r9y9 Aug 14, 2022

synodriver Aug 14, 2022

fumiama Aug 14, 2022

r9y9 Aug 14, 2022

r9y9 Aug 14, 2022

synodriver Aug 14, 2022

fumiama Aug 14, 2022

r9y9 Aug 14, 2022

r9y9 Aug 14, 2022

synodriver Aug 14, 2022

fumiama Aug 14, 2022

r9y9 Aug 14, 2022

fumiama commented Aug 14, 2022

synodriver commented Aug 14, 2022 •

edited

Loading

synodriver commented Aug 15, 2022

r9y9 commented Aug 15, 2022

fumiama commented Aug 15, 2022

synodriver commented Aug 15, 2022

synodriver commented Aug 15, 2022

r9y9 Aug 15, 2022

fumiama Aug 15, 2022

r9y9 Aug 15, 2022

fumiama Aug 15, 2022 •

edited

Loading

synodriver Aug 15, 2022

fumiama Aug 16, 2022 •

edited

Loading

r9y9 Aug 20, 2022

synodriver Aug 20, 2022

r9y9 Aug 22, 2022

synodriver Aug 23, 2022

r9y9 commented Aug 15, 2022

Speed up by releasing gil and using openmp with zerocopy view #36

Are you sure you want to change the base?

Speed up by releasing gil and using openmp with zerocopy view #36

Conversation

synodriver commented Aug 12, 2022

fumiama commented Aug 12, 2022

r9y9 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

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

Choose a reason for hiding this comment

fumiama commented Aug 14, 2022

synodriver commented Aug 14, 2022 • edited Loading

synodriver commented Aug 15, 2022

r9y9 commented Aug 15, 2022

fumiama commented Aug 15, 2022

synodriver commented Aug 15, 2022

synodriver commented Aug 15, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fumiama Aug 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fumiama Aug 16, 2022 • 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

r9y9 commented Aug 15, 2022

synodriver commented Aug 14, 2022 •

edited

Loading

fumiama Aug 15, 2022 •

edited

Loading

fumiama Aug 16, 2022 •

edited

Loading