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.

Sign up for GitHub

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

Jump to bottom

Refactor get_time_stamp_ns #34536

Merged
merged 14 commits into from
Apr 24, 2020
Merged

Refactor get_time_stamp_ns #34536

merged 14 commits into from
Apr 24, 2020

Conversation

fanyang-mono
Copy link
Member

fixes #34074

@monojenkins monojenkins mentioned this pull request Apr 4, 2020
Merged
CoffeeFlux
CoffeeFlux reviewed Apr 4, 2020
View reviewed changes
Copy link
Contributor

@CoffeeFlux CoffeeFlux left a comment

Choose a reason for hiding this comment

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

I like the idea of moving this, though I think I'd prefer to just move that whole batch of time functions instead of the one.

Also, if we're moving this into utils there should be an equivalent Windows implementation. It's not needed in mini-posix.c given the name, but headers in utils are more general.

@lpereira
Copy link
Contributor

lpereira commented Apr 7, 2020

In some places, clock_gettime() was being called with CLOCK_MONOTONIC, but this has now been changed. The parameter changes in certain scenarios, to things that do not mean the same.

Maybe introduce different functions that will give different times? e.g. mono_get_monotonic_time_ns() and mono_get_sampling_time_ns() or something like this

@lpereira lpereira added runtime-mono specific to the Mono runtime and removed area-Infrastructure labels Apr 7, 2020
lpereira
lpereira reviewed Apr 14, 2020
View reviewed changes
src/mono/mono/utils/mono-time.c Outdated
@@ -229,3 +233,48 @@ mono_100ns_datetime_from_timeval (struct timeval tv)

#endif

#ifdef HOST_DARWIN

static clock_serv_t sampling_clock_service;
Copy link
Contributor

Choose a reason for hiding this comment

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

This used to be initialized in clock_init() under mono-posix.c, but it's now always 0.

lpereira
lpereira reviewed Apr 14, 2020
View reviewed changes
src/mono/mono/utils/mono-time.c Outdated

#elif defined(HOST_LINUX)

static clockid_t sampling_posix_clock;
Copy link
Contributor

@lpereira lpereira Apr 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Same: the POSIX version of clock_init() would initialize this, but it's now always zero. The POSIX version, also, will initialize this variable considering a MonoProfilerSampleMode, so this patch will break profiling.

@jaredpar jaredpar mentioned this pull request Apr 16, 2020
Closed
@fanyang-mono
Pass void* to init, clean_up and get_time function to accormodate dif…
5daacc1 
…ferent clock ID types for different os, and move back profiler specific function
CoffeeFlux
CoffeeFlux suggested changes Apr 20, 2020
View reviewed changes
Copy link
Contributor

@CoffeeFlux CoffeeFlux left a comment

Choose a reason for hiding this comment

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

Good initial work! I have some structural concerns, particularly surrounding the use of void * here.

src/mono/mono/utils/mono-time.c Outdated
@@ -229,3 +235,84 @@ mono_100ns_datetime_from_timeval (struct timeval tv)

#endif

#if defined(HOST_DARWIN) || defined(HOST_LINUX)
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer to have distinct sections for Windows, Darwin, and Linux, given they do not seem to share any code.

src/mono/mono/utils/mono-time.c Outdated Show resolved Hide resolved
src/mono/mono/utils/mono-time.h Outdated
@@ -29,6 +29,10 @@ gint64 mono_100ns_datetime (void);
gint64 mono_100ns_datetime_from_timeval (struct timeval tv);
#endif

void mono_clock_init (void *clk_id);
void mono_clock_cleanup (void *clk_id);
guint64 mono_clock_get_time_ns (void *clk_id);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm extremely uncomfortable casting everything to void * when the argument is quite different on different platforms. We should use a platform-specific typedef or something instead.

src/mono/mono/utils/mono-time.c Outdated Show resolved Hide resolved
src/mono/mono/utils/mono-time.c Outdated Show resolved Hide resolved
src/mono/mono/mini/mini-posix.c Show resolved Hide resolved
lpereira
lpereira reviewed Apr 20, 2020
View reviewed changes
src/mono/mono/mini/mini-posix.c Show resolved Hide resolved
src/mono/mono/utils/mono-time.c Outdated Show resolved Hide resolved
src/mono/mono/mini/mini-posix.c Show resolved Hide resolved
@fanyang-mono
Copy link
Member Author

Good initial work! I have some structural concerns, particularly surrounding the use of void * here.

The reason why I use void * here is because clock id has different types on mac and linux. That's the way I found to implement function overloading in C.

@CoffeeFlux
Copy link
Contributor

I think a platform-specific typedef is a much better approach, since then you still benefit from type checking and avoid a lot of potential casting mistakes. It also would let you clean up the calling code a bit.

@fanyang-mono fanyang-mono requested a review from BrzVlad April 22, 2020 14:40
@jaredpar jaredpar mentioned this pull request Apr 23, 2020
Closed
CoffeeFlux
CoffeeFlux approved these changes Apr 24, 2020
View reviewed changes
Copy link
Contributor

@CoffeeFlux CoffeeFlux left a comment

Choose a reason for hiding this comment

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

Just a quick style nit, but looks good overall. Thanks!

src/mono/mono/utils/mono-time.h Outdated Show resolved Hide resolved
@fanyang-mono fanyang-mono merged commit ede936d into dotnet:master Apr 24, 2020
@fanyang-mono fanyang-mono deleted the refactor_time_stamp branch April 24, 2020 15:36
@ghost ghost locked as resolved and limited conversation to collaborators Dec 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@lpereira lpereira lpereira left review comments

@BrzVlad BrzVlad BrzVlad approved these changes

@CoffeeFlux CoffeeFlux CoffeeFlux approved these changes

@lewurm lewurm Awaiting requested review from lewurm

@lambdageek lambdageek Awaiting requested review from lambdageek

@SamMonoRT SamMonoRT Awaiting requested review from SamMonoRT

@alexischr alexischr Awaiting requested review from alexischr

@vargaz vargaz Awaiting requested review from vargaz

Assignees
No one assigned
Labels
area-VM-meta-mono runtime-mono specific to the Mono runtime
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Refactor get_time_stamp_ns
6 participants
@fanyang-mono @lpereira @CoffeeFlux @BrzVlad @akoeplinger @Dotnet-GitSync-Bot