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

raw_spacetime_lib refactor + related fixes #1477

Merged
merged 6 commits into from Dec 7, 2017

Conversation

Projects
None yet
2 participants
@nojb
Copy link
Contributor

nojb commented Nov 13, 2017

I am trying to get Spacetime to work on Windows. As part of this, I did some refactorings of raw_spacetime_lib that have independent interest, which I am submitting in this PR. Along the way, it seems I found a bug in the handling of the deprecated "noalloc" attribute.

  • Move spacetime_offline.c (which is used to read the Spacetime profiles) out of the runtime system and into the library raw_spacetime_lib.
  • Use standard otherlibs/Makefile to simplify otherlibs/raw_spacetime_lib/Makefile
  • Allow the primitives in spacetime_offline.c to be used in bytecode as well. Previously in bytecode these primitives were short-circuited to fail, but I don't see any reason to do so and this allows to use raw_spacetime_lib from e.g. the toplevel.
  • Replace "noalloc" by [@@noalloc] in raw_spacetime_lib

This last change revealed that the deprecated attributes "noalloc", "float" were being ignored when appearing after a pair of names "bytecode primitive" "native-code primitive": see the commit "Fix deprecated noalloc handling".

I experimented a little bit with raw_spacetime_lib from the toplevel and it seems to work, but would appreciate a confirmation that these primitives are safe for bytecode from someone more knowledgeable about Spacetime internals.

@mshinwell @lpw25

@mshinwell
Copy link
Contributor

mshinwell left a comment

OK after minor changes addressed.

@@ -496,8 +472,7 @@ module Trace = struct

(* CR-soon lwhite: These functions should work in bytecode *)

This comment has been minimized.

@mshinwell

mshinwell Dec 6, 2017

Contributor

Please delete this CR-soon line.

@@ -85,7 +85,9 @@ let parse_declaration valdecl ~native_repr_args ~native_repr_res =
let arity = List.length native_repr_args in
let name, native_name, old_style_noalloc, old_style_float =
match valdecl.pval_prim with
| name :: name2 :: "noalloc" :: "float" :: _

This comment has been minimized.

@mshinwell

mshinwell Dec 6, 2017

Contributor

What are these changes needed for? I think these should go in a separate GPR.

This comment has been minimized.

@nojb

nojb Dec 6, 2017

Author Contributor

This fixes a bug in the handling of deprecated "noalloc" and "float" attributes (which is triggered by raw_spacetime_lib.ml as it exists on trunk), but indeed they are not needed in this PR; I will put it in a separate one.

@@ -601,7 +576,7 @@ module Heap_snapshot = struct
call_counts : bool;
}

let pathname_suffix_trace = "trace"
let _pathname_suffix_trace = "trace"

This comment has been minimized.

@mshinwell

mshinwell Dec 6, 2017

Contributor

Here and elsewhere, let's delete the unused values / functions.

@nojb nojb force-pushed the nojb:spacetime_fixes branch from ad5a6c4 to b308a92 Dec 6, 2017

@nojb

This comment has been minimized.

Copy link
Contributor Author

nojb commented Dec 7, 2017

Thanks for the review @mshinwell! Amended as suggested.

@mshinwell

This comment has been minimized.

Copy link
Contributor

mshinwell commented Dec 7, 2017

OK, I will merge. (By the way, I'm not sure if you rewrote existing changesets or something, but I wasn't able to see just the changes since my review. You might want to check your git workflow.)

@mshinwell mshinwell merged commit caf391f into ocaml:trunk Dec 7, 2017

2 checks passed

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

This comment has been minimized.

Copy link
Contributor Author

nojb commented Dec 7, 2017

I rebased on trunk, sorry about that!

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.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.