search

tweag / sparkle

Haskell on Apache Spark.
BSD 3-Clause "New" or "Revised" License
447 stars 30 forks source link

Use foreignptr values to cleanup global references in finalizers #85

Closed facundominguez closed 7 years ago

facundominguez commented 7 years ago

PR for inline-java https://github.com/tweag/inline-java/pull/28

robinbb commented 7 years ago

@facundominguez Please resubmit the PR into the LeapYear/sparkle repo so that we can track it with ZenHub, assign it, and merge it as we desire.

robinbb commented 7 years ago

@facundominguez Please also assign the PR to @alpmestan as a reviewer.

robinbb commented 7 years ago

(Also, I think that if you entitle the PR "Fix #85", then when the PR is merged this issue will automatically be closed.)

facundominguez commented 7 years ago

On @mboes behalf:

Fixing finalizers for global references in jvm is looking in good shape. I pushed a few changes to @facundo's branch that make the patch a bit smaller. Mostly by exploiting a new feature I contributed to inline-c today. All unit tests pass, but we need to find a good solution to @facundo's last comment in #28. Namely: we need to make finalizers a noop once the JVM itself has been finalized. I have actually observed in practice. It's relatively benign: it just means you'll sometimes get segfaults when everything is finished and you try to terminate your program, but it shouldn't affect computations. Also, I'll spend some time tomorrow to document the object lifecycle invariants I think we have converged to.

robinbb commented 7 years ago

The PR in tweag/inline-java#28 was merged. Is there more work to do in this issue?

facundominguez commented 7 years ago

It remains updating sparkle. We did it on our fork but not upstream. This will require either making a submodule of inline-java, or waiting for @mboes to release new versions of jni and jvm.

robinbb commented 7 years ago

@mboes What do you think?

facundominguez commented 7 years ago

This has been addressed upstream already.

facundominguez commented 7 years ago

Requires https://github.com/tweag/sparkle/pull/97, actually.