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

Unix.times unimplemented/suboptimal on Windows #5434

Closed
vicuna opened this issue Dec 18, 2011 · 6 comments

Comments

Projects
None yet
1 participant
@vicuna
Copy link

commented Dec 18, 2011

Original bug ID: 5434
Reporter: @jberdine
Status: closed (set by @xavierleroy on 2012-09-25T18:07:21Z)
Resolution: fixed
Priority: normal
Severity: feature
Version: 3.12.1
Category: ~DO NOT USE (was: OCaml general)
Monitored by: @protz @alainfrisch

Bug description

Here is a slightly improved implementation of Unix.times for Windows. It uses the Windows GetProcessTimes call, which has existed since Windows 2000. User and System time is reported in 100-ns units, although actual precision is less (seems to be 1/64 sec). This implementation makes no effort to report information on child processes.


#include <windows.h>
#include <caml/mlvalues.h>
#include <caml/fail.h>

double to_sec(FILETIME ft) {
ULARGE_INTEGER tmp;

tmp.u.LowPart = ft.dwLowDateTime;
tmp.u.HighPart = ft.dwHighDateTime;

/* convert to seconds:
GetProcessTimes returns number of 100-nanosecond intervals */
return tmp.QuadPart / 1e7;
}

value times(value unit) {

value res;
FILETIME creation, exit, stime, utime;
ULARGE_INTEGER tmp;

if (!(GetProcessTimes(GetCurrentProcess(), &creation, &exit, &stime, &utime)))
caml_failwith("GetProcessTimes failed");

res = alloc_small(4 * Double_wosize, Double_array_tag);
Store_double_field(res, 0, to_sec(utime));
Store_double_field(res, 1, to_sec(stime));
Store_double_field(res, 2, 0);
Store_double_field(res, 3, 0);
return res;

}

File attachments

@vicuna

This comment has been minimized.

Copy link
Author

commented Dec 18, 2011

Comment author: ripoche

I had something like that in mind too :)

Shouldn't the call to failwith be replaced with

win32_maperr(GetLastError());
uerror("times", Nothing);

Cheers

@vicuna

This comment has been minimized.

Copy link
Author

commented Dec 18, 2011

Comment author: @jberdine

Yes indeed, you are right about failwith.

Cheers, Josh

@vicuna

This comment has been minimized.

Copy link
Author

commented Dec 19, 2011

Comment author: @protz

Hi,

First of all thanks for the patch. I just gave your code a try and it seems indeed to work right, both on a 32-bit windows and a 64-bit windows. I was a little worried about compiler support for the QuadWord, but mingw handles this fine.

We don't pretend to be compatible with anything earlier than Windows 2000 so this looks like a good idea to me. Can you submit a patch using the (work-in-progress) guidelines available at http://yquem.inria.fr/~protzenk/caml-faq/doku.php?id=How%20to%20contribute ? Also, I'd like to see a comment explaining why you're not reporting on child processes (too hard on Windows?).

Also, the ULARGE_INTEGER is not needed in the body of the times function. Please remove its definition.

Thanks,

jonathan

@vicuna

This comment has been minimized.

Copy link
Author

commented Dec 20, 2011

Comment author: @jberdine

Please see the attached patch against the current trunk.

The error reporting is done as suggested by ripoche, and the dead variable has been eliminated.

As for the statistics for child processes, the real reason is that I never needed that information so I hadn't implemented it. I could be missing something, but as far as I know Windows does not maintain this information itself. I don't know of a better way to compute this than to enumerate all process on the machine, (partially) reconstruct the process tree, query and sum the statistics for all children individually. At least in my context where Unix.times can be called frequently, I would want to be very careful about making so many, perhaps unnecessary, system calls.

@vicuna

This comment has been minimized.

Copy link
Author

commented Dec 20, 2011

Comment author: @protz

Thanks! I'm about to commit this, please email me (jonathan.protzenko@inria.fr) your real name + email address for the commit message.

@vicuna

This comment has been minimized.

Copy link
Author

commented Dec 21, 2011

Comment author: @protz

Thanks, the patch has been committed. Unfortunately, I was hoping that you would appear as the author, but svn doesn't allow you to commit as someone else (I was hoping that the git-svn bridge I'm using would allow that somehow). I'll make sure to include your name and email in the commit message next time.

This is svn r11912

Cheers,

jonathan

@vicuna vicuna closed this Sep 25, 2012

@vicuna vicuna added the feature-wish label Mar 20, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.