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

RFC: serialize pointer- and padding-free objects in one write #14678

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

JeffBezanson
Copy link
Member

This is a hack to try to address #14106. Any objects that contain no references and no padding can be sent as a single binary blob, instead of iterating over the fields which entails expensive boxing.

Before:

    From worker 2:  (300000,)
  2.356665 seconds (20.25 M allocations: 1.398 GB, 3.00% gc time)
    From worker 3:  (300000,)
  2.218371 seconds (20.10 M allocations: 1.391 GB, 2.71% gc time)

after:

    From worker 2:  (300000,)
  0.649856 seconds (2.25 M allocations: 165.977 MB, 3.34% gc time)
    From worker 3:  (300000,)
  0.501824 seconds (2.10 M allocations: 159.813 MB, 1.42% gc time)

@@ -674,6 +678,11 @@ function deserialize(s::SerializationState, t::DataType)
end
if nf == 0
return ccall(:jl_new_struct, Any, (Any,Any...), t)
elseif t.pointerfree && ccall(:jl_datatype_haspadding, Cint, (Any,), t)==0
x = ccall(:jl_new_struct_uninit, Any, (Any,), t)
a_ = pointer_to_array(convert(Ptr{UInt8},pointer_from_objref(x)), sizeof(t))
Copy link
Member Author

Choose a reason for hiding this comment

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

This is regrettable but there are no read! methods that accept a pointer.

Copy link
Member

Choose a reason for hiding this comment

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

i've thought they should be unsafe_read! and implemented consistently instead of the current ad-hoc implementations

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good to me.

@JeffBezanson
Copy link
Member Author

cc @amitmurthy @abieler

@JeffBezanson
Copy link
Member Author

Oh bloody hell this ends up routing around the code that serializes Ptr as NULL. Gotta love those tests that check that something doesn't work. Does anybody else think this isn't worth a 4x slowdown and 10x increase in allocation? If we want both, I guess we could add a hasptr flag to DataType.

@StefanKarpinski
Copy link
Member

Le sigh. Should probably just add the hasptr flag.

@JeffBezanson
Copy link
Member Author

But then, why is Ptr the only bits type that's allowed to have a non-obvious serialize method? I guess for this to really work we need to limit it to a fixed set of known types.

@yuyichao
Copy link
Contributor

What about Cstring and Cwstring. These are also basically used as alias of the corresponding Ptr types although they are probably not currently used too much in composite types.

@JeffBezanson
Copy link
Member Author

Those types currently can't be serialized at all. And for the sake of not making people's parallel code slow as hell, let's hope they never can be.

else
writetag(s.io, UNDEFREF_TAG)
if t.pointerfree && ccall(:jl_datatype_haspadding, Cint, (Any,), t)==0
write(s.io, pointer_from_objref(x), sizeof(x))
Copy link
Member

Choose a reason for hiding this comment

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

the pointer returned here may be to a box that is not gc-rooted. in general, pointer_from_objref should never be called on something that is immutable and pointerfree (I think we should make it an error)

i'm looking into possible alternatives that would provide the same benefit, but doesn't require calling pointer_from_objref

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
parallelism Parallel or distributed computation performance Must go faster
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants