This is an archive of the discontinued LLVM Phabricator instance.

[sanitizer] Implement NanoTime & MonotonicNanoTime for Windows
ClosedPublic

Authored by cryptoad on Jan 26 2018, 8:48 AM.

Details

Summary

Implement MonotonicNanoTime using QueryPerformanceCounter.

This function is used by Scudo & the 64-bit Primary allocator. Implementing it
now means that the release-to-OS mechanism of the Primary will kick in (it
never did since the function returned 0 always), but ReleaseMemoryPagesToOS is
still not currently implemented for Windows.

Performance wise, this adds a syscall & a 64-bit division per call to
MonotonicNanoTime so the impact might not be negligible, but I don't think
there is a way around it.

Diff Detail

Event Timeline

cryptoad created this revision.Jan 26 2018, 8:48 AM
Herald added subscribers: Restricted Project, delcypher, kubamracek. · View Herald TranscriptJan 26 2018, 8:48 AM

Actually, I guess we don't need to make it relative to a base, we can just use the current counter.
Which would also work as is for NanoTime.

cryptoad updated this revision to Diff 131603.Jan 26 2018, 9:12 AM

Implement NanoTime using QueryPerformanceCounter. MonotonicNanoTime will
just call NanoTime.

cryptoad retitled this revision from [sanitizer] Implement MonotonicNanoTime for Windows to [sanitizer] Implement NanoTime & MonotonicNanoTime for Windows.Jan 26 2018, 9:13 AM

Should we turn return-to-os off by default on Windows then? So that we won't waste time walking free lists.

lib/sanitizer_common/sanitizer_win.cc
505–514

Why function static, why not at the top level?

512

ULL

513

I wonder what frequency.QuadPart values typically are.

cryptoad updated this revision to Diff 132003.Jan 30 2018, 10:38 AM
cryptoad marked an inline comment as done.

U -> ULL

cryptoad added inline comments.Jan 30 2018, 10:38 AM
lib/sanitizer_common/sanitizer_win.cc
505–514

It's only used here, I didn't feel like it warranted being outside of the scope of the function.

513

On my test machine: 0x2fcd8 (ticks/s).

@rnk : how does this look to you?

What about my top level comment? Should we turn return-to-os off by default on Windows to donot waste time walking free lists?

What about my top level comment? Should we turn return-to-os off by default on Windows to donot waste time walking free lists?

I didn't see it, my apologies.
Yeah I think it's worth turning it off by default for now.
And on Fuchsia as well (cc: @flowerhack) as they don't have a return-to-os function either yet.
Want me to do it in a separate one 1st?

amccarth requested changes to this revision.Feb 1 2018, 11:23 AM
amccarth added a subscriber: amccarth.

rnk's been out sick, but this looks right to me.

Even with the multiplication by 10^9, you'd have to run for more than a year before you would risk an overflow (assuming the 195,800 ticks/second frequency seen by cryptoad).

This revision now requires changes to proceed.Feb 1 2018, 11:23 AM
amccarth accepted this revision.Feb 1 2018, 11:24 AM

LGTM. Wrong action selected in last update.

This revision is now accepted and ready to land.Feb 1 2018, 11:24 AM
alekseyshl accepted this revision.Feb 1 2018, 11:50 AM

What about my top level comment? Should we turn return-to-os off by default on Windows to donot waste time walking free lists?

I didn't see it, my apologies.
Yeah I think it's worth turning it off by default for now.
And on Fuchsia as well (cc: @flowerhack) as they don't have a return-to-os function either yet.
Want me to do it in a separate one 1st?

Whatever works, this patch or separate one, I'm fine with either.

cryptoad updated this revision to Diff 132447.Feb 1 2018, 12:07 PM

As per review comments, disable release-to-os by default on Windows (& Fuchsia).
Neither of those platforms implement ReleaseMemoryPagesToOS, but since we
are implementing MonotonicNanoTime, this would trigger spurious scanning of
the memory pages.

cryptoad marked 4 inline comments as done.Feb 1 2018, 12:08 PM
alekseyshl accepted this revision.Feb 1 2018, 1:37 PM
This revision was automatically updated to reflect the committed changes.