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.

Sign up for GitHub

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

Jump to bottom

Fix #23567 #23577

Merged
merged 3 commits into from
Sep 5, 2017
Merged

Fix #23567 #23577

merged 3 commits into from
Sep 5, 2017

Conversation

quinnj
Copy link
Member

@quinnj quinnj commented Sep 4, 2017

No description provided.

@quinnj
Fix #23567. In general, we need to be careful when we start branching…
a3de583 
… on isbits(eltype(::Array)) because of Union isbits optimizations because isbits Unions don't return isbits == true.
@quinnj quinnj mentioned this pull request Sep 4, 2017
Closed
yuyichao
yuyichao reviewed Sep 4, 2017
View reviewed changes
base/array.jl
ccall(:memmove, Ptr{Void}, (Ptr{Void}, Ptr{Void}, UInt),
convert(Ptr{UInt8}, pointer(dest)) + length(dest) * Base.bitsunionsize(T) + doffs - 1,
convert(Ptr{UInt8}, pointer(src)) + length(src) * Base.bitsunionsize(T) + soffs - 1,
n)
else
ccall(:jl_array_ptr_copy, Void, (Any, Ptr{Void}, Any, Ptr{Void}, Int),
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove the branch from the C function?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, thanks for reminding me; just pushed that.

@vtjnash
Copy link
Member

vtjnash commented Sep 4, 2017

This looks mostly ok, at least as an improvement over the current issues. Note however, that isbitsunion is a very, very slow and inefficient query – and also is effectively just as incorrect as branching on is bits. But to avoid splitting the discussion, comments and design should probably happen on #23567 instead of here.

@ararslan
Copy link
Member

ararslan commented Sep 4, 2017

@nanosoldier runbenchmarks(ALL, vs=":master")

@nanosoldier
Copy link
Collaborator

Something went wrong when running your job:

NanosoldierError: failed to run benchmarks against primary commit: failed process: Process(`make -j3`, ProcessExited(2)) [2]

Logs and partial data can be found here
cc @ararslan

@quinnj
Copy link
Member Author

quinnj commented Sep 5, 2017

I'm guessing the curl compile error is unrelated 😄 . But on a related node, does nanosoldier build julia from scratch for each benchmark run? That seems intense! (if not excessive!)

@quinnj quinnj merged commit ff59aad into master Sep 5, 2017
@quinnj quinnj deleted the jq/bitsunionarrays branch September 5, 2017 01:52
@ararslan
Copy link
Member

ararslan commented Sep 5, 2017

Yeah, Nanosoldier builds from scratch on every run, which is why the runs can take so long.

@c42f c42f mentioned this pull request Aug 30, 2019
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yuyichao yuyichao yuyichao left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@quinnj @vtjnash @ararslan @nanosoldier @yuyichao