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

revert breaking change to GC.gc() #34336

Merged
merged 1 commit into from
Jan 15, 2020
Merged

revert breaking change to GC.gc() #34336

merged 1 commit into from
Jan 15, 2020

Conversation

vtjnash
Copy link
Member

@vtjnash vtjnash commented Jan 10, 2020

Removes the breaking part of b0ed147 from PR #34303

Removes the breaking part of b0ed147
from PR #34303
@StefanKarpinski
Copy link
Member

StefanKarpinski commented Jan 10, 2020

Definitely good to do this to unbreak CI, but this means there's no longer any way to call GC with the AUTO level where it decides if it wants to do a full or incremental cleaning automatically. Can you elaborate on what code was relying on GC.gc() doing a full collection?

@Keno
Copy link
Member

Keno commented Jan 10, 2020

Can you elaborate on what code was relying on GC.gc() doing a full collection?

SharedArrays is relying on GC.gc to actually delete the array and run the finalizer.

@Keno
Copy link
Member

Keno commented Jan 10, 2020

https://github.com/JuliaLang/julia/blob/master/stdlib/SharedArrays/test/runtests.jl#L148-L150

Of course, we could also change those to GC.gc(true), but we'd likely want to audit all other uses of this function also.

@maleadt
Copy link
Member

maleadt commented Jan 10, 2020

Of course, we could also change those to GC.gc(true), but we'd likely want to audit all other uses of this function also.

All uses of GC.gc() seem to be in tests, but then this change would be breaking. OTOH, the docstring never mentioned anything about full vs. incremental collections, so maybe it would be OK to do this change (but take care adapting the tests this time)?

Keno added a commit that referenced this pull request Jan 11, 2020
Fixes the same CI failure as #34336,
but hopefully should be immediately mergeable without objection about what
the interface is.
Keno added a commit that referenced this pull request Jan 11, 2020
Fixes the same CI failure as #34336,
but hopefully should be immediately mergeable without objection about what
the interface is.
@StefanKarpinski
Copy link
Member

I guess the question is what people probably want when they explicitly call GC.gc(). I'm guessing they want a full collection 99% of the time. What are the use cases for explicitly invoking an incremental collection or a "maybe incremental, maybe full" collection? Hard to think of any.

@maleadt
Copy link
Member

maleadt commented Jan 13, 2020

What are the use cases for explicitly invoking an incremental collection

GPU GC. From a resnet50 trace:

 ────────────────────────────────────────────────────
                                       Time          
                               ──────────────────────
       Tot / % measured:            82.2s / 84.0%    

 Section               ncalls     time   %tot     avg
 ────────────────────────────────────────────────────
 pooled alloc           65.1k    68.9s   100%  1.06ms
   1. try alloc         23.7k    5.66s  8.19%   239μs
   2. gc(false)         4.31k    15.6s  22.6%  3.62ms
     pooled free        33.1k   19.4ms  0.03%   586ns
   3. reclaim unused    3.32k    7.25s  10.5%  2.19ms
     reclaim            3.32k    7.24s  10.5%  2.18ms
     scan               3.32k   2.86ms  0.00%   863ns
   4. try alloc         3.32k    3.32s  4.81%  1.00ms
   5. gc(true)            127    36.9s  53.4%   291ms
     pooled free        24.2k   13.5ms  0.02%   557ns
 pooled free            6.78k   7.53ms  0.01%  1.11μs
 ────────────────────────────────────────────────────

So 97% of the time we need to collect GPU memory, an incremental collection is sufficient to free something up. Always doing a full collection (which here is 100x as expensive) would kill performance.

@StefanKarpinski
Copy link
Member

So I guess the remaining question is when does one want to explicitly invoke a GC that may be incremental or full?

@JeffBezanson
Copy link
Member

It does stand to reason that if you're explicitly asking for a GC, it should be of a specific kind.

@JeffBezanson JeffBezanson added this to the 1.4 milestone Jan 14, 2020
KristofferC pushed a commit that referenced this pull request Jan 15, 2020
Fixes the same CI failure as #34336,
but hopefully should be immediately mergeable without objection about what
the interface is.

(cherry picked from commit 3ed4e94)
@JeffBezanson JeffBezanson merged commit 15d693b into master Jan 15, 2020
@JeffBezanson JeffBezanson deleted the jn/34303-breakage branch January 15, 2020 18:13
KristofferC pushed a commit that referenced this pull request Jan 17, 2020
Fixes the same CI failure as #34336,
but hopefully should be immediately mergeable without objection about what
the interface is.

(cherry picked from commit 3ed4e94)
KristofferC pushed a commit that referenced this pull request Jan 21, 2020
Removes the breaking part of b0ed147
from PR #34303

(cherry picked from commit 15d693b)
KristofferC pushed a commit that referenced this pull request Jan 21, 2020
Removes the breaking part of b0ed147
from PR #34303

(cherry picked from commit 15d693b)
@KristofferC KristofferC mentioned this pull request Jan 21, 2020
7 tasks
KristofferC pushed a commit that referenced this pull request Apr 11, 2020
Fixes the same CI failure as #34336,
but hopefully should be immediately mergeable without objection about what
the interface is.
KristofferC pushed a commit that referenced this pull request Apr 11, 2020
Removes the breaking part of b0ed147
from PR #34303
KristofferC pushed a commit to JuliaLang/SharedArrays.jl that referenced this pull request Dec 4, 2024
Fixes the same CI failure as JuliaLang/julia#34336,
but hopefully should be immediately mergeable without objection about what
the interface is.
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.

6 participants