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

Small tweak for some numeric functions in Pervasives #4070

Closed
vicuna opened this issue Jul 26, 2006 · 3 comments · Fixed by #10398
Closed

Small tweak for some numeric functions in Pervasives #4070

vicuna opened this issue Jul 26, 2006 · 3 comments · Fixed by #10398

Comments

@vicuna
Copy link

vicuna commented Jul 26, 2006

Original bug ID: 4070
Reporter: @mmottl
Status: acknowledged (set by @damiendoligez on 2006-08-29T14:59:36Z)
Resolution: open
Priority: normal
Severity: feature
Version: 3.08.4
Target version: undecided
Category: runtime system and C interface
Tags: patch
Monitored by: @jmeber @hcarty @Chris00 @mmottl

Bug description

The functions caml_frexp_float and caml_modf_float in byterun/floats.c could be implemented slightly more efficiently:

Both protect parameter "f", which is not necessary, because it is converted to a C-double before the next allocation anyway, and is not needed afterwards. Instead of "caml_alloc_tuple" it would be more efficient to use "Alloc_small", which would avoid unnecessary function calls. Finally, local value "res" does not need to be protected, because there are no allocations after it has been created.

Here are the alternative functions (untested):

CAMLprim value caml_frexp_float(value f)
{
CAMLparam0 ();
CAMLlocal1 (mantissa);
int exponent;
value res;

mantissa = caml_copy_double(frexp (Double_val(f), &exponent));
Alloc_small(res, 2, 0);
Field(res, 0) = mantissa;
Field(res, 1) = Val_int(exponent);
CAMLreturn (res);
}

CAMLprim value caml_modf_float(value f)
{
#if SC
_float_eval frem; /* Problem with Apple's <math.h> */
#else
double frem;
#endif
CAMLparam0 ();
CAMLlocal2 (quo, rem);
value res;

quo = caml_copy_double(modf (Double_val(f), &frem));
rem = caml_copy_double(frem);
Alloc_small(res, 2, 0);
Field(res, 0) = quo;
Field(res, 1) = rem;
CAMLreturn (res);
}

@vicuna
Copy link
Author

vicuna commented Jul 2, 2013

Comment author: @damiendoligez

Needs careful review and testing.

@github-actions
Copy link

This issue has been open one year with no activity. Consequently, it is being marked with the "stale" label. What this means is that the issue will be automatically closed in 30 days unless more comments are added or the "stale" label is removed. Comments that provide new information on the issue are especially welcome: is it still reproducible? did it appear in other contexts? how critical is it? etc.

@github-actions github-actions bot added the Stale label Apr 30, 2021
@damiendoligez
Copy link
Member

Do we really care all that much about the performance of these function? If so, will someone take this code and update it, then submit a PR?

cc @jmeber @hcarty @Chris00 @mmottl

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants