Enhance `Managed_Resource` to allow implementation of in-memory caches #11577

JaroslavTulach · 2024-11-18T07:39:13Z

Pull Request Description

Allows connection between refresh button and caches as requested by #11485:

please use Managed_Resource.new system_resource=True to create a reference that "can disappear"
please call EnsoContext.getResourceManager().scheduleFinalization() to clear the caches when the refresh button is pressed

ResourceManager provides scheduleFinalization method that allows cleanup of "system managed" references

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

The documentation has been updated, if necessary.
All code follows the
Scala,
Java,
Unit tests have been written where possible.

engine/runtime/src/main/java/org/enso/interpreter/runtime/ReferencesManager.java

jdunkerley · 2024-11-18T08:59:09Z

How do we create a "static" instance of one of these?

Guess we just put one into Java as usual.

radeusgd · 2024-11-19T10:24:04Z

distribution/lib/Standard/Base/0.0.0-dev/src/Runtime/Ref.enso


       Arguments:
       - value: The value to be contained in the ref.
+       - allow_gc: Is the value eligible for being cleared by the system at any time?


Could we replace this with an atom type reference_type?

e.g.

type Reference_Type Weak Regular

Then if we ever want to also add support for soft references, it is just the matter of adding yet another variant to Reference_Type.

I will try to get this working with Greg's HTTP response caching, with TTL and LRU logic #11342

then we'll see what's actually needed

Right now I am thinking about:

type Ref_Auto_Clean Never # regular reference Explicit # only per user request Soon # using `WeakReference` disappears as soon as "possible" Managed estimated_memory_size:Integer time_in_ms_to_recompute:Integer # automatic when needed

the ..Explicit cleaning policy would solve @GregoryTravis's

the reference ... collected, then I know that the refresh button has been clicked

We would connect cleanup of ..Explicit directly to refresh button click and no GC.

Or we could map all above types to estimates of memory and recomputation time:

Never - zero memory, maximal time

Explicit - zero memory, zero time

Soon - some memory, zero time

Managed - some memory and some time

engine/runtime/src/main/java/org/enso/interpreter/runtime/data/Ref.java

engine/runtime-integration-tests/src/test/java/org/enso/interpreter/runtime/RefTest.java

Akirathan

Interesting idea. LGTM. I am a bit afraid about transient failures in the RefTest.

engine/runtime-integration-tests/src/test/java/org/enso/interpreter/runtime/RefTest.java

GregoryTravis · 2024-11-19T16:14:08Z

@JaroslavTulach I'll get this connected to my cache and write a test for it.

std-bits/base/src/main/java/org/enso/base/enso_cloud/EnsoSecretHelper.java

GregoryTravis · 2024-11-26T20:01:37Z

@JaroslavTulach
I implemented the logic we discussed -- my cache checks a reference to see if it has been cleared. The reference is created here.

@4e6
I modified RecomputeContextCmd to clear weak references on reload. This is just a proof-of-concept, I don't know if I did it correctly.

…lize was called

…ager11485

… finalized

JaroslavTulach · 2024-11-28T17:55:25Z

std-bits/base/src/main/java/org/enso/base/enso_cloud/EnsoSecretHelper.java

+          private Value ref:Managed_Resource
+
+          new obj -> Cache =
+              on_finalize _ = 0


@GregoryTravis - At the end I decided to use enhanced Managed_Resource as it already has support for GC and finalizers. Feel free to implement a cleanup logic in this on_finalize method.

If things seem to work for you and I get approvals from engine guys, let's integrate!

radeusgd · 2024-11-29T11:56:17Z

distribution/lib/Standard/Base/0.0.0-dev/src/Runtime/Managed_Resource.enso

-    register : Any -> (Any -> Nothing) -> Managed_Resource
-    register resource function = @Builtin_Method "Managed_Resource.register"
+    register : Any -> (Any -> Nothing) -> Boolean -> Managed_Resource
+    register resource function system_finalization_allowed=Boolean.False =


Suggested change

register resource function system_finalization_allowed=Boolean.False =

register resource function system_finalization_allowed=False =

I think the preferred convention for importing Boolean is:

from project.Data.Boolean import Boolean, False, True

radeusgd · 2024-11-29T11:58:25Z

distribution/lib/Standard/Base/0.0.0-dev/src/Runtime/Managed_Resource.enso

+       Value returned from the `action` or `Nothing` if the managed resource
+       was already finalized
+    with : (Any -> Any) -> Any -> Any
+    with self ~action ~on_missing=Nothing = with_builtin self action on_missing...


I think it would be much better to default to returning a dataflow error, or perhaps even a panic because it is an internal error.

Suggested change

with self ~action ~on_missing=Nothing = with_builtin self action on_missing...

with self ~action ~on_missing=(Panic.throw (Illegal_State.Error "The Managed_Resource is used after it has been finalized.")) = with_builtin self action on_missing...

Defaulting to Nothing may make it too easy for "usage after finalize" to go unnoticed and lead to hard to debug issues.

radeusgd · 2024-11-29T12:02:38Z

engine/runtime/src/main/java/org/enso/interpreter/runtime/ResourceManager.java

+   * @see #register
+   */
+  @CompilerDirectives.TruffleBoundary
+  public final synchronized void scheduleFinalization() {


nitpick, but IMO scheduleFinalizationOfSystemResources could be better

Suggested change

public final synchronized void scheduleFinalization() {

public final synchronized void scheduleFinalizationOfSystemResources() {

The name scheduleFinalization sounds like it may finalize all references.

Documentation clarifies this, so I'm ok to keep it as is though.

radeusgd · 2024-11-29T12:03:38Z

engine/runtime/src/main/java/org/enso/interpreter/runtime/data/ManagedResource.java

+/**
+ * An Enso runtime representation of a managed resource.
+ *
+ * <p><b>Instance of this class</b> is convoluted with instances playing various roles:
+ *
+ * <ul>
+ *   <li>this {@link ManagedResource} points to {@code resource}
+ *   <li>this {@link ManagedResource} points to {@link PhantomReference} that is "phantom
+ *       referencing" {@code this}
+ *   <li>the implementation of {@link PhantomReference} is {@code Item} in {@link ResourceManager}
+ *   <li>the {@code Item} <em>"phantom referencing"</em> {@code this} {@link ManagedResource} is
+ *       stored in {@link ResourceManager} {@code pendingItems} collection.
+ *   <li>the {@code Item} has a pointer to the {@code resource} as well
+ *   <li>the {@code Item} has a pointer to the {@code finalizer} function
+ * </ul>
+ *
+ * Once all this braided chunk of objects is eligible for GC because <b>nobody holds pointer to
+ * {@link ManagedResource}</b>, the {@code Item} is put into {@link ResourceManager} {@code
+ * referenceQueue} and process by the intricate machinery of {@link ResourceManager} and its {@code
+ * ProcessItems} processor.
+ */


Very appreciated

radeusgd

Looks good but I'd change the default on_missing to something more 'visible' than Nothing. We are having plenty of issues caused by accidentally discarding errors, so it's better if we program more 'defensively' against this.

Akirathan

LGTM. I have created #11714 to potentially remove now even more deprecated AutomaticParallelism IR pass and Name.Special IR elements.

Akirathan · 2024-11-29T12:01:01Z

distribution/lib/Standard/Base/0.0.0-dev/src/Runtime/Managed_Resource.enso

+       Value returned from the `action` or `Nothing` if the managed resource
+       was already finalized
+    with : (Any -> Any) -> Any -> Any
+    with self ~action ~on_missing=Nothing = with_builtin self action on_missing...


Remind me, what is the semantics of on_missing...?

Akirathan · 2024-11-29T12:03:24Z

...ne/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/resource/WithNode.java

+        resourceManager.unpark(mr);
+      }
+    } else {
+      return onMissing;


According to the docs of Managed_Resource.with, on_missing is "an optional action", not a constant. Clarify and/or add a test. If it is an action, it should be suspended thunk and called appropriatelly. Maybe even add @Suspended annotation to the parameter?

Akirathan · 2024-11-29T12:10:38Z

engine/runtime/src/main/scala/org/enso/interpreter/runtime/IrToTruffle.scala

@@ -1963,16 +1962,6 @@ class IrToTruffle(
              "a Self occurence must be resolved"
            ).target
          )


By removing handling of Special names here, and removing NewRefNode and co., you are making AutomaticParallelism IR pass even more obsolete than it was before (which seems like a good thing because I am convinced that AutomaticParallelism has never actually done anything and will not even work if enabled). It is worth thinking about getting rid of that pass, along with the Name.Special IR definitions altogether.

Akirathan · 2024-11-29T12:14:21Z

...untime-integration-tests/src/test/java/org/enso/interpreter/runtime/ManagedResourceTest.java

+  }
+
+  @Test
+  public void regularReference() throws Exception {


After copy-pasting from RefTest.java, please, at least rename names of the tests, as well as the private static fields like newRef, createRef, etc.

Using Ref.new allow_gc=True to implement in-memory caches

5266a4b

JaroslavTulach added the CI: Clean build required CI runners will be cleaned before and after this PR is built. label Nov 18, 2024

JaroslavTulach self-assigned this Nov 18, 2024

JaroslavTulach requested review from jdunkerley, radeusgd, GregoryTravis, AdRiley, marthasharkey, 4e6, hubertp and Akirathan as code owners November 18, 2024 07:39

JaroslavTulach commented Nov 18, 2024

View reviewed changes

engine/runtime/src/main/java/org/enso/interpreter/runtime/ReferencesManager.java Outdated Show resolved Hide resolved

Hide operations with References behind @TruffleBoundary

d803b28

Work with v and not this.value

38a20b4

JaroslavTulach changed the title ~~Using Ref.new allow_gc=True to implement in-memory caches~~ Using Ref.new allow_gc=True to implement in-memory caches Nov 18, 2024

4e6 approved these changes Nov 18, 2024

View reviewed changes

hubertp approved these changes Nov 18, 2024

View reviewed changes

enso-bot bot mentioned this pull request Nov 19, 2024

Allow (multi_value : Table) : Column to succeed #11482

Open

hubertp mentioned this pull request Nov 19, 2024

Optional scripts section in libraries' config #11555

Closed

5 tasks

radeusgd reviewed Nov 19, 2024

View reviewed changes

engine/runtime/src/main/java/org/enso/interpreter/runtime/data/Ref.java Outdated Show resolved Hide resolved

radeusgd reviewed Nov 19, 2024

View reviewed changes

engine/runtime/src/main/java/org/enso/interpreter/runtime/data/Ref.java Outdated Show resolved Hide resolved

JaroslavTulach commented Nov 19, 2024

View reviewed changes

engine/runtime-integration-tests/src/test/java/org/enso/interpreter/runtime/RefTest.java Show resolved Hide resolved

Akirathan approved these changes Nov 19, 2024

View reviewed changes

engine/runtime-integration-tests/src/test/java/org/enso/interpreter/runtime/RefTest.java Show resolved Hide resolved

JaroslavTulach added 3 commits November 20, 2024 14:36

Merging with latest develop

0f437fc

Fixing typo

404e3cc

Using Ref.new to cache reference to EnsoHTTPResponseCache

5629e20

JaroslavTulach commented Nov 20, 2024

View reviewed changes

std-bits/base/src/main/java/org/enso/base/enso_cloud/EnsoSecretHelper.java Outdated Show resolved Hide resolved

enso-bot bot mentioned this pull request Nov 27, 2024

fansi fails with IllegalArgumentException #11526

Closed

JaroslavTulach linked an issue Nov 27, 2024 that may be closed by this pull request

Refresh button should clear HTTP cache #11485

Open

JaroslavTulach added 10 commits November 27, 2024 17:37

Can no longer invoke Managed_Resource.with when Managed_Resource.fina…

85e8b6d

…lize was called

Backing out the Ref changes

e56b867

Merge tag '2024.5.1-nightly.2024.11.27' into wip/jtulach/ReferenceMan…

3ca29aa

…ager11485

Ref.new takes only one argument (again)

da2d438

Commenting out releaseAll call for now

ae83008

Allow system controlled Managed_Resource

276a885

Merging with most recent develop

205811c

on_missing behavior for managed resources that get access after being…

40aedfc

… finalized

Updating micro-distribution with the new Managed_Resource builtins

2e91135

Moving the GC related parts of RefTest to ManagedResourceTest

786f156

JaroslavTulach changed the title ~~Using Ref.new allow_gc=True to implement in-memory caches~~ Enhance Managed_Resource to allow implementation of in-memory caches Nov 28, 2024

Better note in changelog

934ec5f

JaroslavTulach requested review from radeusgd, Akirathan, 4e6 and hubertp November 28, 2024 17:22

JaroslavTulach added 2 commits November 28, 2024 18:46

Making public so the Fetch_Spec passes OK

a3c07e0

Using Managed_Resource inside of EnsoSecretHelper

a7890d2

JaroslavTulach commented Nov 28, 2024

View reviewed changes

radeusgd reviewed Nov 29, 2024

View reviewed changes

Akirathan mentioned this pull request Nov 29, 2024

Remove deprecated AutomaticParallelism IR pass and Name.Special IR elements #11714

Open

Akirathan approved these changes Nov 29, 2024

View reviewed changes

enso-bot bot mentioned this pull request Nov 29, 2024

Enso builtins don't expose Enso methods #11589

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhance `Managed_Resource` to allow implementation of in-memory caches #11577

Enhance `Managed_Resource` to allow implementation of in-memory caches #11577

JaroslavTulach commented Nov 18, 2024 •

edited

Loading

jdunkerley commented Nov 18, 2024 •

edited

Loading

radeusgd Nov 19, 2024

JaroslavTulach Nov 19, 2024

JaroslavTulach Nov 22, 2024

Akirathan left a comment

GregoryTravis commented Nov 19, 2024

GregoryTravis commented Nov 26, 2024 •

edited

Loading

JaroslavTulach Nov 28, 2024 •

edited

Loading

radeusgd Nov 29, 2024

radeusgd Nov 29, 2024

radeusgd Nov 29, 2024

radeusgd Nov 29, 2024

radeusgd left a comment

Akirathan left a comment

Akirathan Nov 29, 2024

Akirathan Nov 29, 2024

Akirathan Nov 29, 2024

Akirathan Nov 29, 2024

	register resource function system_finalization_allowed=Boolean.False =
	register resource function system_finalization_allowed=False =

	with self ~action ~on_missing=Nothing = with_builtin self action on_missing...
	with self ~action ~on_missing=(Panic.throw (Illegal_State.Error "The Managed_Resource is used after it has been finalized.")) = with_builtin self action on_missing...

	public final synchronized void scheduleFinalization() {
	public final synchronized void scheduleFinalizationOfSystemResources() {

Enhance Managed_Resource to allow implementation of in-memory caches #11577

Are you sure you want to change the base?

Enhance Managed_Resource to allow implementation of in-memory caches #11577

Conversation

JaroslavTulach commented Nov 18, 2024 • edited Loading

Pull Request Description

Checklist

jdunkerley commented Nov 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Akirathan left a comment

Choose a reason for hiding this comment

GregoryTravis commented Nov 19, 2024

GregoryTravis commented Nov 26, 2024 • edited Loading

JaroslavTulach Nov 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

radeusgd left a comment

Choose a reason for hiding this comment

Akirathan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Enhance `Managed_Resource` to allow implementation of in-memory caches #11577

Enhance `Managed_Resource` to allow implementation of in-memory caches #11577

JaroslavTulach commented Nov 18, 2024 •

edited

Loading

jdunkerley commented Nov 18, 2024 •

edited

Loading

GregoryTravis commented Nov 26, 2024 •

edited

Loading

JaroslavTulach Nov 28, 2024 •

edited

Loading