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

SystemTime::now should use GetSystemTimePreciseAsFileTime where available #67266

Closed
dbregman opened this issue Dec 12, 2019 · 6 comments
Closed
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. O-windows Operating system: Windows T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@dbregman
Copy link

The current implementation of std::time::SystemTime::now on Windows uses the GetSystemTimeAsFileTime function. This function has relatively poor resolution, between 1-16ms depending on the system, which is unacceptable for many real-world applications.

GetSystemTimePreciseAsFileTime on the other hand has a resolution between 1us and 100ns. This extra precision comes at a small performance cost[1], but it is still an extremely lightweight function call. I measured it at 20ns/call on my system. I believe GetSystemTimePreciseAsFileTime should be preferred whenever it is available (Windows Vista and higher)

There seems to be some FUD out there surrounding this function, however Microsoft recommends using this function in this fairly recent article [2] Quote:

When you need UTC-synchronized time stamps with a resolution of 1 microsecond or better, choose GetSystemTimePreciseAsFileTime

See also: Same issue was identified and fixed a couple years ago in the .net runtime: [3]

[1] https://devblogs.microsoft.com/oldnewthing/20170921-00/?p=97057
[2] https://docs.microsoft.com/en-us/windows/win32/sysinfo/acquiring-high-resolution-time-stamps.
[3] dotnet/coreclr#9736

@jonas-schievink jonas-schievink added C-enhancement Category: An issue proposing an enhancement or a PR with one. O-windows Operating system: Windows T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Dec 12, 2019
@dbregman
Copy link
Author

Since I needed it for my own use, here is an example implementation that does the fallback on pre-Vista systems.

#[cfg(windows)]
pub fn now() -> Instant {
	use std::sync::atomic::{AtomicUsize, Ordering};
	use winapi::shared::minwindef::{FILETIME, LPFILETIME};
	use winapi::um::libloaderapi;
	use winapi::um::sysinfoapi::GetSystemTimeAsFileTime;
	static PTR: AtomicUsize = AtomicUsize::new(0);
	// FIXME: not even sure if this atomic ordering stuff is needed.
	// code adapted from https://github.com/rust-lang/rust/blob/master/src/libstd/sys/windows/compat.rs
	let addr = match PTR.load(Ordering::SeqCst) {
		0 => {
			let module = b"kernel32\0".as_ptr() as *const i8;
			let symbol = b"GetSystemTimePreciseAsFileTime\0".as_ptr() as *const i8;
			let addr = unsafe {
				let handle = libloaderapi::GetModuleHandleA(module);
				match libloaderapi::GetProcAddress(handle, symbol) as usize {
					0 => GetSystemTimeAsFileTime as usize, // fallback function
					addr => addr,
				}
			};
			PTR.store(addr, Ordering::SeqCst);
			addr
		}
		addr => addr,
	};
	let ticks = unsafe {
		let mut ft: FILETIME = std::mem::zeroed();
		type PFNGSTAFT = unsafe extern "system" fn(LPFILETIME);
		std::mem::transmute::<usize, PFNGSTAFT>(addr)(&mut ft as LPFILETIME);
		std::mem::transmute::<FILETIME, u64>(ft) - 116444736000000000
	};
	Instant::from_ticks(ticks)
}

@retep998
Copy link
Member

std already has code in place for loading functions that might not exist, so this should be a relatively simple change, and nowhere as complicated as that example implementation.

@spunit262
Copy link
Contributor

XP will likely be split off into it's own target, so fallback won't be needed if that happens (#66801)

@ollie27
Copy link
Member

ollie27 commented Dec 31, 2019

GetSystemTimePreciseAsFileTime was actually added in Windows 8 so even if we completely dropped support for XP and Vista the fallback would still be needed for Windows 7.

@da-x
Copy link
Member

da-x commented Mar 9, 2020

I opened a PR #69858 for this.

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Mar 16, 2020
std: on Windows, use GetSystemTimePreciseAsFileTime if it is available

This implements rust-lang#67266.
@arthurprs
Copy link
Contributor

I think this can be closed now that #69858 is merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. O-windows Operating system: Windows T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

8 participants