← 返回首页
Fix reference count if array used in JIT operations. by umar456 · Pull Request #3167 · arrayfire/arrayfire · GitHub
Skip to content

Navigation Menu

Toggle navigation
Sign in
Appearance settings
Search or jump to...

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Include my email address so I can be contacted
Cancel Submit feedback

Saved searches

Use saved searches to filter your results more quickly

Cancel Create saved search
Appearance settings
Resetting focus

Fix reference count if array used in JIT operations.#3167

Merged
9prady9 merged 6 commits into
arrayfire:masterfrom
umar456:ref_count
Aug 18, 2021
Merged

Fix reference count if array used in JIT operations.#3167
9prady9 merged 6 commits into
arrayfire:masterfrom
umar456:ref_count

Conversation

Copy link
Copy Markdown
Member

umar456 commented Aug 6, 2021

Better manage the reference count of an Array object when it is used in a JIT operation

Description

Previously when an af::array was used in a jit operation and it was backed by a
buffer, a buffer node was created and the internal shared_ptr was stored in the
Array for future use and returned when getNode was called. This increased the
reference count of the internal buffer. This reference count never decreased
because of the internal reference to the shared_ptr.

This commit changes this behavior by storing a weak_ptr instead of a
shared_ptr inernally to the Array object. This change reduces the
reference count once the Array is evaluated in an expression. If
the weak_ptr can be converted into a

Changes to Users

Enjoy lower reference counts. This may increase performance under certain scenarios

Checklist

  • Rebased on latest master
  • Code compiles
  • Tests pass
  • [ ] Functions added to unified API
  • Functions documented

umar456 added this to the 3.8.1 milestone Aug 6, 2021
umar456 force-pushed the ref_count branch 7 times, most recently from f872e20 to 6312353 Compare August 10, 2021 07:00
umar456 changed the title Fix reference count if array used in JIT operations. breaks evalMultiple Fix reference count if array used in JIT operations. Aug 10, 2021
9prady9 requested changes Aug 12, 2021
View reviewed changes
Copy link
Copy Markdown
Member

9prady9 left a comment

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

The following commits can be merged

I have left comments in other respective files/commits.

There are some typos in Node Hash commit I think.

Comment thread src/backend/common/cast.hpp Outdated Show resolved Hide resolved
Comment thread src/backend/common/cast.hpp Outdated Show resolved Hide resolved
Comment thread src/api/c/binary.cpp Outdated Show resolved Hide resolved
Comment thread src/api/c/median.cpp Show resolved Hide resolved
Comment thread src/backend/opencl/jit/BufferNode.hpp Outdated Show resolved Hide resolved
Comment thread src/backend/cuda/jit/BufferNode.hpp Outdated Show resolved Hide resolved
Comment thread src/backend/cpu/jit/BufferNode.hpp Outdated Show resolved Hide resolved
Comment thread test/jit.cpp Outdated Show resolved Hide resolved
Comment thread src/backend/common/jit/Node.hpp Outdated Show resolved Hide resolved
Copy link
Copy Markdown
Member

9prady9 commented Aug 12, 2021

Also I forgot to mention this earlier. median_cpu and canny_cpu are failing consistently across all CPU jobs on Ubuntu 20.04 now.

9prady9 requested changes Aug 13, 2021
View reviewed changes
Copy link
Copy Markdown
Member

9prady9 left a comment

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

Couple of more minor improvements.

Comment thread src/backend/common/jit/Node.hpp Outdated Show resolved Hide resolved
Comment thread src/backend/cpu/jit/BufferNode.hpp Outdated Show resolved Hide resolved
Comment thread src/backend/common/jit/BufferNodeBase.hpp Show resolved Hide resolved
umar456 force-pushed the ref_count branch 5 times, most recently from 5e4d034 to 319c411 Compare August 17, 2021 20:11
umar456 added 5 commits August 17, 2021 16:12
Add ASSERT_REF to check for reference counts of af::arrays
53088f6
Move createBinaryNode to common
063cd01
Move cast and castArray to the common directory
aaf92fc
Use getArray instead of castArray if types are the same in arithOp
ce74c8e
Create a hash function for Node objects for NodeMap …
6c8f0ea
The Node_map_t unordered_map object uses the pointer of the nodes for the key. This worked because you could previously because the node buffer objects tracked the buffer object's shared pointer. This required holding an additional reference to the buffer object when an Array was used in a JIT operation. This did not leak memory because both the buffer and the node were deleted when the Array object was destroyed. This commit creates a new hash function for the node pointers which dereferences the Node pointers and if they are buffers, it checks the buffer's pointer and its offset to determine if its unique. This approach allows us to remove the call_once construct from the setData member function of the buffer node. You can now create node objects for each invocation getNode function.
Fix reference count if array used in JIT operations. …
9f38a34
Previously when an af::array was used in a jit operation and it was backed by a buffer, a buffer node was created and the internal shared_ptr was stored in the Array for future use and returned when getNode was called. This increased the reference count of the internal buffer. This reference count never decreased because of the internal reference to the shared_ptr. This commit changes this behavior by createing new buffer nodes for each call the getNode. We use the new hash function to ensure the equality of the buffer node when the jit code is generated. This avoids holding the call_once flag in the buffer object and simplifies the management of the buffer node objects. Additionally when a jit node goes out of scope the reference count decrements as expected.
9prady9 approved these changes Aug 18, 2021
View reviewed changes
Copy link
Copy Markdown
Member

9prady9 left a comment

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

Looks good but I have some queries about the node hashing, but we can discuss those over chat

9prady9 merged commit a57b291 into arrayfire:master Aug 18, 2021
jacobkahn mentioned this pull request Feb 27, 2023
Closed
jacobkahn added a commit to jacobkahn/flashlight that referenced this pull request Feb 28, 2023
Update refcount tests to be compatible with ArrayFire >= v3.7.3 (flas… …
4a7bff5
…hlight#1080) Summary: See title. These changes are due to arrayfire/arrayfire#3167, which updates and improves refcounting for ArrayFire arrays. Pull Request resolved: flashlight#1080 Test Plan: CI Reviewed By: richjames0 Differential Revision: D43633315 Pulled By: jacobkahn fbshipit-source-id: 8f838a42c4b724953900b8ccccde6dac23e137ec
facebook-github-bot pushed a commit to flashlight/flashlight that referenced this pull request Mar 1, 2023
Update refcount tests to be compatible with ArrayFire >= v3.7.3 (#1080) …
e3868e2
Summary: See title. These changes are due to arrayfire/arrayfire#3167, which updates and improves refcounting for ArrayFire arrays. Pull Request resolved: #1080 Test Plan: CI Reviewed By: richjames0 Differential Revision: D43633315 Pulled By: jacobkahn fbshipit-source-id: d8ed1592b1d57f3d357d40386a1c571110ad1950
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

9prady9 9prady9 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

3.8.1

Development

Successfully merging this pull request may close these issues.

2 participants

Footer

© 2026 GitHub, Inc.