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

Move gc and gc_enable to their own module #25616

Merged
merged 2 commits into from
Jan 19, 2018
Merged

Move gc and gc_enable to their own module #25616

merged 2 commits into from
Jan 19, 2018

Conversation

ararslan
Copy link
Member

@ararslan ararslan commented Jan 17, 2018 •
edited
Loading

We document that these functions should not generally be used, and yet they're exported from Base. This PR moves the two functions into their own submodule, Base.GC. It seems a little silly to me that we say, "Here are these functions in your namespace, but you really shouldn't call them." Apparently they've been exported since Julia 0.1, so I imagine it's sort of a historical artifact at this point.

@ararslan ararslan added GC Garbage collector deprecation This change introduces or involves a deprecation triage This should be discussed on a triage call labels Jan 17, 2018
@KristofferC
Copy link
Member

KristofferC commented Jan 17, 2018 •
edited
Loading

Just unexport them? Seems silly to have a whole module just for this...

@JeffBezanson
Copy link
Member

JeffBezanson commented Jan 17, 2018 •
edited
Loading

I agree just unexporting them is fine. But if we want the module, I prefer the name GC. Then we could have GC.enable instead of gc_enable.

@ararslan
Copy link
Member Author

Okay, then would you prefer no module or a module named GC? I'm pretty indifferent.

@ararslan ararslan mentioned this pull request Jan 17, 2018
Closed
19 tasks
@StefanKarpinski
Copy link
Member

I think calling it GC and having GC.run() and GC.enable() would be preferable.

@JeffBezanson
Copy link
Member

I'd prefer GC.gc() or GC.collect().

vtjnash
vtjnash reviewed Jan 17, 2018
View reviewed changes
Copy link
Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

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

I agree that these don't need to be in a module, then can just exist in Base. I'm not sure they really need to be unexported either. I would just update the docs to sound more informative and less scary.

base/gcutils.jl Outdated
Perform garbage collection.

!!! warning
This should not generally be used.
Copy link
Member

Choose a reason for hiding this comment

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

I think this is too strong. We probably should have just said "Excessive use will likely lead to poor performance". Being "not used often" (appropriate wording) and "should not generally be used" (current wording) don't quite express the same level of concern.

base/gcutils.jl Outdated
`false` for disabled). Return previous GC state.

!!! warning
Disabling garbage collection should be used only with extreme caution, as it can cause
Copy link
Member

Choose a reason for hiding this comment

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

delete "extreme"

@ararslan
Copy link
Member Author

ararslan commented Jan 18, 2018 •
edited
Loading

Documentation updated. This now puts gc and gc_enable into Base.GC as GC.gc and GC.enable, and exports the GC module from Base. gc is exported from GC, so if you do using GC you get gc but not enable.

@ararslan ararslan force-pushed the aa/stop-exporting-gc-stuff branch 2 times, most recently from b59970e to aae96d0 Compare January 18, 2018 19:13
@StefanKarpinski StefanKarpinski added this to the 1.0 milestone Jan 18, 2018
@StefanKarpinski StefanKarpinski removed the triage This should be discussed on a triage call label Jan 18, 2018
@StefanKarpinski
Copy link
Member

Good to go when it passes.

@ararslan
Copy link
Member Author

I'm having a hard time coming up with an appropriate deprecation for gc. Is it possible to @deprecate a function in Base and export it, but also export it from a Base submodule? What I'm trying to do is deprecate gc to GC.gc but let the user call gc unqualified if they've done using Base.GC. Currently using Base.GC gives an error saying that both Base and GC export gc.

@JeffBezanson
Copy link
Member

I would suggest that people not write using Base.GC.

@ararslan
Copy link
Member Author

Fair enough.

@ararslan
Copy link
Member Author

Should we consider moving Base.@gc_preserve to Base.GC as GC.@preserve?

@ararslan ararslan changed the title Move gc and gc_enable to their own unexported module Move gc and gc_enable to their own module Jan 18, 2018
@ararslan
Deprecate gc and gc_enable in favor of GC.gc and GC.enable
46b5d75 
We document that these functions should not generally be used, and yet
they're exported from Base. This moves the two functions into their own
submodule, Base.GC, and deprecates the exported functions.
@ararslan
Deprecate Base.@gc_preserve in favor of GC.@preserve
bdc9094
@JeffBezanson JeffBezanson merged commit 544e2ae into master Jan 19, 2018
@JeffBezanson JeffBezanson deleted the aa/stop-exporting-gc-stuff branch January 19, 2018 15:48
@stevengj
Copy link
Member

Would be good to have Compat support for these.

@martinholters
Copy link
Member

We have since JuliaLang/Compat.jl#477, except for @preserve.

@jmkuhn jmkuhn mentioned this pull request Feb 14, 2018
Merged
@stevengj
Copy link
Member

@preserve is the one I needed 😬

@stevengj
Copy link
Member

Possibly Compat.GC.@preserve could just be a no-op in Julia 0.6, since that had a less aggressive garbage collector (see #23562)?

@ararslan ararslan mentioned this pull request Feb 14, 2018
Closed
@cormullion cormullion mentioned this pull request Mar 18, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vtjnash vtjnash vtjnash left review comments

@JeffBezanson JeffBezanson JeffBezanson approved these changes

@StefanKarpinski StefanKarpinski StefanKarpinski approved these changes

Assignees
No one assigned
Labels
deprecation This change introduces or involves a deprecation GC Garbage collector
Projects
None yet
Milestone
0.7
Development

Successfully merging this pull request may close these issues.
7 participants
@ararslan @KristofferC @JeffBezanson @StefanKarpinski @stevengj @martinholters @vtjnash