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

Fix Gc.minor_{words,free} by removing [@@noalloc] #1088

Merged
merged 1 commit into from Mar 9, 2017

Conversation

Projects
None yet
5 participants
@stedolan
Contributor

stedolan commented Mar 8, 2017

In native code, Gc.minor_words and Gc.minor_free return wrong data, with code like:

let a = Gc.minor_words () in
f ();
let c = Gc.minor_words () in
c -. a

returning 0 even if f allocates thousands of words. The issue is the [@noalloc] annotation on those functions: indeed, they do not allocate, but the [@noalloc] annotation prevents them seeing an accurate allocation pointer. By removing this annotation, they still don't allocate, but they return accurate data.

(I'm aware that the documentation says "This number is accurate in byte-code programs, but only an approximation in programs compiled to native code.", so one could argue that arbitrarily wrong answers are not a bug. Still, the results are badly wrong and easily fixed)

@chambart

This comment has been minimized.

Show comment
Hide comment
@chambart

chambart Mar 8, 2017

Contributor

I effectively overlooked that when introducing it. good catch.

Contributor

chambart commented Mar 8, 2017

I effectively overlooked that when introducing it. good catch.

@chambart chambart added the approved label Mar 8, 2017

Show outdated Hide outdated testsuite/tests/misc/gcwords.ml
generally boxes the floats *)
assert (n < 10.);
if Sys.backend_type = Sys.Native then assert (n = 0.);
let n = measure (fun () -> r := Some (allocate_lots 42 10)) in

This comment has been minimized.

@chambart

chambart Mar 8, 2017

Contributor

The right way to do that now is Sys.opaque_identity

@chambart

chambart Mar 8, 2017

Contributor

The right way to do that now is Sys.opaque_identity

This comment has been minimized.

@stedolan

stedolan Mar 8, 2017

Contributor

Changed.

@stedolan

stedolan Mar 8, 2017

Contributor

Changed.

@xavierleroy

This comment has been minimized.

Show comment
Hide comment
@xavierleroy

xavierleroy Mar 8, 2017

Contributor

Great catch-22, well spotted!

Could you please put the Changes entry in the "4.05.0" section? This is a bug fix and it should go both on trunk and on 4.05 branch.

Contributor

xavierleroy commented Mar 8, 2017

Great catch-22, well spotted!

Could you please put the Changes entry in the "4.05.0" section? This is a bug fix and it should go both on trunk and on 4.05 branch.

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Mar 8, 2017

Member

Note that this may break library that re-export the external declaration, as the type-checker will complain if the .ml and .mli disagree of noalloc-ness of a primitive. (I think that for example Batteries may have to be patched.)

(I still agree that fixing this and having the fix in 4.05 is the good solution.)

For this reason we may or may not mark it as a "breaking change" in the Changelog. It also mean that doing this as early as possible in the 4.05 release process is important -- to give library authors time to update their code before the release.

Member

gasche commented Mar 8, 2017

Note that this may break library that re-export the external declaration, as the type-checker will complain if the .ml and .mli disagree of noalloc-ness of a primitive. (I think that for example Batteries may have to be patched.)

(I still agree that fixing this and having the fix in 4.05 is the good solution.)

For this reason we may or may not mark it as a "breaking change" in the Changelog. It also mean that doing this as early as possible in the 4.05 release process is important -- to give library authors time to update their code before the release.

@stedolan

This comment has been minimized.

Show comment
Hide comment
@stedolan

stedolan Mar 8, 2017

Contributor

OK, Changes entry moved to 4.05 section. Should it be - or *?

@gasche re-exporting external declarations seems fragile. Would it not be better for Batteries to export val minor_words : unit -> float instead? Or even to just do include Gc?

Contributor

stedolan commented Mar 8, 2017

OK, Changes entry moved to 4.05 section. Should it be - or *?

@gasche re-exporting external declarations seems fragile. Would it not be better for Batteries to export val minor_words : unit -> float instead? Or even to just do include Gc?

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Mar 8, 2017

Member

Re. - and *, I think either are correct choices. In general there is an unspoken agreement that "having to change already-buggy code does not mandate a * mark". Here, the broken code is not buggy, but one could agree that this is a very specific failure scenario. I think the purpose of * marks is to let users quickly review what risks breaking their code when reading the Changelog, so the main question I think is whether this particular mark would be useful to anyone. For Batteries, I already know, so it's not very important; it may or may not affect other people (other external declarations are used in libraries, cf. the %string_set questions, but I don't know of any library that would export this one in particular). Long story short, do what you think is best.

Re. Batteries: The implementation does include Gc, but for the interface we re-export. There are several reasons, for example:

  • Having the interface of both Batteries-added functions and standard functions makes for an easier-to-browse documentation
  • Generally we could argue that if exporting val was always the better design, then the standard library should do it as well. If there are concerns with doing so (for example, performance, although inlining should solve that), the concerns probably also apply to Batteries. Mimicking stdlib is the safe bet here.
  • Whenever possible we want to make sure that the BatFoo module always also respect the module type of the standard Foo module; this lets clients use it interchangeably, and it is also the way we do internal testing (we check that this test pass to detect new stdlib functions we should bind and possibly backport).
Member

gasche commented Mar 8, 2017

Re. - and *, I think either are correct choices. In general there is an unspoken agreement that "having to change already-buggy code does not mandate a * mark". Here, the broken code is not buggy, but one could agree that this is a very specific failure scenario. I think the purpose of * marks is to let users quickly review what risks breaking their code when reading the Changelog, so the main question I think is whether this particular mark would be useful to anyone. For Batteries, I already know, so it's not very important; it may or may not affect other people (other external declarations are used in libraries, cf. the %string_set questions, but I don't know of any library that would export this one in particular). Long story short, do what you think is best.

Re. Batteries: The implementation does include Gc, but for the interface we re-export. There are several reasons, for example:

  • Having the interface of both Batteries-added functions and standard functions makes for an easier-to-browse documentation
  • Generally we could argue that if exporting val was always the better design, then the standard library should do it as well. If there are concerns with doing so (for example, performance, although inlining should solve that), the concerns probably also apply to Batteries. Mimicking stdlib is the safe bet here.
  • Whenever possible we want to make sure that the BatFoo module always also respect the module type of the standard Foo module; this lets clients use it interchangeably, and it is also the way we do internal testing (we check that this test pass to detect new stdlib functions we should bind and possibly backport).
@damiendoligez

This comment has been minimized.

Show comment
Hide comment
@damiendoligez

damiendoligez Mar 9, 2017

Member

The stdlib exports externals for historical reasons (concerns with the performance of byte-code programs). It is now considered a bad idea.

Still, this change is going to break the build for some libraries, so you should use a *.

Member

damiendoligez commented Mar 9, 2017

The stdlib exports externals for historical reasons (concerns with the performance of byte-code programs). It is now considered a bad idea.

Still, this change is going to break the build for some libraries, so you should use a *.

@stedolan

This comment has been minimized.

Show comment
Hide comment
@stedolan

stedolan Mar 9, 2017

Contributor

Still, this change is going to break the build for some libraries, so you should use a *.

Done.

Contributor

stedolan commented Mar 9, 2017

Still, this change is going to break the build for some libraries, so you should use a *.

Done.

@gasche gasche merged commit 06396f0 into ocaml:trunk Mar 9, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Mar 9, 2017

Member

Merged in trunk ( 06396f0 ) and 4.05 ( f541292 ).

Member

gasche commented Mar 9, 2017

Merged in trunk ( 06396f0 ) and 4.05 ( f541292 ).

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