Skip to content

add isbitstype optimized path for FixedSizeList getindex - #104

Merged
quinnj merged 3 commits into
apache:mainfrom
jrevels:jr/fixedsizelistindex
Jan 12, 2021
Merged

add isbitstype optimized path for FixedSizeList getindex#104
quinnj merged 3 commits into
apache:mainfrom
jrevels:jr/fixedsizelistindex

Conversation

@jrevels

Copy link
Copy Markdown
Contributor

ref #103 (comment)

#103 before this PR:

julia> @time copy(uuids_fixed);
0.142450 seconds (1.06 M allocations: 74.713 MiB, 2.43% gc time)

#103 after this PR:

julia> @time copy(y.recording_uuid);
0.016563 seconds (2 allocations: 3.248 MiB)

Order of magnitude improvement + allocations/memory now on par with pre-#103 behavior. Nice!

Still an order of magnitude of runtime perf on the table compared to the equivalent Primitive though. will play around and check whether branching changes/direct pointer manipulation might recover some of that.

@codecov

codecovBot commented Jan 9, 2021

Copy link
Copy Markdown

Codecov Report

Merging #104 (2d8b8d8) into main (31476c9) will decrease coverage by 0.42%.
The diff coverage is 96.15%.

Impacted file tree graph

@@ Coverage Diff @@## main #104 +/- ##
==========================================
- Coverage 84.48% 84.06% -0.43% 
==========================================
Files 22 23 +1 Lines 2611 2692 +81 ==========================================
+ Hits 2206 2263 +57 - Misses 405 429 +24 
Impacted FilesCoverage Δ
src/arraytypes/fixedsizelist.jl85.71% <93.33%> (-1.63%)⬇️
src/arrowtypes.jl90.00% <100.00%> (+0.25%)⬆️
src/arraytypes/list.jl88.59% <0.00%> (-1.59%)⬇️
src/arraytypes/map.jl95.38% <0.00%> (-1.50%)⬇️
src/arraytypes/bool.jl85.24% <0.00%> (-1.43%)⬇️
src/arraytypes/unions.jl82.06% <0.00%> (-0.57%)⬇️
src/metadata/Flatbuf.jl100.00% <0.00%> (ø)
src/utils.jl82.79% <0.00%> (+1.27%)⬆️
src/Arrow.jl52.38% <0.00%> (+5.01%)⬆️
... and 1 more

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 31476c9...2d8b8d8. Read the comment docs.

@jrevels
jrevelsforce-pushed the jr/fixedsizelistindex branch from b7bb22d to 239bdcaCompareJanuary 9, 2021 21:26
@jrevels

Copy link
Copy Markdown
ContributorAuthor

Using the same _unsafe_cast strategy from #103, we now get:

julia> @time copy(y.recording_uuid);
0.009492 seconds (2 allocations: 3.248 MiB) 

Still slower than the Primitive direct copy but now within the same order of magnitude, so I'm happy 😁

Once either #103 or this PR is merged, the remaining PR should be refactored to use the same underlying _unsafe_cast! utility defined here. (shouldn't matter which one is merged first)

@quinnjquinnj left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, let's just rebase the _unsafe_cast out since #103 is merged

@jrevels
jrevelsforce-pushed the jr/fixedsizelistindex branch from 239bdca to 2d8b8d8CompareJanuary 12, 2021 06:20
@jrevels

Copy link
Copy Markdown
ContributorAuthor

let's just rebase the _unsafe_cast out since #103 is merged

done!

@quinnj
quinnj merged commit 76ee57d into apache:mainJan 12, 2021
@jrevels
jrevels deleted the jr/fixedsizelistindex branch January 18, 2021 17:41
tanmaykm pushed a commit to tanmaykm/arrow-julia that referenced this pull request Apr 7, 2021
* add isbitstype optimized path for FixedSizeList getindex
* reuse _unsafe_cast strategy from apache#103
* rebase + DRY _unsafe_cast
Sign up for freeto 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

@jrevels@quinnj