Add pool sharing with copy on write #56

bkamins · 2021-02-20T08:55:59Z

Fixes #54

Could you please have a look at the core of the design proposal. If we are OK with it I will reimplement the rest, add tests, and update documentation.

src/PooledArrays.jl

bkamins · 2021-02-20T17:31:22Z

Can you have a quick look if now it looks as a right direction? (I will then polish the PR)
I ensures that both getindex and copyto! will be fast in generic code we have in joining algorithm in DataFrames.jl

codecov · 2021-02-20T18:26:18Z

Codecov Report

Merging #56 (3eb65e7) into main (32f1c2a) will increase coverage by 5.72%.
The diff coverage is 90.72%.

@@            Coverage Diff             @@
##             main      #56      +/-   ##
==========================================
+ Coverage   79.41%   85.14%   +5.72%     
==========================================
  Files           1        1              
  Lines         204      249      +45     
==========================================
+ Hits          162      212      +50     
+ Misses         42       37       -5

Impacted Files	Coverage Δ
src/PooledArrays.jl	`85.14% <90.72%> (+5.72%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 32f1c2a...3eb65e7. Read the comment docs.

bkamins · 2021-02-20T18:57:02Z

Benchmarks are as follows:

This PR:

julia> x = PooledArray(1:10^7);

julia> @time x[1:10^6];
  0.005101 seconds (7 allocations: 7.630 MiB)

julia> @time x[:];
  0.045468 seconds (6 allocations: 76.294 MiB)

julia> y = similar(x);

julia> @time copy!(y, x);
  0.005369 seconds

julia> y = similar(x);

julia> @time copyto!(y, x);
  0.003873 seconds

julia> @time copyto!(y, x);
  0.003631 seconds

julia> y = similar(x);

julia> @time copyto!(y, 1, x, 1, 10^6);
  0.000406 seconds (1 allocation: 16 bytes)

julia> @time copyto!(y, 1, x, 1, 10^6);
  0.000405 seconds (1 allocation: 16 bytes)

main branch:

julia> x = PooledArray(1:10^7);

julia> @time x[1:10^6];
  0.260175 seconds (14 allocations: 288.109 MiB, 3.09% gc time)

julia> @time x[:];
  1.897181 seconds (100 allocations: 580.985 MiB)

julia> y = similar(x);

julia> @time copy!(y, x);
  1.941643 seconds (90 allocations: 542.838 MiB, 3.85% gc time)

julia> y = similar(x);

julia> @time copyto!(y, x);
  1.965071 seconds (90 allocations: 542.838 MiB, 1.09% gc time)

julia> @time copyto!(y, x);
  1.038685 seconds

julia> y = similar(x);

julia> @time copyto!(y, 1, x, 1, 10^6);
  0.095800 seconds (69 allocations: 58.837 MiB)

julia> @time copyto!(y, 1, x, 1, 10^6);
  0.065425 seconds (1 allocation: 16 bytes)

bkamins · 2021-02-20T18:57:57Z

I need to add tests, but other than that this should be good for a review.

bkamins · 2021-02-20T20:02:31Z

@nalimilan + @quinnj : do you think we should add a separate branch when we detect that Julia is run in a single thread (a quite common case I think) and then we can just ignore everything and never copy pool and invpool?

nalimilan · 2021-02-20T22:35:46Z

Yes, looks quite good!

@nalimilan + @quinnj : do you think we should add a separate branch when we detect that Julia is run in a single thread (a quite common case I think) and then we can just ignore everything and never copy pool and invpool?

Good question. That makes the code harder to test so it's risky. Maybe better wait and see whether this would really matter in practice.

src/PooledArrays.jl

Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>

bkamins · 2021-02-21T07:36:39Z

That makes the code harder to test so it's risky.

OK. Then I will leave it as is for now.

bkamins · 2021-02-21T07:48:02Z

I will add tests for this functionality later as I have to think how to make them accurate since it is not easy.

What version of PooledArrays.jl this PR should introduce?

src/PooledArrays.jl

bkamins · 2021-02-24T21:08:30Z

src/PooledArrays.jl

-Base.convert(::Type{PooledArray{S,R,N}}, pa::PooledArray{T,R,N}) where {S,T,R<:Integer,N} =
-    PooledArray(RefArray(copy(pa.refs)), convert(Dict{S,R}, pa.invpool))
+function Base.convert(::Type{PooledArray{S,R1,N}}, pa::PooledArray{T,R2,N}) where {S,T,R1<:Integer,R2<:Integer,N}
+    if S === T && R1 === R2


this is not covered as it should never be true (we have a separate method for this - should I change this to @assert)

I'd just remove the branch.

OK - removed

src/PooledArrays.jl

test/runtests.jl

…aring

Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>

…aring

src/PooledArrays.jl

nalimilan · 2021-02-28T18:41:54Z

src/PooledArrays.jl

-Base.@propagate_inbounds function Base.isassigned(pa::PooledArray, I::Int...)
-    !iszero(pa.refs[I...])
+if VERSION < v"1.1"
+    Base.@propagate_inbounds function Base.getindex(A::SubArray{T,D,P,I,true} ,


Is this really untested?

I think coverage is not run on Julia 1.0 and this is the reason. Without this definition tests were failing on Julia 1.0 though.

This line https://github.com/JuliaData/PooledArrays.jl/pull/56/files#r584517703 fails without this special method

Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>

bkamins · 2021-02-28T22:54:58Z

I will merge it tomorrow if there are no more comments and tag a release.

bkamins · 2021-03-01T08:20:24Z

test/runtests.jl

+    @test refcount(pat1)[] == 3
+
+    copy!(pat1, pav1)
+    @test pat1 == pav1


@nalimilan - this line fails without the special method for Julia 1.0:

julia> pa = PooledArray(1:4) 4-element PooledArray{Int64,UInt32,1,Array{UInt32,1}}: 1 2 3 4 julia> pav1 = @view pa[2:3] 2-element view(::PooledArray{Int64,UInt32,1,Array{UInt32,1}}, 2:3) with eltype Int64: 2 3 julia> pav1[1] ERROR: MethodError: getindex(::SubArray{Int64,1,PooledArray{Int64,UInt32,1,Array{UInt32,1}},Tuple{UnitRange{Int64}},true}, ::Int64) is ambiguous. Candidates: getindex(A::SubArray{#s22,N,#s20,I,L} where L where I where #s20<:PooledArray where #s22, I::Vararg{Int64,N}) where {T, N} in PooledArrays at /home/bkamins/.julia/dev/PooledArrays/src/PooledArrays.jl:476 getindex(V::SubArray{T,N,P,I,true} where I<:Tuple{AbstractUnitRange,Vararg{Any,N} where N} where P where N where T, i::Int64) in Base at subarray.jl:234 Possible fix, define getindex(::SubArray{T,1,P,I,true} where I<:Tuple{AbstractUnitRange,Vararg{Any,N} where N} where P<:PooledArray where T, ::Int64) Stacktrace: [1] top-level scope at none:0

bkamins · 2021-03-01T08:28:33Z

Thank you! Today I will test it in DataFrames.jl new joining code after tagging

cwiese · 2021-10-22T20:39:12Z

Upgrading PooledArrays for JuliaDB/Indexed Tables from v.0.5.2 cause 10x slow down in groupreduce. I suspect this thread safe copy. We are trying to update to utilized newest CSV.jl.

There is no way to switch it off?

bkamins · 2021-10-23T09:09:43Z

Can you please open an issue with a reproducible code showing the problem?
In general pool sharing with copy on write should never degrade performance (it should only improve performance in cases when no copying of pool is needed), so this should be fixed, but we need a working code.

cwiese · 2021-10-24T12:01:32Z

there is so much going in when you do a groupreduce in IndexTables - I will try to reduce this to a PooledArray operation

nalimilan · 2021-10-24T15:11:53Z

Also if you can confirm that the regression appeared between say 0.5.1 and 0.5.2 it would be useful to track down the problem. Do make sure that other packages use the same version.

bkamins · 2021-10-24T15:53:59Z

@cwiese - you can edit appropriate methods in PooledArrays.jl that do pool copying to print some debugging information to see when/if things are invoked.

cwiese · 2021-10-24T15:55:15Z

Okay there is something else happening here - I will open an issue if I can narrow this down to PooledArrays. Thanks!

bkamins added 2 commits February 20, 2021 09:42

add pool sharing to PooledArrays

96e5d3a

further code review

dd77169

bkamins requested a review from nalimilan February 20, 2021 08:56

bkamins linked an issue Feb 20, 2021 that may be closed by this pull request

PooledArray is potentially not thread safe #55

Closed

bkamins mentioned this pull request Feb 20, 2021

left, right and outerjoin rewrite JuliaData/DataFrames.jl#2622

Merged

nalimilan reviewed Feb 20, 2021

View reviewed changes

src/PooledArrays.jl Outdated Show resolved Hide resolved

src/PooledArrays.jl Outdated Show resolved Hide resolved

src/PooledArrays.jl Outdated Show resolved Hide resolved

src/PooledArrays.jl Show resolved Hide resolved

src/PooledArrays.jl Outdated Show resolved Hide resolved

bkamins added 5 commits February 20, 2021 18:20

use Atomic{Int} and implement copyto!

7932121

fix copy

8dbee32

fix typo

92850e5

fix leftover code

995b3cd

add missing )

4c407f3

fix small issues

00af820

bkamins added 3 commits February 20, 2021 19:29

add missing method

45a33a9

another missing method

da13879

fix various lurking problems (and bugs) in old code

665bf9a

bkamins marked this pull request as ready for review February 20, 2021 18:57

nalimilan reviewed Feb 20, 2021

View reviewed changes

Apply suggestions from code review

cb1308c

Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>

bkamins mentioned this pull request Feb 21, 2021

implement compactpool! and compactpool functions #58

Open

apply comments from the review

be31412

bkamins commented Feb 21, 2021

View reviewed changes

src/PooledArrays.jl Show resolved Hide resolved

improve getindex

b41e9a1

bkamins added 5 commits February 24, 2021 14:27

update getindex

35430e0

continue adding tests

d3fb0a3

fix current tests

b6e6c85

finalize tests

2af32ca

hopefully final fixes

c0333af

bkamins commented Feb 24, 2021

View reviewed changes

src/PooledArrays.jl Show resolved Hide resolved

bkamins commented Feb 24, 2021

View reviewed changes

add Julia 1.0.5 support

46d971c

bkamins commented Feb 25, 2021

View reviewed changes

src/PooledArrays.jl Outdated Show resolved Hide resolved

Update src/PooledArrays.jl

c55b8e9

nalimilan reviewed Feb 25, 2021

View reviewed changes

bkamins and others added 4 commits February 27, 2021 23:07

fixes after code review

03a3221

Merge remote-tracking branch 'origin/bk/pool_sharing' into bk/pool_sh…

0b8ba4d

…aring

Update src/PooledArrays.jl

3a92892

Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>

Merge remote-tracking branch 'origin/bk/pool_sharing' into bk/pool_sh…

49afb14

…aring

quinnj approved these changes Feb 28, 2021

View reviewed changes

nalimilan approved these changes Feb 28, 2021

View reviewed changes

src/PooledArrays.jl Show resolved Hide resolved

nalimilan reviewed Feb 28, 2021

View reviewed changes

Update src/PooledArrays.jl

3eb65e7

Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>

bkamins commented Mar 1, 2021

View reviewed changes

bkamins merged commit 040053c into main Mar 1, 2021

bkamins deleted the bk/pool_sharing branch March 1, 2021 08:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add pool sharing with copy on write #56

Add pool sharing with copy on write #56

bkamins commented Feb 20, 2021

bkamins commented Feb 20, 2021

codecov bot commented Feb 20, 2021 •

edited

Loading

bkamins commented Feb 20, 2021

bkamins commented Feb 20, 2021

bkamins commented Feb 20, 2021

nalimilan commented Feb 20, 2021

bkamins commented Feb 21, 2021

bkamins commented Feb 21, 2021

bkamins Feb 24, 2021

nalimilan Feb 25, 2021

bkamins Feb 25, 2021

nalimilan Feb 28, 2021

bkamins Feb 28, 2021

bkamins Feb 28, 2021 •

edited

Loading

bkamins commented Feb 28, 2021

bkamins Mar 1, 2021

bkamins commented Mar 1, 2021

cwiese commented Oct 22, 2021

bkamins commented Oct 23, 2021

cwiese commented Oct 24, 2021

nalimilan commented Oct 24, 2021

bkamins commented Oct 24, 2021

cwiese commented Oct 24, 2021 •

edited

Loading

Add pool sharing with copy on write #56

Add pool sharing with copy on write #56

Conversation

bkamins commented Feb 20, 2021

bkamins commented Feb 20, 2021

codecov bot commented Feb 20, 2021 • edited Loading

Codecov Report

bkamins commented Feb 20, 2021

bkamins commented Feb 20, 2021

bkamins commented Feb 20, 2021

nalimilan commented Feb 20, 2021

bkamins commented Feb 21, 2021

bkamins commented Feb 21, 2021

bkamins Feb 24, 2021

Choose a reason for hiding this comment

nalimilan Feb 25, 2021

Choose a reason for hiding this comment

bkamins Feb 25, 2021

Choose a reason for hiding this comment

nalimilan Feb 28, 2021

Choose a reason for hiding this comment

bkamins Feb 28, 2021

Choose a reason for hiding this comment

bkamins Feb 28, 2021 • edited Loading

Choose a reason for hiding this comment

bkamins commented Feb 28, 2021

bkamins Mar 1, 2021

Choose a reason for hiding this comment

bkamins commented Mar 1, 2021

cwiese commented Oct 22, 2021

bkamins commented Oct 23, 2021

cwiese commented Oct 24, 2021

nalimilan commented Oct 24, 2021

bkamins commented Oct 24, 2021

cwiese commented Oct 24, 2021 • edited Loading

codecov bot commented Feb 20, 2021 •

edited

Loading

bkamins Feb 28, 2021 •

edited

Loading

cwiese commented Oct 24, 2021 •

edited

Loading