make preserve/unpreserve_handle use the pin/unpin_count functions#96
Conversation
| v = get(uvhandles, x, 0)::Int | ||
| uvhandles[x] = v + 1 | ||
| unlock(preserve_handle_lock) | ||
| @static if Base.USING_STOCK_GC |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
to use the extensions
(for the stock GC)
There was a problem hiding this comment.
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'?
There was a problem hiding this comment.
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.
|
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. |
|
Thanks, @udesou. Will open a follow up PR. |
|
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) |
There was a problem hiding this comment.
Seems like these could actually be pinned rather than tpinned? Something to follow up on. 😊
See PR's title.
@qinsoon, @udesou: this refactor fixes the
LibCURL.jltests.