Skip to content
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

convert output of readbytes! to Int #141

Merged
merged 1 commit into from
Sep 8, 2022

Conversation

ranocha
Copy link
Contributor

@ranocha ranocha commented Sep 6, 2022

This improves type stability and fixes some invalidations when loading ArrayInterface.jl.
It follows the approach of #112 to cast the optional argument of readbytes! to an Int and fixes additional invalidations not covered by #138, #139.

I built Julia from the current release-1.8 branch with

julia$ git diff release-1.8
diff --git a/stdlib/Tar.version b/stdlib/Tar.version
index b7ee00e5a2..7835dfb18f 100644
--- a/stdlib/Tar.version
+++ b/stdlib/Tar.version
@@ -1,4 +1,4 @@
-TAR_BRANCH = master
-TAR_SHA1 = 0f8a73d5cd4b0c8f1f3c36799c96e9515e9dc595
-TAR_GIT_URL := https://github.com/JuliaIO/Tar.jl.git
-TAR_TAR_URL = https://api.github.com/repos/JuliaIO/Tar.jl/tarball/$1
+TAR_BRANCH = hr/fix_invalidations
+TAR_SHA1 = f63d9034a30171962de52c375572a310e1dffd2c
+TAR_GIT_URL := https://github.com/ranocha/Tar.jl.git
+TAR_TAR_URL = https://api.github.com/repos/ranocha/Tar.jl/tarball/$1

Then, I get no invalidations from loading ArrayInterface.jl anymore:

julia> using Pkg; Pkg.activate(temp=true); Pkg.add("ArrayInterface")

julia> using SnoopCompileCore; invalidations = @snoopr(using ArrayInterface); using SnoopCompile

julia> trees = invalidation_trees(invalidations);

julia> import Tar; ftrees = filtermod(Tar, trees)
SnoopCompile.MethodInvalidations[]

This improves type stability and fixes some invalidations when loading ArrayInterface.jl.
It follows the approach of JuliaIO#112 to cast the optional argument of readbytes! to an Int.
Comment on lines +299 to +300
max_read_len = Int(min(padded_size, length(buf)))::Int
read_len = Int(readbytes!(tar, buf, max_read_len))::Int
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you actually need the annotations of ::Int now? I would think that actually doing Int() would be enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought so, too, and just used Int(...). However, I still got the invalidations with that 😕

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@staticfloat Looks like we can't do anything better than this right now.

@codecov
Copy link

codecov bot commented Sep 6, 2022

Codecov Report

Merging #141 (f63d903) into master (c887de7) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #141   +/-   ##
=======================================
  Coverage   97.29%   97.29%           
=======================================
  Files           4        4           
  Lines         775      775           
=======================================
  Hits          754      754           
  Misses         21       21           
Impacted Files Coverage Δ
src/create.jl 100.00% <100.00%> (ø)
src/extract.jl 98.11% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@ranocha ranocha closed this Sep 7, 2022
@ranocha ranocha reopened this Sep 7, 2022
@staticfloat staticfloat merged commit 5914ef9 into JuliaIO:master Sep 8, 2022
@ranocha ranocha deleted the hr/fix_invalidations branch September 9, 2022 05:05
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