Skip to content

make preserve/unpreserve_handle use the pin/unpin_count functions#96

Merged
d-netto merged 1 commit intommtk-support-moving-upstreamfrom
dcn-refactor-preserve-and-unpreserve_handle
Aug 21, 2025
Merged

make preserve/unpreserve_handle use the pin/unpin_count functions#96
d-netto merged 1 commit intommtk-support-moving-upstreamfrom
dcn-refactor-preserve-and-unpreserve_handle

Conversation

@d-netto
Copy link
Collaborator

@d-netto d-netto commented Aug 21, 2025

See PR's title.

@qinsoon, @udesou: this refactor fixes the LibCURL.jl tests.

              _
   _       _ _(_)_     |  Documentation: https://docs.julialang.org
  (_)     | (_) (_)    |
   _ _   _| |_  __ _   |  Type "?" for help, "]?" for Pkg help.
  | | | | | | |/ _` |  |
  | | |_| | | | (_| |  |  Version 1.13.0-DEV.356 (2025-08-21)
 _/ |\__'_|_|_|\__'_|  |  dcn-refactor-preserve-and-unpreserve_handle/a29d4f466d (fork: 46 commits, 147 days)
|__/                   |

julia> Base.runtests(["LibCURL"])
Running parallel tests with:
  getpid() = 39153
  nworkers() = 1
  nthreads(:interactive) = 1
  nthreads(:default) = 1
  Sys.CPU_THREADS = 8
  Sys.total_memory() = 15.399 GiB
  Sys.free_memory() = 9.352 GiB
  Sys.uptime() = 4455.67 (1.2 hours)

Test  (Worker) | Time (s) | GC (s) | GC % | Alloc (MB) | RSS (MB)
LibCURL    (1) |        started at 2025-08-21T15:19:24.775
LibCURL    (1) |     1.16 |   0.00 |  0.0 |      22.97 |   488.36

Test Summary: | Pass  Total  Time
  Overall     |    6      6  4.6s
    SUCCESS

@d-netto d-netto requested review from qinsoon and udesou August 21, 2025 19:20
v = get(uvhandles, x, 0)::Int
uvhandles[x] = v + 1
unlock(preserve_handle_lock)
@static if Base.USING_STOCK_GC
Copy link
Member

Choose a reason for hiding this comment

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

Is this supposed to be a temporary change?

Why can't we just use increment_tpin_count (or something similar) for the stock GC? We introduced the GC interface so that the runtime will not need to care about the underlying implementation of the GC. I think the pin/tpin count functions should be included in the interface.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As discussed in #95, the APIs might change before we settle on a final version.

I'd prefer to use the extensions once the final version is decided.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

to use the extensions

(for the stock GC)

Copy link
Member

Choose a reason for hiding this comment

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

As discussed in #95, the APIs might change before we settle on a final version.

I don't understand what you were saying. The API is temporary, but I was not talking about the APIs themselves. I was asking about how the API would be implemented, and how the library code would use the API. My feeling is that the API should be eventually forwarded to the GC interface, and the library should use the API without knowing the underlying GC implementation. The current code does not reflect this. I wanted to know if we agree on this and if the current implementation is temporary.

I'd prefer to use the extensions once the final version is decided.

What do you mean by 'extensions'?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My feeling is that the API should be eventually forwarded to the GC interface, and the library should use the API without knowing the underlying GC implementation

That's my long-term plan, indeed.

However, for us to do that, the stock GC will also need to be aware of explicit pinning and rooting introduced by these APIs. For example, in the current implementation, it would need to scan the maps where we keep the pinning counts.

I believe that propagating API changes to both GCs during this testing period in which the APIs are not final would lead to unnecessary work, and would prefer to implement them in the stock GC only when we're closer to a final version.

@d-netto d-netto merged commit f7f82b0 into mmtk-support-moving-upstream Aug 21, 2025
4 checks passed
@udesou
Copy link

udesou commented Aug 21, 2025

You should also open a PR to the binding, uncommenting any tests that you're fixing, similar to mmtk/mmtk-julia#252.

Use this comment to point to the branch you want to run the tests for. Possibly run any tests locally to check if the issue is fixed, but the places you want to uncomment are here and here.

@d-netto
Copy link
Collaborator Author

d-netto commented Aug 21, 2025

Thanks, @udesou. Will open a follow up PR.

@udesou
Copy link

udesou commented Aug 21, 2025

Since we have CI running there, you should probably do that before merging the PRs here just in case there is any regression...

uvhandles[x] = v + 1
unlock(preserve_handle_lock)
else
Base.increment_tpin_count!(x)
Copy link

Choose a reason for hiding this comment

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

Seems like these could actually be pinned rather than tpinned? Something to follow up on. 😊

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.

4 participants