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

Remove float array optimisation #163

Closed
wants to merge 7 commits into from
Closed

Conversation

lpw25
Copy link
Contributor

@lpw25 lpw25 commented Apr 8, 2015

The float array optimisation has a number of negative consequences. Alain's blog post contains a good description of the most important ones.

So here is a patch to remove the float array optimisation. It does three things:

  • It adds a FloatArray (and corresponding FloatArrayLabels) module to the standard library. The type FloatArray.t is abstract and is represented using the optimised float array representation. It provides the same operations as Array, as well as a create function similar to the one in String. The primitives used by FloatArray are defined in double_array.c.
  • It removes the optimised representation for float array.
  • It allows float lazy values to be short-circuited by the garbage collector -- since the risk from float arrays is gone.

@alainfrisch
Copy link
Contributor

Thanks, this is great! I'm not sure that this could be integrated directly, however, since it would directly degrade performance of any numerical code that rely on float arrays. What do you think of the migration path I describe in my blog post? First make available the new FloatArray module and add some helpers to find out when they should be used (static analysis of .cmt files to find occurences of "float array" and/or runtime warnings to detect and report the creation of such float arrays [e.g. with some stacktrace]); and only remove the support for the unboxed representation in a later version.

@hhugo
Copy link
Contributor

hhugo commented Apr 8, 2015

Any consequences for records of float ?

@lpw25
Copy link
Contributor Author

lpw25 commented Apr 8, 2015

What do you think of the migration path I describe in my blog post?

I would love to see it gone as soon as possible, but you're right that it might be worth doing the removal in stages. This would mean applying the first two commits of the patch now, and the rest later.

I'm not sure that static analysis of .cmt files would be significantly more useful than grep. A warning might be more useful, since it would inform people that didn't know that the change was coming.

Any consequences for records of float ?

No, they are unchanged.

@alainfrisch
Copy link
Contributor

A warning might be more useful,

Yes, indeed.

I would love to see it gone as soon as possible

Me too! For the records, I think I remember Xavier saying he was still fond of automatically unboxed float arrays. It's probably best to wait for his opinion before spending too much time on that. (Maybe the argument about Coq will facilitate the discussion :-) )

@xavierleroy
Copy link
Contributor

It would have been better to discuss this proposal on caml-devel before starting coding. For the record, I am opposed to removing the float array optimization.

@gasche
Copy link
Member

gasche commented Apr 9, 2015

Why doesn't Floatarray use the new support for custom index operators (.()) and (.()<-) introduced by #69 ? This would reduce the readability cost of moving from Array to FloatArray.

@chambart
Copy link
Contributor

One potential slow way to move forward would be to introduce an new 'array' type that always box float value. If it gets used, some day the array type could be deprecated. But that would need quite a lot of time to get rid of special cases for float (if it can ever happen).

@bluddy
Copy link
Contributor

bluddy commented Apr 13, 2015

@xavierleroy, could you outline your rationale for keeping the float array optimization? So far, I have only seen good arguments for removing it.

let-def and others added 2 commits May 22, 2015 10:17
Parallel build was broken
@damiendoligez damiendoligez added this to the 4.04-or-later milestone Jan 25, 2016
@DemiMarie
Copy link
Contributor

What about Int32array, Int64array, IntArray, and NativeIntArray?

@lpw25
Copy link
Contributor Author

lpw25 commented Mar 26, 2016

This proposal has been subsumed by another (which doesn't have a PR yet) that will allow users to created unboxed array types for types other than float.

@lpw25
Copy link
Contributor Author

lpw25 commented Sep 11, 2017

I guess I'll close this in favour of #1294.

@lpw25 lpw25 closed this Sep 11, 2017
chambart pushed a commit to chambart/ocaml-1 that referenced this pull request Sep 9, 2021
* Discard dead code after Itailcalls and Iextcalls that don't return

Revert a line from flambda-backend#72

* Don't propagate Ladjust_trap_depth past Llabel

* Add terminator "Throw" for invoking a continuation

Blocks that end with Iextcall that does not return
can be generated by Flambda2 (e.g., bounds check)
and result in a block without terminator.

* Rename Cfg.terminator.Throw to Call_no_return

* Don't propagate Ladjust_trap_depth past Lbranch

* Revert to pre-Cmm-traps version of discard_dead_code after Itailcall
EmileTrotignon pushed a commit to EmileTrotignon/ocaml that referenced this pull request Jan 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.