Skip to content

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

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.

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

Merged
merged 6 commits into from
Aug 18, 2021

Conversation

umar456
Copy link
Member

@umar456 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 umar456 added this to the 3.8.1 milestone Aug 6, 2021
@umar456 umar456 force-pushed the ref_count branch 7 times, most recently from f872e20 to 6312353 Compare August 10, 2021 07:00
@umar456 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
Copy link
Member

@9prady9 9prady9 left a comment

Choose a reason for hiding this comment

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

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.

@9prady9
Copy link
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.

Copy link
Member

@9prady9 9prady9 left a comment

Choose a reason for hiding this comment

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

Couple of more minor improvements.

@umar456 umar456 force-pushed the ref_count branch 5 times, most recently from 5e4d034 to 319c411 Compare August 17, 2021 20:11
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.
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.
Copy link
Member

@9prady9 9prady9 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 but I have some queries about the node hashing, but we can discuss those over chat

@9prady9 9prady9 merged commit a57b291 into arrayfire:master Aug 18, 2021
jacobkahn added a commit to jacobkahn/flashlight that referenced this pull request Feb 28, 2023
…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
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants