Skip to content

Commit

Permalink
Backport deque memory fix (#898)
Browse files Browse the repository at this point in the history
* Use `Base._unsetindex!` in `pop!` and `popfirst!` (#897)

* Bump codecov/codecov-action from 3 to 4

Bumps [codecov/codecov-action](https://github.com/codecov/codecov-action) from 3 to 4.
- [Release notes](https://github.com/codecov/codecov-action/releases)
- [Changelog](https://github.com/codecov/codecov-action/blob/main/CHANGELOG.md)
- [Commits](codecov/codecov-action@v3...v4)

---
updated-dependencies:
- dependency-name: codecov/codecov-action
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <support@github.com>

* Use `Base._unsetindex!` in `pop!` and `popfirst!` for Deque

So that popped elements are not rooted by the deque and can be
GCed when they drop out of caller scope.

* Test Deque for leaks

* Use `Base._unsetindex!` in `pop!` and `popfirst!` for CircularDeque

So that popped elements are not rooted by the deque and can be GCed
when they drop out of caller scope.

* Test CircularDeque for leaks

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Bump version

* Make compatible with ancient Julia versions

* fixup! Make compatible with ancient Julia versions

* fixup! fixup! Make compatible with ancient Julia versions

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: Kiran Pamnany <kpamnany@users.noreply.github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
  • Loading branch information
3 people authored Feb 20, 2024
1 parent 685f7bc commit b5ba47d
Show file tree
Hide file tree
Showing 6 changed files with 59 additions and 2 deletions.
2 changes: 1 addition & 1 deletion Project.toml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
name = "DataStructures"
uuid = "864edb3b-99cc-5e75-8d2d-829cb0a9cfe8"
version = "0.18.16"
version = "0.18.17"

[deps]
Compat = "34da2185-b29b-5c13-b0c7-acf172513d20"
Expand Down
8 changes: 7 additions & 1 deletion src/DataStructures.jl
Original file line number Diff line number Diff line change
Expand Up @@ -112,4 +112,10 @@ module DataStructures
include("splay_tree.jl")

include("deprecations.jl")
end

@static if VERSION <= v"1.3"
_unsetindex!(a, i) = a

Check warning on line 117 in src/DataStructures.jl

View check run for this annotation

Codecov / codecov/patch

src/DataStructures.jl#L117

Added line #L117 was not covered by tests
else
_unsetindex! = Base._unsetindex!
end
end
2 changes: 2 additions & 0 deletions src/circ_deque.jl
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ end

@inline Base.@propagate_inbounds function Base.pop!(D::CircularDeque)
v = last(D)
_unsetindex!(D.buffer, D.last) # see issue/884
D.n -= 1
tmp = D.last - 1
D.last = ifelse(tmp < 1, D.capacity, tmp)
Expand Down Expand Up @@ -90,6 +91,7 @@ Remove the element at the front.
"""
@inline Base.@propagate_inbounds function Base.popfirst!(D::CircularDeque)
v = first(D)
_unsetindex!(D.buffer, D.first) # see issue/884
D.n -= 1
tmp = D.first + 1
D.first = ifelse(tmp > D.capacity, 1, tmp)
Expand Down
2 changes: 2 additions & 0 deletions src/deque.jl
Original file line number Diff line number Diff line change
Expand Up @@ -276,6 +276,7 @@ function Base.pop!(q::Deque{T}) where T
@assert rear.back >= rear.front

@inbounds x = rear.data[rear.back]
_unsetindex!(rear.data, rear.back) # see issue/884
rear.back -= 1
if rear.back < rear.front
if q.nblocks > 1
Expand All @@ -301,6 +302,7 @@ function Base.popfirst!(q::Deque{T}) where T
@assert head.back >= head.front

@inbounds x = head.data[head.front]
_unsetindex!(head.data, head.front) # see issue/884
head.front += 1
if head.back < head.front
if q.nblocks > 1
Expand Down
23 changes: 23 additions & 0 deletions test/test_circ_deque.jl
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,29 @@
for i in 1:5 push!(D, i) end
@test collect([i for i in D]) == collect(1:5)
end

VERSION >= v"1.3" && @testset "pop! and popfirst! do not leak" begin
D = CircularDeque{String}(5)

@testset "pop! doesn't leak" begin
push!(D,"foo")
push!(D,"bar")
ss2 = Base.summarysize(D)
pop!(D)
GC.gc(true)
ss1 = Base.summarysize(D)
@test ss1 < ss2
end
@testset "popfirst! doesn't leak" begin
push!(D,"baz")
push!(D,"bug")
ss2 = Base.summarysize(D)
popfirst!(D)
GC.gc(true)
ss1 = Base.summarysize(D)
@test ss1 < ss2
end
end
end

nothing
24 changes: 24 additions & 0 deletions test/test_deque.jl
Original file line number Diff line number Diff line change
Expand Up @@ -197,4 +197,28 @@
@test typeof(empty!(q)) === typeof(Deque{Int}())
@test isempty(q)
end

VERSION >= v"1.3" && @testset "pop! and popfirst! don't leak" begin
q = Deque{String}(5)
GC.gc(true)

@testset "pop! doesn't leak" begin
push!(q,"foo")
push!(q,"bar")
ss2 = Base.summarysize(q.head)
pop!(q)
GC.gc(true)
ss1 = Base.summarysize(q.head)
@test ss1 < ss2
end
@testset "popfirst! doesn't leak" begin
push!(q,"baz")
push!(q,"bug")
ss2 = Base.summarysize(q.head)
popfirst!(q)
GC.gc(true)
ss1 = Base.summarysize(q.head)
@test ss1 < ss2
end
end
end # @testset Deque

4 comments on commit b5ba47d

@nickrobinson251
Copy link
Member Author

Choose a reason for hiding this comment

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

@JuliaRegistrator
Copy link

Choose a reason for hiding this comment

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

Error while trying to register: Register Failed
@nickrobinson251, it looks like you are not a publicly listed member/owner in the parent organization (JuliaCollections).
If you are a member/owner, you will need to change your membership to public. See GitHub Help

@nickrobinson251
Copy link
Member Author

Choose a reason for hiding this comment

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

@JuliaRegistrator
Copy link

Choose a reason for hiding this comment

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

Registration pull request created: JuliaRegistries/General/101284

Tip: Release Notes

Did you know you can add release notes too? Just add markdown formatted text underneath the comment after the text
"Release notes:" and it will be added to the registry PR, and if TagBot is installed it will also be added to the
release that TagBot creates. i.e.

@JuliaRegistrator register

Release notes:

## Breaking changes

- blah

To add them here just re-invoke and the PR will be updated.

Tagging

After the above pull request is merged, it is recommended that a tag is created on this repository for the registered package version.

This will be done automatically if the Julia TagBot GitHub Action is installed, or can be done manually through the github interface, or via:

git tag -a v0.18.17 -m "<description of version>" b5ba47d658235e6a67d9c05a539dfbc503c6246f
git push origin v0.18.17

Please sign in to comment.