Implement -XStaticValues
ClosedPublic

Authored by facundominguez on Dec 2 2014, 10:57 AM.

Details

Summary

As proposed in [1], this extension introduces a new syntactic form
static e, where e :: a can be any closed expression. The static form
produces a value of type StaticPtr a, which works as a reference that
programs can "dereference" to get the value of e back. References are
like Ptrs, except that they are stable across invocations of a
program.

The relevant wiki pages are [2, 3], which describe the motivation/ideas
and implementation plan respectively.

Authored-by: Facundo Domínguez <facundo.dominguez@tweag.io>
Authored-by: Mathieu Boespflug <m@tweag.io>
Authored-by: Alexander Vershilov <alexander.vershilov@tweag.io>

[1] Jeff Epstein, Andrew P. Black, and Simon Peyton-Jones. Towards
Haskell in the cloud. SIGPLAN Not., 46(12):118–129, September 2011. ISSN
0362-1340.
[2] https://ghc.haskell.org/trac/ghc/wiki/StaticPointers
[3] https://ghc.haskell.org/trac/ghc/wiki/StaticPointers/ImplementationPlan

Test Plan

./validate

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
There are a very large number of changes, so older changes are hidden. Show Older Changes
bgamari added inline comments.Dec 3 2014, 11:20 AM
compiler/deSugar/Desugar.lhs
131 ↗(On Diff #1830)

Perhaps be more specific: "Stub to insert the static entries of the module into the static pointer table"

140 ↗(On Diff #1830)

What is going on with the commented code here and the next lines?

compiler/deSugar/DsExpr.lhs
66 ↗(On Diff #1830)

Perhaps this can be removed?

compiler/deSugar/DsMeta.hs
1095

Is it just me or is the alignment off here?

compiler/deSugar/DsMonad.lhs
509 ↗(On Diff #1830)

This is visible; it should have a Haddock comment. Perhaps,

-- | Retrieve the global static bindings list.
rts/Hash.c
103

is there a reason why StgWord64 wasn't chosen here in place of uint64_t? It seems this would eliminate the need for unistd.h.

facundominguez added inline comments.Dec 3 2014, 12:26 PM
rts/SPT.c
19 ↗(On Diff #1830)

I may be getting wrong the meaning of the C code. The intention is to pass the "key" array pointer to the hashtable rather than truncating the array.

  • Treat body of static as top-level bindings for constraint resolution. Floating the body works again.
  • Implement Static* as KnownTypes.
  • Have hs_spt_lookup return NULL if the SPT is not initialized.
  • Collect SPT entries produced in splices.
  • Have CgStaticPointers rehearse the lookups after caching values is implemented.
  • Edit comments in tests.
bgamari added inline comments.Dec 3 2014, 12:49 PM
rts/SPT.c
19 ↗(On Diff #1830)

I'm afraid not. You are currently using the address of key as your key. lookupHashTable takes the key by value,

void *lookupHashTable(HashTable *table, StgWord key)

By contrast, your intention would require that the key be passed by reference (and an explicit length be provided as C has no concept of dynamically sized arguments, only pointers). This would look something like this,

void *lookupHashTable(HashTable *table, StgWord *key, size_t key_len)

That being said, it seems to me that one 64-bit word should be a large enough key-space for this application, no?

bgamari added inline comments.Dec 3 2014, 1:02 PM
rts/SPT.c
19 ↗(On Diff #1830)

Ahhh, I see what's happening here. The hash-table doesn't care about the contents of the key; it's just an opaque word. hashFingerprint later casts this back to a pointer. Sorry for the noise, this should work as-is.

qnikst added a subscriber: qnikst.Dec 3 2014, 3:11 PM
hvr added inline comments.Dec 3 2014, 3:14 PM
rts/Hash.c
103

Btw, uint64_t officially lives in the portable C99 header <stdint.h>

austin requested changes to this revision.Dec 3 2014, 6:20 PM
austin edited edge metadata.

First review pass. This generally looks pretty good and quite easy to understand; I'll take another look soon.

compiler/deSugar/DsExpr.lhs
947 ↗(On Diff #1845)

Add haddocks for this function.

compiler/deSugar/SPT.lhs
7 ↗(On Diff #1845)

Please rename this module; SPT alone is a bit odd. The abbreviated name could just be StaticPtrTbl, which is roughly inline with the identifier usage in other places.

84 ↗(On Diff #1845)

Fix the lint errors; and you should also replace ptext . sLit with text.

85 ↗(On Diff #1845)

Ditto

compiler/prelude/PrelNames.lhs
1180 ↗(On Diff #1845)

Fix these too. I would just indent like:

staticNameTyConName :: Name
staticNameTyConName
  = tcQual gHC_STATICPTR (fsLit "StaticName") staticNameTyConKey
compiler/prelude/TysWiredIn.lhs
1 ↗(On Diff #1845)

? Either way, we have no .lhs files anymore. :)

includes/rts/SPT.h
1–32 ↗(On Diff #1845)

Also, similar to the other comment, please rename this StaticPtrTbl.h and the .c file the same way, please.

32 ↗(On Diff #1845)

s/RTS_HPC_H/RTS_SPT_H/

libraries/base/GHC/StaticPtr.hs
31–36

Please move the extensions to the top of the file, above the module header (this is how we do it in most of the others).

96–106

Add Haddocks for these two functions, even though they aren't exported (we try to do this for all new top-level functions).

rts/Hash.c
116

Please make this more readable.

This revision now requires changes to proceed.Dec 3 2014, 6:20 PM
facundominguez edited edge metadata.
  • Undo unneeded export of tcClass in TcHsType.
  • Remove spurious changes.
  • Add coverage ticks to bodies of static expressions.
  • Replace uint64_t with StgWord64 in Hash.*.
  • Address comments from bgamari, hvr, austin and linter.
facundominguez edited edge metadata.
  • Remove spurious comment.
  • Add missing test output.
simonmar requested changes to this revision.Dec 4 2014, 3:57 AM
simonmar edited edge metadata.

Looks like you've done a nice job here, I reviewed the RTS and codegen parts but didn't have much to say. The main thing is to add a way to deallocate the hash table in the RTS (see inline comment).

compiler/deSugar/StaticPtrTable.lhs
1 ↗(On Diff #1851)

You might want to convert this to a .hs file, because it will conflict with the change to get rid of .lhs files that is due to go in shortly.

63 ↗(On Diff #1851)

We're not fingerprinting TypeReps? I thought that was the idea.

rts/Hash.c
108–114

Is this necessary? I'd have thought the fingerprint itself (or the low word of it) is a good enough hash by itself, without hashing it further.

rts/StaticPtrTable.c
2–4

It's customary to put a couple of sentences in a comment at the top of the file to explain what the file is for, and to point to relevant documentation/wiki pages.

11

I whether it might make more sense to pass the fingerprint by value, so that the caller doesn't have to allocate memory for it. Just a thought, I don't object to doing it this way if you think it's better.

12–13

You have to init this way because hs_spt_insert is called from constructors (a comment to that effect would be good), but there should also be a way to deallocate this hash table. Please add exitStaticPtrTable() and call it from hs_ext().

This revision now requires changes to proceed.Dec 4 2014, 3:57 AM
facundominguez added inline comments.Dec 4 2014, 5:43 AM
libraries/base/GHC/StaticPtr.hs
31–36

Probably this is better discussed after implementing the latest API proposal. But I anticipate that there will be an unsafeLookupStablePtr call which can potentially crash the program if used on the wrong type.

facundominguez added inline comments.Dec 4 2014, 5:56 AM
compiler/deSugar/StaticPtrTable.lhs
63 ↗(On Diff #1851)

This may be related to the final version of the extension. In the interim version we don't try to achieve type safety. Let me know if you had some other goal in mind.

facundominguez added inline comments.Dec 4 2014, 6:38 AM
rts/StaticPtrTable.c
11

I think this is not an issue, because the call site looks like

hs_spt_insert((StgWord64[2]){0UUL, 1ULL}, &_closure);

So the array literal is valid for the lifetime of the program or so I've read over there.

bgamari added inline comments.Dec 4 2014, 7:41 AM
rts/StaticPtrTable.c
11

Something to consider here would be how this would be affected if one wanted dynamic code loading/unlloading. It would certainly be tricky to get right in this context, but it would be nice to at least think about it at this stage.

facundominguez edited edge metadata.
  • Address simonmar comments.
  • Remove more spurius changes.
facundominguez edited edge metadata.
  • Implement interim StaticPtr API.
  • Update comments and manual.
facundominguez added inline comments.Dec 4 2014, 7:30 PM
compiler/deSugar/DsExpr.lhs
472 ↗(On Diff #1861)

I couldn't find a better way to create a core expression for the fingerprint. fingerprintDataCon has type Word# -> Word# -> Fingerprint on 64 bits.

I tried to use the wrapper with type Word64 -> Word64 -> Fingerprint instead, but then I didn't found a simple way to create literals of the lifted type Word64.

simonmar added inline comments.Dec 5 2014, 6:34 AM
includes/rts/StaticPtrTable.h
33–35

This is a private RTS function, so its definition should go in a header file under rts rather than includes, and use the BeginPrivate/EndPrivate declarations (there are lot of examples under rts to follow).

  • Move exitStaticPtr prototype to rts/StaticPtrTable.h.
  • Fixed location of keys in initialization of the SPT.
simonpj added inline comments.Dec 5 2014, 11:51 AM
compiler/deSugar/Desugar.lhs
96 ↗(On Diff #1862)

I'm still unhappy with this field. It's only needed by the desugarer, not the type checker.

If you are saying that splices encountered *and run* (and hence desugared, converted to code etc) by the type checker may mention statics, well I'm terribly unsure what that even means. You may say that

...$( f (static e) )....

is equivalent to

t = static e
... $(f (static e))...

but splices can't usually mention things bound at top level in *this* module. What if f serialses the static?

It smells bad to me. At very least, it needs a big Note, with an example, and the kind of code you expect to generate.

Personally I think it's a distraction, and you should simply make it illegal. Then you would not need this field in TcGblEnv at all.

compiler/deSugar/DsExpr.lhs
426 ↗(On Diff #1862)

Not quite. It's more like

g x = ....(static f)....

is replaced by

spe1 :: StaticPtr (Int -> Int)
spe1 = StaticPtr (StaticName "pkg id of f" "module of f" "f")

g x = ...spe1....
464 ↗(On Diff #1862)

No! return (Var speId)!

simonpj accepted this revision.Dec 5 2014, 11:51 AM
simonpj edited edge metadata.

Have not reviewed thoroughly yet

facundominguez added inline comments.Dec 5 2014, 12:11 PM
compiler/deSugar/Desugar.lhs
96 ↗(On Diff #1862)

I have no problem removing this. At this time is a matter of time mostly. What's the most effective way to recognize in the renamer that the static form occurs in a splice?

compiler/deSugar/DsExpr.lhs
464 ↗(On Diff #1862)

I would, but the core linter complains that "sptEntry:0" (the name of speId) is out of scope.

Even if that worked, the type of speId is quantified (e.g. forall a . StaticPtr (a -> a)) while the type of spe is not, but I guess I could return something like mkApps (Var speId) (map Type tvars) if we can bring sptEntry:0 in scope.

facundominguez added inline comments.Dec 5 2014, 12:19 PM
compiler/deSugar/DsExpr.lhs
464 ↗(On Diff #1862)

hm, disregard the bit about sptEntry:0 not in scope.

facundominguez edited edge metadata.
  • Address simonpj comments in DsExpr.lhs.
  • Make the static form illegal in splices.
  • Remove tcg_static_binds from TcGblEnv.
austin accepted this revision.Dec 5 2014, 2:28 PM
austin edited edge metadata.

LGTM with another pass.

@facundominguez - can you rebase this please to get it ready to land? HEAD has changed a bit; there aren't .lhs files anymore, and a bit of DynFlags etc has changed. Should not take long I hope. Thank you!

austin retitled this revision from Implement -XStaticValues V2. to Implement -XStaticValues.Dec 5 2014, 2:32 PM
austin updated this object.
austin edited the test plan for this revision. (Show Details)
austin edited edge metadata.
austin updated this object.Dec 5 2014, 2:35 PM

Rebased master.

I meant to add a source location to StaticPtrInfo and a top-level staticPtrKeys :: [StaticKey] today but with the last minute issues I couldn't.

Will take care of that next week, probably in a separate patch.

Many thanks to all reviewers.

  • Simplify sptInitCode.
  • Simplify DsExpr.hs.
  • Implement GHC.StaticPtr.staticPtrKeys.
  • Implement spInfoSrcLoc field of StaticPtrInfo.
This revision was automatically updated to reflect the committed changes.