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.

Sign up for GitHub

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 context lock removal #10273

Merged
merged 6 commits into from
Jul 16, 2024
Merged

Fix context lock removal #10273

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1b9444d
Fix context lock removal
hubertp Jun 13, 2024
f0449e4
Merge branch 'develop' into wip/hubert/10254-lock-removal
hubertp Jul 4, 2024
1c5ee6f
Merge branch 'develop' into wip/hubert/10254-lock-removal
hubertp Jul 15, 2024
380e02a
address review
hubertp Jul 15, 2024
e03c48b
Add missing file
hubertp Jul 15, 2024
c8b5111
Hiding ContextLock internals from other than ReentrantLocking classes
JaroslavTulach Jul 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion ...nt-common/src/main/scala/org/enso/interpreter/instrument/execution/ReentrantLocking.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,7 +248,10 @@ class ReentrantLocking(logger: TruffleLogger) extends Locking {
contextMapLock.lock()
try {
if (contextLocks.contains(contextId)) {
contextLocks(contextId).unlock()
assertNotLocked(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand the desired behavior. Why do we have removeContextLock at all? Because of this line? Then just remove it!

Copy link
Collaborator Author

@hubertp hubertp Jul 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I suppose it was added to avoid creating new instances of ReentrantLock. Once the lock is created it is re-used. It saves roughly 10ms per command (as that particular lock is required quite often).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everywhere, where ctx.locking.withContextLock( is used, there is an instance of RuntimeContext. To

avoid creating new instances of ReentrantLock

the RuntimeContext shall obtain some object instance via ctx.locking.createContextLock and then use it via ctx.locking.withContextLock( until it is needed. At the end it should close it (removeContextLock).

Copy link
Member

@JaroslavTulach JaroslavTulach Jul 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possible API could look like:

diff --git engine/runtime-instrument-common/src/main/java/org/enso/interpreter/instrument/execution/Locking.java engine/runtime-instrument-common/src/main/java/org/enso/interpreter/instrument/execution/Locking.java
index fc6e3f6389..b223ce34fb 100644
--- engine/runtime-instrument-common/src/main/java/org/enso/interpreter/instrument/execution/Locking.java
+++ engine/runtime-instrument-common/src/main/java/org/enso/interpreter/instrument/execution/Locking.java
@@ -37,21 +37,25 @@ public interface Locking {
   <T> T withPendingEditsLock(Class<?> where, Callable<T> callable);
 
   /**
-   * Executes `callable` while holding a context lock
+   * Allocates new handle for use in {@link #withContextLock} associated with given context ID.
    *
-   * @param contextId context id for which the lock is being requested
-   * @param where the class requesting the lock
-   * @param callable code to be executed while holding the lock
-   * @return the result of calling `callable` or null, if no result is expected
+   * @param contextId context ID
+   * @return an object to pass to {@link #withContextLock} and to {@link AutoCloseable#close()} when
+   *     no longer needed
    */
-  <T> T withContextLock(UUID contextId, Class<?> where, Callable<T> callable);
+  AutoCloseable createContextLock(UUID contextId);
 
   /**
-   * Removes a context lock.
+   * Executes `callable` while holding a context lock
    *
-   * @param contextId a context to remove
+   * @param contextLockHandle handle created by {@link #createContextLock(UUID)}
+   * @param where the class requesting the lock
+   * @param callable code to be executed while holding the lock
+   * @return the result of calling `callable` or null, if no result is expected
+   * @throws ClassCastException if {@code contextLockHandle} isn't coming from {@link
+   *     #createContextLock(UUID)}
    */
-  void removeContextLock(UUID contextId);
+  <T> T withContextLock(AutoCloseable contextLockHandle, Class<?> where, Callable<T> callable);
 
   /**
    * Executes `callable` while holding a file lock

More advanced tricks with generics (to hide ClassCastException from the docs) are possible, but probably unnecessary.

contextLocks(contextId),
s"Cannot remove context ${contextId} lock when having a lock on it"
)
contextLocks -= contextId
}
} finally {
Expand Down
Loading