Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improve performance of host to device memory copy #1367

Merged
merged 5 commits into from Jun 26, 2018
Merged

Improve performance of host to device memory copy #1367

merged 5 commits into from Jun 26, 2018

Conversation

okuta
Copy link
Member

@okuta okuta commented Jun 14, 2018

This PR removes CPU overhead.

@okuta okuta added cat:enhancement Improvements to existing features to-be-backported Pull-requests to be backported to stable branch labels Jun 14, 2018
@chainer-ci
Copy link
Member

Jenkins CI test failed with status FAILURE.
(For contributors, please wait until the reviewer confirms the details of the error.)

@okuta okuta force-pushed the improve-memcopy branch 3 times, most recently from 222b16d to 9ab0d5d Compare June 14, 2018 15:47
@chainer-ci
Copy link
Member

Jenkins CI test (for commit
222b16d) failed with status FAILURE.
(For contributors, please wait until the reviewer confirms the details of the error.)

@chainer-ci
Copy link
Member

Jenkins CI test (for commit
c27c3d4) failed with status FAILURE.
(For contributors, please wait until the reviewer confirms the details of the error.)

@okuta okuta added the cat:performance Performance in terms of speed or memory consumption label Jun 17, 2018
@okuta okuta added this to the v5.0.0b2 milestone Jun 18, 2018
kmaehashi
kmaehashi requested changes Jun 18, 2018
View reviewed changes
Copy link
Member

@kmaehashi kmaehashi left a comment

Choose a reason for hiding this comment

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

Could you check Jenkins test failures?

cupy/core/core.pyx Outdated
@@ -2124,20 +2125,20 @@ cpdef ndarray array(obj, dtype=None, bint copy=True, str order='K',
order = 'A'
a_cpu = numpy.array(obj, dtype=dtype, copy=False, order=order,
ndmin=ndmin)
order = 'C' if a_cpu.flags.c_contiguous else 'F'
order = None if a_cpu.flags.c_contiguous else 'F'
Copy link
Member

Choose a reason for hiding this comment

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

Is this change needed? It seems the behavior does not change between 'C' and None.

Copy link
Member Author

Choose a reason for hiding this comment

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

None means 'C'. And, checking None is faster than checking 'C'.

Copy link
Member

Choose a reason for hiding this comment

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

I see, thanks!

@kmaehashi kmaehashi added the st:awaiting-author Awaiting response from author label Jun 18, 2018
@kmaehashi
Copy link
Member

@okuta Could you check Jenkins test failures?

@chainer-ci
Copy link
Member

Jenkins CI test (for commit 1faceab) failed with status FAILURE.
(For contributors, please wait until the reviewer confirms the details of the error.)

@chainer-ci
Copy link
Member

Jenkins CI test (for commit 50e9150) failed with status FAILURE.
(For contributors, please wait until the reviewer confirms the details of the error.)

@okuta
Copy link
Member Author

okuta commented Jun 19, 2018

Cloud you merge #1385 first?

@kmaehashi kmaehashi added st:blocked-by-another-pr Blocked by another pull-request and removed st:awaiting-author Awaiting response from author st:blocked-by-another-pr Blocked by another pull-request labels Jun 19, 2018
@kmaehashi
Copy link
Member

Jenkins, test this please.

@chainer-ci
Copy link
Member

Jenkins CI test (for commit 50e9150) failed with status FAILURE.
(For contributors, please wait until the reviewer confirms the details of the error.)

@kmaehashi
Copy link
Member

Jenkins, test this please.

@chainer-ci
Copy link
Member

Jenkins CI test (for commit 50e9150) failed with status FAILURE.
(For contributors, please wait until the reviewer confirms the details of the error.)

@kmaehashi
Copy link
Member

@okuta Could you check #1393 to fix jenkins failure?

@chainer-ci
Copy link
Member

Jenkins CI test (for commit 0b9dec6) failed with status FAILURE.
(For contributors, please wait until the reviewer confirms the details of the error.)

@beam2d beam2d modified the milestones: v5.0.0b2, v5.0.0b3 Jun 21, 2018
@kmaehashi
Copy link
Member

Jenkins, test this please.

@chainer-ci
Copy link
Member

Jenkins CI test (for commit 0b9dec6) succeeded without errors!

kmaehashi
kmaehashi requested changes Jun 22, 2018
View reviewed changes
Copy link
Member

@kmaehashi kmaehashi left a comment

Choose a reason for hiding this comment

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

LGTM except commented.

cupy/core/core.pyx Outdated
@@ -2139,24 +2140,26 @@ cpdef ndarray array(obj, dtype=None, bint copy=True, str order='K',
a = a.view()
a.shape = (1,) * (ndmin - ndim) + a.shape
else:
if order == 'K':
order = 'A'
if order is not None and len(order) == 1 and order in 'KAka':
Copy link
Member

Choose a reason for hiding this comment

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

Order needs to be normalized to accept longer names (ANY, KEEP).

@kmaehashi
Copy link
Member

I benchmarked this PR with this code and confirmed performance improvement! 🎉

Current master:

====== ========== ======== ======== ========
--              in_order / out_order
------ -------------------------------------
 ndim    C / C     C / F    F / C    F / F
====== ========== ======== ======== ========
  1      1.53ms    1.47ms   1.46ms   1.40ms
  2      1.44ms    1.47ms   1.51ms   1.43ms
  5      1.49ms    3.78ms   2.34ms   2.91ms
  8     750.61ms   4.62s    1.36s    4.04s
====== ========== ======== ======== ========

This PR:

====== ========== ======== ======== ==========
--               in_order / out_order         
------ ---------------------------------------
 ndim    C / C     C / F    F / C     F / F   
====== ========== ======== ======== ==========
  1      1.31ms    1.30ms   1.37ms    1.29ms  
  2      1.41ms    1.47ms   1.34ms    1.40ms  
  5      1.47ms    2.38ms   2.35ms    1.55ms  
  8     594.14ms   1.08s    2.39s    635.28ms 
====== ========== ======== ======== ==========

@kmaehashi kmaehashi added the st:awaiting-author Awaiting response from author label Jun 25, 2018
@okuta
Copy link
Member Author

okuta commented Jun 25, 2018

Thank you for your performance tests.
I fixed.

@kmaehashi
Copy link
Member

Thanks! Jenkins, test this please.

@kmaehashi kmaehashi removed the st:awaiting-author Awaiting response from author label Jun 25, 2018
@chainer-ci
Copy link
Member

Jenkins CI test (for commit f65a4c5) succeeded without errors!

@kmaehashi kmaehashi merged commit c561fa4 into cupy:master Jun 26, 2018
@kmaehashi
Copy link
Member

LGTM!

kmaehashi added a commit to kmaehashi/cupy that referenced this pull request Jun 26, 2018
@kmaehashi
Merge pull request cupy#1367 from okuta/improve-memcopy
63d585b 
Improve host to device memory copy
@kmaehashi kmaehashi mentioned this pull request Jun 26, 2018
Merged
@okuta okuta deleted the improve-memcopy branch June 29, 2018 08:15
@kmaehashi kmaehashi changed the title Improve host to device memory copy Improve performance of host to device memory copy Jul 18, 2018
@chainer-ci
Copy link
Member

Jenkins CI test (for commit f65a4c5) failed with status FAILURE.
(For contributors, please wait until the reviewer confirms the details of the error.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kmaehashi kmaehashi kmaehashi approved these changes

Assignees

@kmaehashi kmaehashi

Labels
cat:enhancement Improvements to existing features cat:performance Performance in terms of speed or memory consumption to-be-backported Pull-requests to be backported to stable branch
Projects
None yet
Milestone
v5.0.0b3
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@okuta @chainer-ci @kmaehashi @beam2d