[GUI] Use Taichi kernels instead of NumPy operations to reduce GUI.set_image overhead #1132

archibate · 2020-06-04T01:01:41Z

Related issue = close #1124

archibate · 2020-06-04T07:09:41Z

Before:

x64 Profiler: 21.1 fps
[  9.23%] paint                                        min   1.605 ms   avg   2.164 ms    max   6.565 ms   total   0.433 s [    200x]
[ 90.77%] set_image                                    min  20.121 ms   avg  21.288 ms    max  23.373 ms   total   4.258 s [    200x]
CUDA Profiler: 23.9 fps
[  3.10%] paint                                        min   0.882 ms   avg   0.892 ms    max   0.947 ms   total   0.178 s [    200x]
[ 96.90%] set_image                                    min  26.985 ms   avg  27.858 ms    max  29.705 ms   total   5.572 s [    200x]

After:

x64 Profiler: 50.4 fps
[ 33.59%] paint                                        min   1.590 ms   avg   2.092 ms    max   3.523 ms   total   0.418 s [    200x]
[ 66.41%] set_image                                    min   3.711 ms   avg   4.137 ms    max   5.504 ms   total   0.827 s [    200x]

CUDA Profiler: 36.1 fps
[  6.06%] paint                                        min   0.874 ms   avg   0.886 ms    max   1.040 ms   total   0.177 s [    200x]
[ 93.94%] set_image                                    min  13.323 ms   avg  13.739 ms    max  14.824 ms   total   2.748 s [    200x]

k-ye

Thx. I have left a few questions, since I'm not very familiar with the GUI part. Also, could you briefly explain what the optimization is in the PR description?

examples/simple_uv.py

python/taichi/lang/matrix.py

k-ye · 2020-06-04T11:43:19Z

python/taichi/lang/matrix.py

@@ -497,11 +497,17 @@ def assign_renamed(x, y):
        from .meta import fill_matrix
        fill_matrix(self, val)

+    def shape_ext(self, as_vector=None):
+        if as_vector is None:


Why None instead of making it a bool?

I don't get it. Why not just

def shape_ext(self, as_vector=True): shape_ext = (self.n, ) if as_vector else (self.n, self.m)

How is this off the topic?

self.m is ignored even if self.m != 1?

I don't see why this is a problem when as_vector is made a bool? For example, I can ask the question about what happens if as_vector is not None, but self.m != 1? According to the current logic, it also returns (self.n, ).

How about, ~~when as_vector is passed in by users, then raise an exception if self.m != 1~~ return (self.n, ) only when as_vector and self.m == 1?

taichi/python/export_visual.cpp

python/taichi/misc/gui.py

archibate · 2020-06-04T12:19:04Z

Also, could you briefly explain what the optimization is in the PR description?

See [GUI], I decided to expose this message to end-users... we may consider seperate end-user message and dev-user message like:

[GUI] less copy overhead for simple shader kernels with high resolution [gui] use taichi kernels instead of numpy functions to reduce set_image overhead

k-ye

Use Taichi kernels instead of NumPy operations to reduce GUI.set_image overhead

Sorry, this is still not clear to me. Wasn't the old code also using Taichi kernel to copy out the data, e.g. to_numpy()? Is it because vector_to_image is faster than matrix_to_ext_arr? Or did we avoid some unnecessary copy somewhere?

k-ye · 2020-06-04T13:13:22Z

python/taichi/lang/matrix.py

@@ -497,11 +497,17 @@ def assign_renamed(x, y):
        from .meta import fill_matrix
        fill_matrix(self, val)

+    def shape_ext(self, as_vector=None):
+        if as_vector is None:


I don't get it. Why not just

def shape_ext(self, as_vector=True): shape_ext = (self.n, ) if as_vector else (self.n, self.m)

How is this off the topic?

archibate · 2020-06-04T15:26:42Z

Use Taichi kernels instead of NumPy operations to reduce GUI.set_image overhead

Sorry, this is still not clear to me. Wasn't the old code also using Taichi kernel to copy out the data, e.g. to_numpy()? Is it because vector_to_image is faster than matrix_to_ext_arr? Or did we avoid some unnecessary copy somewhere?

It's a kernel fusion.

Before:
ti.meta.matrix_to_ext_arr
np.astype
np.__div__
np.concatenate
np.clip

After:
ti.meta.vector_to_image

If you don't understand, just remember the result of these "mystery" works is: 21 fps -> 50 fps.

archibate · 2020-06-04T15:39:13Z

I don't get it. Why not just
def shape_ext(self, as_vector=True):
shape_ext = (self.n, ) if as_vector else (self.n, self.m)
How is this off the topic?

So, if I call mat.shape_ext(), self.m is ignored even if self.m != 1?

k-ye

It's a kernel fusion.

I see. That's kind of the things I was looking for. Can you put that in the PR description (it's empty) so that we know how this has been improved?

If you don't understand, just remember the result of these "mystery" works is: 21 fps -> 50 fps.

While I'm glad about the result, this is really not how code review works. If you list me as a reviewer, then I will need to care about how it works just as much as the outcome, so that I can give useful feedback.

k-ye · 2020-06-05T10:07:57Z

python/taichi/lang/matrix.py

@@ -497,11 +497,17 @@ def assign_renamed(x, y):
        from .meta import fill_matrix
        fill_matrix(self, val)

+    def shape_ext(self, as_vector=None):
+        if as_vector is None:


self.m is ignored even if self.m != 1?

I don't see why this is a problem when as_vector is made a bool? For example, I can ask the question about what happens if as_vector is not None, but self.m != 1? According to the current logic, it also returns (self.n, ).

How about, ~~when as_vector is passed in by users, then raise an exception if self.m != 1~~ return (self.n, ) only when as_vector and self.m == 1?

k-ye

I think the as_vector is a tiny detail. TBH given the current usage pattern, maybe we should just completely remove it and always return (self.n, ) when self.m == 1? Otherwise LGTM, thx!

zhai-xiao

Great work, thanks!

python/taichi/misc/gui.py

python/taichi/misc/image.py

yuanming-hu

LGTM in general! Please fix a few minor issues.

If you don't understand, just remember the result of these "mystery" works is: 21 fps -> 50 fps.

Although I really like the performance boost, this sentence sounds really aggressive and unfriendly. Please always be polite. As a core developer, it's important for you to lead by example.

python/taichi/lang/meta.py

python/taichi/misc/image.py

python/taichi/misc/gui.py

python/taichi/lang/meta.py

python/taichi/misc/gui.py

Co-authored-by: Yuanming Hu <yuanming-hu@users.noreply.github.com>

python/taichi/misc/gui.py

yuanming-hu

Thanks! LGTM.

The fact that we don't have unit tests for these APIs may lead to bad code quality/stability control here. We should consider adding tests for these functionalities.

archibate · 2020-06-06T15:03:28Z

Yes, you're right, it was till now I realized that ti.imwrite have bug with RGBA images, maybe we should add test_gui, but it's hard to test, isn't it? e.g. How can we simulate key press in test? Worth consideration in a sep PR/issue. I'll merge this for now before v0.6.8.

[skip ci] tmp work save

da9a553

archibate added this to the v0.6.8 milestone Jun 4, 2020

archibate added GAMES201 GAMES 201 students' wishlist rendering labels Jun 4, 2020

archibate added 2 commits June 4, 2020 12:55

[skip ci] Merge branch 'master' into guicp

5c27446

reduce some redulent numpy copy for f32 image

7e57cfb

archibate marked this pull request as ready for review June 4, 2020 06:52

archibate requested review from k-ye, taichi-gardener and yuanming-hu and removed request for k-ye June 4, 2020 06:52

[skip ci] no hoc

3aaece3

archibate changed the title ~~[GUI] less copy overhead, fix low FPS for small shaders with high resolution~~ [GUI] less copy overhead for simple shader kernels with high resolution Jun 4, 2020

archibate force-pushed the guicp branch from 8f5dee1 to 3aaece3 Compare June 4, 2020 07:07

[skip ci] enforce code format

9a355b8

k-ye reviewed Jun 4, 2020

View reviewed changes

archibate self-assigned this Jun 4, 2020

archibate changed the title ~~[GUI] less copy overhead for simple shader kernels with high resolution~~ [GUI] Use Taichi kernels instead of NumPy operations to reduce GUI.set_image overhead Jun 4, 2020

k-ye reviewed Jun 4, 2020

View reviewed changes

merge

5342585

Merge branch 'guicp' of github.com:archibate/taichi into guicp

c08de80

archibate force-pushed the guicp branch from f7fcde6 to c08de80 Compare June 4, 2020 15:53

archibate added 3 commits June 5, 2020 10:32

fix image test

1780b6a

[skip ci] Merge branch 'master' into guicp

87bbc61

[skip ci] remove misc/simple_uv.py

721b1f1

archibate added 2 commits June 5, 2020 12:58

[skip ci] remove get_img_ptr

c0953bd

[skip ci] comment

36b11e5

archibate requested a review from k-ye June 5, 2020 04:59

fix image test again

39564c8

k-ye reviewed Jun 5, 2020

View reviewed changes

k-ye approved these changes Jun 5, 2020

View reviewed changes

[skip ci] no shape_ext

8b6e8fe

archibate requested a review from zhai-xiao June 5, 2020 14:31

zhai-xiao approved these changes Jun 6, 2020

View reviewed changes

python/taichi/misc/gui.py Outdated Show resolved Hide resolved

python/taichi/misc/image.py Outdated Show resolved Hide resolved

yuanming-hu reviewed Jun 6, 2020

View reviewed changes

archibate and others added 5 commits June 6, 2020 12:34

merge with master

ccd4d1e

[skip ci]

2f9b38d

[skip ci] Apply suggestions from code review

77b6aa0

Co-authored-by: Yuanming Hu <yuanming-hu@users.noreply.github.com>

[skip travis] self.cook_image

e7f59ce

[skip travis] fix torch test

686d808

archibate requested a review from yuanming-hu June 6, 2020 04:49

archibate and others added 2 commits June 6, 2020 12:50

[skip ci] better message

0d46229

[skip ci] enforce code format

3d34e11

zhai-xiao reviewed Jun 6, 2020

View reviewed changes

python/taichi/misc/gui.py Outdated Show resolved Hide resolved

archibate added 2 commits June 6, 2020 16:32

fix win32 not clamping

cd3e1b4

[skip ci] fix imread/write

79e4883

archibate added the skip ci label Jun 6, 2020

[skip ci] enforce code format

5d9a5ae

archibate added the waiting for reply label Jun 6, 2020

yuanming-hu approved these changes Jun 6, 2020

View reviewed changes

archibate merged commit d7663b7 into taichi-dev:master Jun 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GUI] Use Taichi kernels instead of NumPy operations to reduce GUI.set_image overhead #1132

[GUI] Use Taichi kernels instead of NumPy operations to reduce GUI.set_image overhead #1132

archibate commented Jun 4, 2020 •

edited

Loading

archibate commented Jun 4, 2020

k-ye left a comment

k-ye Jun 4, 2020

This comment was marked as outdated.

k-ye Jun 4, 2020

k-ye Jun 5, 2020 •

edited

Loading

archibate commented Jun 4, 2020

k-ye left a comment

k-ye Jun 4, 2020

archibate commented Jun 4, 2020 •

edited

Loading

archibate commented Jun 4, 2020

k-ye left a comment

k-ye Jun 5, 2020 •

edited

Loading

k-ye left a comment

zhai-xiao left a comment

yuanming-hu left a comment •

edited

Loading

yuanming-hu left a comment

archibate commented Jun 6, 2020

[GUI] Use Taichi kernels instead of NumPy operations to reduce GUI.set_image overhead #1132

[GUI] Use Taichi kernels instead of NumPy operations to reduce GUI.set_image overhead #1132

Conversation

archibate commented Jun 4, 2020 • edited Loading

archibate commented Jun 4, 2020

k-ye left a comment

Choose a reason for hiding this comment

k-ye Jun 4, 2020

Choose a reason for hiding this comment

This comment was marked as outdated.

k-ye Jun 4, 2020

Choose a reason for hiding this comment

k-ye Jun 5, 2020 • edited Loading

Choose a reason for hiding this comment

archibate commented Jun 4, 2020

k-ye left a comment

Choose a reason for hiding this comment

k-ye Jun 4, 2020

Choose a reason for hiding this comment

archibate commented Jun 4, 2020 • edited Loading

archibate commented Jun 4, 2020

k-ye left a comment

Choose a reason for hiding this comment

k-ye Jun 5, 2020 • edited Loading

Choose a reason for hiding this comment

k-ye left a comment

Choose a reason for hiding this comment

zhai-xiao left a comment

Choose a reason for hiding this comment

yuanming-hu left a comment • edited Loading

Choose a reason for hiding this comment

yuanming-hu left a comment

Choose a reason for hiding this comment

archibate commented Jun 6, 2020

archibate commented Jun 4, 2020 •

edited

Loading

k-ye Jun 5, 2020 •

edited

Loading

archibate commented Jun 4, 2020 •

edited

Loading

k-ye Jun 5, 2020 •

edited

Loading

yuanming-hu left a comment •

edited

Loading