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

Use global API table instead of each handle's having table ptr #4708

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

masa-koz
Copy link
Contributor

Description

This pull request includes several changes to the src/lib.rs file and an update to the Cargo.toml file. The primary focus is on removing the table pointer from various structs and centralizing the API table management using a static API instance. Additionally, the ctor crate is introduced for constructor and destructor functions to manage the API lifecycle.

Testing

cargo test

Documentation

No

@masa-koz masa-koz requested a review from a team as a code owner December 13, 2024 12:36
Cargo.toml Outdated Show resolved Hide resolved
src/lib.rs Outdated
pub static mut API: Api = Api { table: ptr::null() };

#[ctor::ctor]
fn open_msquic() {
Copy link
Member

Choose a reason for hiding this comment

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

It's unclear what calls this and when. Is this effectively called at DLL load (on Windows)?

Copy link
Member

Choose a reason for hiding this comment

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

Is there a way to make this lazy initialized (when the first registration is created) instead of a global init?

Copy link
Contributor

Choose a reason for hiding this comment

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

It runs when executable starts or dll load. The other option is to use global OnceCell, but that has the limitation that its destructor and Drop impl will not be executed (rust limitation), and memory and handle cleanup of the API table would be left to the os at program exit time.

Copy link
Contributor

Choose a reason for hiding this comment

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

It might be possible to do OnceCell to do lazy init, and then use dtor to close the api table.

src/lib.rs Outdated Show resolved Hide resolved
Copy link

codecov bot commented Dec 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.66%. Comparing base (9934866) to head (238fa63).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4708      +/-   ##
==========================================
- Coverage   86.79%   86.66%   -0.14%     
==========================================
  Files          56       56              
  Lines       17363    17363              
==========================================
- Hits        15071    15048      -23     
- Misses       2292     2315      +23     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/lib.rs Outdated
pub static mut API: Api = Api { table: ptr::null() };

#[ctor::ctor]
fn open_msquic() {
Copy link
Contributor

Choose a reason for hiding this comment

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

It runs when executable starts or dll load. The other option is to use global OnceCell, but that has the limitation that its destructor and Drop impl will not be executed (rust limitation), and memory and handle cleanup of the API table would be left to the os at program exit time.

@masa-koz
Copy link
Contributor Author

Now, we use lazy init for MsQuic API instead of a global init.

@masa-koz masa-koz changed the title Use global API table instead of each handle's having table ptr. Use global API table instead of each handle's having table ptr Dec 14, 2024
@nibanks nibanks added external Proposed by non-MSFT Language: Rust Related to the Rust interop layer labels Dec 16, 2024
src/lib.rs Outdated
}
}

pub static mut API: LazyLock<Api> = LazyLock::new(|| {
Copy link
Member

Choose a reason for hiding this comment

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

I assume this means it actually uses a Lock every time the API is accessed now?

Copy link
Contributor Author

@masa-koz masa-koz Dec 17, 2024

Choose a reason for hiding this comment

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

No. I suppose that we can access API in paralell from more than one threads after initiliazation. LazyLock acquires "lock" only when it executes the closure for initilization.

https://github.com/rust-lang/rust/blob/6d9f6ae36ae1299d6126ba40c15191f7aa3b79d8/library/std/src/sync/lazy_lock.rs#L207-L224

https://github.com/rust-lang/rust/blob/6d9f6ae36ae1299d6126ba40c15191f7aa3b79d8/library/std/src/sync/once.rs#L152-L158

Copy link
Member

Choose a reason for hiding this comment

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

But usually, you need a lock to check for initialization too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. But I suppose that the cost is cheap because an AtomicPtr is used to check for initilization.
https://github.com/rust-lang/rust/blob/6d9f6ae36ae1299d6126ba40c15191f7aa3b79d8/library/std/src/sys/sync/once/queue.rs#L125C1-L129C62

Copy link
Member

Choose a reason for hiding this comment

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

But it imposes a cost on every single usage of the variable, which might be happening on a LOT of parallel threads. Imagine a machine with a 100+ CPUs, all grabbing the lock every time they want to check if the variable is initialized.

I think something a bit better is necessary. Only when you are initializing a registration might you not have an initialized library after; but once you have a registration, you know you have initialized the library. So I'm wondering if we have (1) an unprotected global variable for the library and (2) a lazy 'run once' call when RegistrationOpen is called to make sure the global variable is properly set.

Copy link
Contributor

Choose a reason for hiding this comment

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

The first time init is expensive and requires lock. After that is should be cheap. But if it is just an atomic var comparison, it is still expensive. (Does it messup the cpu cache? with atomic acquire Relaxed?)

This is similar to cpp11 thread save static var init: https://en.cppreference.com/w/cpp/language/storage_duration#Static_block_variables
It can do none-atomic bool check.

Copy link
Member

Choose a reason for hiding this comment

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

You still need to have a volatile read, where the only real place you need synchronization is the initialization point of all the callers (i.e. where they call RegistrationOpen). I want to make sure we don't introduce overhead here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We initialize APITABLE when we first initialize Api or Registration using std::sync::Once. There is no longer overhead when accessing APITABLE!

pub fn new() -> Self {
// We initialize APITABLE at only once.
unsafe {
START_MSQUIC.call_once(|| {
Copy link
Member

Choose a reason for hiding this comment

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

I'm confused why this is both in the Api and Registration interface. Aren't they separate 'call once' things?

Copy link
Contributor Author

@masa-koz masa-koz Dec 18, 2024

Choose a reason for hiding this comment

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

Api may be initialized and its method may be called before registration initilized. But Api may not be initialized. So, I suppose that we should call MsQuicOpenVersion() when Api OR Registration is initialized.
And both Api::new() and Registration::new() will access the same START_MSQUIC. If Api is initialized before Registration is initialized, the closure which is passed to START_MSQUIC.call_once in Registration::new will not be executed because START_MSQUIC.call_once is already executed in Api::new(). If Registration is first initialized, its closure will be executed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
external Proposed by non-MSFT Language: Rust Related to the Rust interop layer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants