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

Skip some stock GC-specific tests when using MMTk #57104

Merged
merged 3 commits into from
Jan 24, 2025

Conversation

udesou
Copy link
Contributor

@udesou udesou commented Jan 20, 2025

As noted in #57103, there are a few tests that are specific to the stock GC and will fail for MMTk. For now, this PR simply skips those tests when not using the stock GC until we agree on how to approach the incompatibilities described in the issue.

@udesou udesou added the GC Garbage collector label Jan 20, 2025
@udesou udesou force-pushed the mmtk/skipping-some-tests branch from 874d41d to a991b9a Compare January 20, 2025 04:22
@udesou udesou changed the title Skipping some tests that are currently incompatible with MMTk Skip some stock GC-specific tests when using MMTk Jan 20, 2025
@udesou udesou force-pushed the mmtk/skipping-some-tests branch from a991b9a to bb00540 Compare January 20, 2025 05:40
@udesou udesou requested a review from d-netto January 20, 2025 23:14
@d-netto
Copy link
Member

d-netto commented Jan 21, 2025

Any chance we could avoid the duplication of const USING_STOCK_GC...?

@gbaraldi
Copy link
Member

That can probably be a global defined in Base or Sys

@udesou
Copy link
Contributor Author

udesou commented Jan 21, 2025

gcutils.jl maybe? Let me try that out. That didn't work, I got an undefined var error. I moved it to Base instead.

@udesou udesou force-pushed the mmtk/skipping-some-tests branch from bb00540 to 833c1b9 Compare January 22, 2025 00:14
Copy link
Member

@d-netto d-netto left a comment

Choose a reason for hiding this comment

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

For the tests in test/gc.jl, I also wonder whether it would be possible to guard these lines with a @static if ....

@testset "GC page metrics" begin
    run_nonzero_page_utilization_test()
    run_pg_size_test()
end
@testset "issue-54275" begin
    issue_54275_test()
end
@testset "Base.GC docstrings" begin
    @test isempty(Docs.undocumented_names(GC))
end
@testset "Full GC reasons" begin
    full_sweep_reasons_test()
end

As opposed to the individual function bodies.

stdlib/Profile/test/allocs.jl Outdated Show resolved Hide resolved
stdlib/Profile/test/allocs.jl Outdated Show resolved Hide resolved
stdlib/Profile/test/runtests.jl Outdated Show resolved Hide resolved
test/cmdlineargs.jl Outdated Show resolved Hide resolved
test/cmdlineargs.jl Outdated Show resolved Hide resolved
test/gc.jl Outdated Show resolved Hide resolved
test/gc.jl Outdated Show resolved Hide resolved
test/misc.jl Outdated Show resolved Hide resolved
test/gc.jl Outdated Show resolved Hide resolved
base/Base.jl Outdated
@@ -128,6 +128,8 @@ include("sysinfo.jl")
include("libc.jl")
using .Libc: getpid, gethostname, time, memcpy, memset, memmove, memcmp

const USING_STOCK_GC = occursin("stock", unsafe_string(ccall(:jl_gc_active_impl, Ptr{UInt8}, ())))
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should also expose GC.gc_active_impl() which returns the string, and then have a separate function GC.is_stock_gc() that does this occursin?

Copy link
Contributor Author

@udesou udesou Jan 23, 2025

Choose a reason for hiding this comment

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

I can add GC.gc_active_impl(). However, I got an undefined var error when I tried adding something with occursin to the GC module in gcutils.jl. I assume due to that function only being imported later on, and I wasn't sure if I should change that order...

@udesou udesou force-pushed the mmtk/skipping-some-tests branch 3 times, most recently from 182594a to 9f98de7 Compare January 23, 2025 22:44
@udesou udesou force-pushed the mmtk/skipping-some-tests branch 8 times, most recently from 519c8fb to c332104 Compare January 24, 2025 03:28
@udesou udesou force-pushed the mmtk/skipping-some-tests branch from c332104 to d5f0e83 Compare January 24, 2025 03:29
Copy link
Member

@d-netto d-netto left a comment

Choose a reason for hiding this comment

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

Please fix the grammar NIT.

Other than that, LGTM.

stdlib/Profile/test/runtests.jl Outdated Show resolved Hide resolved
@d-netto d-netto added the merge me PR is reviewed. Merge when all tests are passing label Jan 24, 2025
@d-netto d-netto merged commit 899d2f5 into JuliaLang:master Jan 24, 2025
6 of 8 checks passed
@d-netto d-netto removed the merge me PR is reviewed. Merge when all tests are passing label Jan 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GC Garbage collector
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants