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

Reduce unnecessary zero-init'd allocations #632

Merged
merged 10 commits into from
Apr 14, 2022
Merged

Reduce unnecessary zero-init'd allocations #632

merged 10 commits into from
Apr 14, 2022

Conversation

shadeMe
Copy link
Collaborator

@shadeMe shadeMe commented Apr 11, 2022

I've added a new keyword arg uninitialized to the allocator methods in the NumpyOps and Ops classes. Also replaced some zero-init'd allocations where the elements in the destination array were immediately replaced.

@shadeMe shadeMe requested a review from danieldk April 11, 2022 13:45
@shadeMe shadeMe added enhancement Feature requests and improvements feat / ops Backends and maths labels Apr 11, 2022
svlandeg
svlandeg reviewed Apr 12, 2022
View reviewed changes
Copy link
Member

@svlandeg svlandeg left a comment

Choose a reason for hiding this comment

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

Great idea!

Not sure about the naming - uninitialized is not the most straightforward to type, but maybe just empty is too cryptic?

@shadeMe
Copy link
Collaborator Author

shadeMe commented Apr 12, 2022

Great idea!

Not sure about the naming - uninitialized is not the most straightforward to type, but maybe just empty is too cryptic?

Yeah, I thought empty was wasn't explicit enough, especially given that the incorrect usage of this flag will almost certainly lead to memory corruption. I also considered flipping the logic and calling the param zero_init but wasn't convinced that it was better.

I'm open to suggestions/switching to zero_init if you find that it's a better alternative.

@shadeMe shadeMe requested a review from svlandeg April 13, 2022 08:05
svlandeg
svlandeg reviewed Apr 13, 2022
View reviewed changes
Copy link
Member

@svlandeg svlandeg left a comment

Choose a reason for hiding this comment

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

Looks good to me!

Could you also update the documentation in /website/docs ?

@danieldk
Copy link
Contributor

There are some allocations in CupyOps/_custom_kernels that could be updated in a similar way. E.g. the maxout kernel wrapper unnecessarily zero-initializes an array.

@shadeMe
Copy link
Collaborator Author

shadeMe commented Apr 13, 2022

CupyOps had a fair amount of zero-allocs that could be replaced. These changes pass the tests on the GPU.

@danieldk
Copy link
Contributor

@explosion-bot please test_gpu

@explosion-bot
Copy link
Collaborator

explosion-bot commented Apr 13, 2022 •
edited
Loading

🪁 Successfully triggered build on Buildkite

URL: https://buildkite.com/explosion-ai/thinc-gpu-test-suite/builds/36

svlandeg
svlandeg reviewed Apr 13, 2022
View reviewed changes
Copy link
Member

@svlandeg svlandeg left a comment

Choose a reason for hiding this comment

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

Marked a few cases where I was unsure, but an additional set of eyes would be good!

thinc/backends/_custom_kernels.py Outdated Show resolved Hide resolved
thinc/backends/_custom_kernels.py Show resolved Hide resolved
thinc/backends/_custom_kernels.py Outdated Show resolved Hide resolved
@shadeMe shadeMe requested a review from danieldk April 14, 2022 11:02
@danieldk
Copy link
Contributor

@explosion-bot please test_gpu

@explosion-bot
Copy link
Collaborator

explosion-bot commented Apr 14, 2022 •
edited
Loading

🪁 Successfully triggered build on Buildkite

URL: https://buildkite.com/explosion-ai/thinc-gpu-test-suite/builds/37

@danieldk danieldk requested a review from svlandeg April 14, 2022 11:43
danieldk
danieldk approved these changes Apr 14, 2022
View reviewed changes
Copy link
Contributor

@danieldk danieldk left a comment

Choose a reason for hiding this comment

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

Looks great to me! But I'll defer to @svlandeg for a second opinion :).

@shadeMe shadeMe requested a review from danieldk April 14, 2022 11:50
@shadeMe
Copy link
Collaborator Author

shadeMe commented Apr 14, 2022

Whoops! Clicked on the wrong "Re-request review" button. Only mean to do so for @svlandeg.

@svlandeg svlandeg merged commit 5f615eb into explosion:develop Apr 14, 2022
@svlandeg
Copy link
Member

Merged!

svlandeg pushed a commit that referenced this pull request Apr 21, 2022
@shadeMe
Revert uninit'd allocs in Numpy.reduce_max introduced in #632. (#636)
fb92135 
* `NumpyOps`: Revert uninit'd alloc in `reduce_max`

* Test `which` in `test_reduce_max`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@svlandeg svlandeg Awaiting requested review from svlandeg

@danieldk danieldk Awaiting requested review from danieldk

Assignees
No one assigned
Labels
enhancement Feature requests and improvements feat / ops Backends and maths
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@shadeMe @danieldk @explosion-bot @svlandeg