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

Additional GC.@ preserve documentation #35139

Merged
merged 1 commit into from
Mar 18, 2020
Merged

Additional GC.@ preserve documentation #35139

merged 1 commit into from
Mar 18, 2020

Conversation

c42f
Copy link
Member

@c42f c42f commented Mar 17, 2020

Here's some additional documentation for GC.@preserve, taking into account the discussion on discourse at https://discourse.julialang.org/t/on-the-garbage-collection/35695/31

CC @yuyichao

base/gcutils.jl Outdated
`x` which the compiler cannot see. Some examples:
* Accessing memory of an object directly via a `Ptr`
* Passing a pointer to `x` to `ccall` when the pointer is computed outside the
normal `cconvert` argument conversion pipeline.
Copy link
Contributor

Choose a reason for hiding this comment

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

cconvert -> ccall. It's not what function you use to do the conversion that matters here, if you call cconvert and unsafe_convert it won't help you preserving the object. It's the fact that it is done by the automatic mechaism of ccall that matters.


Also if you are calling cconvert and unsafe_convert it is the result of cconvert that should be preserved though that's a fairly independent issue.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. I'm trying to somehow briefly say that ccall preserves the objects which are explicitly passed to it, but if they implicitly use some other object then those implicitly used objects must be @preserved.

But without rehashing the documentation for ccall too much.

I've reworked this by shortening this part and adding more detail in an example.

Copy link

Choose a reason for hiding this comment

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

I find the enhanced docstring well written and understandable.

Maybe a drastic counterexample could raise programmer's attention, like the following. If you think it is helpful and not too long for a docstring, feel free to use or modify my suggestion:

Do not try to "optimize away" GC.@preserve because you think x is valid when a pointer to memory of x is dereferenced. Example:

function needs_preserve(s::String, t::String)
    do_some_allocations() # might cause garbage collection
    x = s * t * "ensure length > 0"
    p = pointer(x) # x is explicitly referenced and alive here
    c = unsafe_load(p) # x is implicitly used, not transparent to compiler
    # correct code makes use of x explicit: c = GC.preserve x unsafe_load(p)
    show(x) # x is explicitly referenced and alive here
    c
end

The code sequence suggests that x is valid before and after unsafe_load(p). But: without protecting pointer use by GC.@preserve, nothing prevents the compiler from reordering that code to:

function needs_preserve_reordered_by_compiler(s::String, t::String)
    x = s * t * "ensure length > 0"
    p = pointer(x) # x is explicitly referenced and alive here
    show(x) # after this, x is no longer referenced and available to garbage collection
    do_some_allocations() # might cause garbage collection of x
    unsafe_load(p) # possibly invalid result or even program crash by segfault
end

Copy link
Contributor

Choose a reason for hiding this comment

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

Reordering really isn't the issue though. There are a lot of things that actually prevent the reordering.

Copy link
Contributor

Choose a reason for hiding this comment

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

In particular, it's basically impossible to reorder anything that needs @preserve across anything that can trigger GC as far as the user is concerned, and you can actually rely on that if you really want to and does so correctly (I'm not sure if there's a way to do that though). However, that does not make the first case valid.

Copy link

Choose a reason for hiding this comment

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

Ok - if there is a chance to prove my example is wrong, forget about it.

I love performance optimization, and the opportunity to avoid GC.@preserve is a temptation. However, I know about the (day-and-)nightmare of 5000 customers again and again complaining about production crashes and data loss, and the hectics to find any clue to what happened in GBytes of production logfiles. Safety first. I will follow the recommmendation to put any pointer dereferencing into a GC.@preserve block.

However, if you come up with a bulletproof rule when to safely skip GC.@preserve, let me know ;-)

Copy link
Member

Choose a reason for hiding this comment

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

It's safe to skip (and can emit better code) if you avoid using pointers entirely. It's the pointer usages that have the performance risks, the GC.@preserve has no cost of its own.

@c42f c42f force-pushed the cjf/gc-preserve-docs branch from 2c25a5b to 1379c92 Compare March 17, 2020 13:08
@yuyichao
Copy link
Contributor

LGTM

@c42f c42f merged commit 2a7801f into master Mar 18, 2020
@c42f c42f deleted the cjf/gc-preserve-docs branch March 18, 2020 00:06
@c42f
Copy link
Member Author

c42f commented Mar 18, 2020

Thanks for the review!

@KristofferC KristofferC added backport 1.4 docs This change adds or pertains to documentation labels Mar 23, 2020
KristofferC pushed a commit that referenced this pull request Mar 23, 2020
@KristofferC KristofferC mentioned this pull request Mar 23, 2020
27 tasks
oxinabox pushed a commit to oxinabox/julia that referenced this pull request Apr 8, 2020
ravibitsgoa pushed a commit to ravibitsgoa/julia that referenced this pull request Apr 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs This change adds or pertains to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants