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

Jump to bottom

Experiment in lock-free module invalidation #9639

Merged
merged 11 commits into from
Apr 16, 2024
Merged

Experiment in lock-free module invalidation #9639

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
0110d86
Experiment in lock-free module invalidation
hubertp Apr 5, 2024
65e901b
Increae pool for background jobs
hubertp Apr 5, 2024
bd590ca
executors with non-empty queue
hubertp Apr 8, 2024
655d9aa
Fix broken logic
hubertp Apr 9, 2024
a65178d
PR Review: Extract indexing logic
hubertp Apr 9, 2024
641d968
Eliminate race-conditions when indexing
hubertp Apr 11, 2024
2d662fd
docs
hubertp Apr 11, 2024
6348cf4
Avoid GC problems
hubertp Apr 11, 2024
e6b6cf2
naming is hard
hubertp Apr 11, 2024
99d6560
Merge branch 'develop' into wip/hubert/lock-free-invalidation
hubertp Apr 15, 2024
7881f9d
Update doc
hubertp Apr 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
16 changes: 3 additions & 13 deletions .../src/main/java/org/enso/interpreter/instrument/command/InvalidateModulesIndexCommand.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,13 +30,13 @@ public Future<BoxedUnit> executeAsynchronously(RuntimeContext ctx, ExecutionCont
return Future.apply(
() -> {
TruffleLogger logger = ctx.executionService().getLogger();
long writeCompilationLockTimestamp = ctx.locking().acquireWriteCompilationLock();
try {
logger.log(Level.FINE, "Invalidating modules, cancelling background jobs");
ctx.jobControlPlane().stopBackgroundJobs();
ctx.jobControlPlane().abortBackgroundJobs(DeserializeLibrarySuggestionsJob.class);

EnsoContext context = ctx.executionService().getContext();
context.getTopScope().getModules().forEach(module -> module.setIndexed(false));
context.getTopScope().getModules().forEach(module -> module.needsIndexing());

context
.getPackageRepository()
Expand All @@ -47,19 +47,9 @@ public Future<BoxedUnit> executeAsynchronously(RuntimeContext ctx, ExecutionCont
.runBackground(new DeserializeLibrarySuggestionsJob(pkg.libraryName()));
return BoxedUnit.UNIT;
});

reply(new Runtime$Api$InvalidateModulesIndexResponse(), ctx);
} finally {
ctx.locking().releaseWriteCompilationLock();
logger.log(
Level.FINEST,
"Kept write compilation lock [{0}] for {1} milliseconds.",
new Object[] {
this.getClass().getSimpleName(),
System.currentTimeMillis() - writeCompilationLockTimestamp
});
reply(new Runtime$Api$InvalidateModulesIndexResponse(), ctx);
}

return BoxedUnit.UNIT;
},
ec);
Expand Down
2 changes: 1 addition & 1 deletion ...ment-common/src/main/scala/org/enso/interpreter/instrument/command/RenameProjectCmd.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ class RenameProjectCmd(
)

projectModules.foreach { module =>
Module.fromCompilerModule(module).setIndexed(false)
Module.fromCompilerModule(module).needsIndexing()
ctx.endpoint.sendToClient(
Api.Response(
Api.SuggestionsDatabaseModuleUpdateNotification(
Expand Down
2 changes: 1 addition & 1 deletion ...-common/src/main/scala/org/enso/interpreter/instrument/execution/JobExecutionEngine.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ final class JobExecutionEngine(
)

private val backgroundJobExecutor: ExecutorService =
context.newFixedThreadPool(1, "background-job-pool", false)
context.newFixedThreadPool(4, "background-job-pool", false)

private val runtimeContext =
RuntimeContext(
Expand Down
10 changes: 8 additions & 2 deletions ...t-common/src/main/scala/org/enso/interpreter/instrument/job/AnalyzeModuleInScopeJob.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ final class AnalyzeModuleInScopeJob(
ctx: RuntimeContext
): Unit = {
if (!module.isIndexed && module.getSource != null) {
module.indexing()
hubertp marked this conversation as resolved.
Show resolved Hide resolved
ctx.executionService.getLogger
.log(Level.FINEST, s"Analyzing module in scope ${module.getName}")
val moduleName = module.getName
Expand All @@ -60,8 +61,13 @@ final class AnalyzeModuleInScopeJob(
exports = ModuleExportsDiff.compute(prevExports, newExports),
updates = SuggestionDiff.compute(Tree.empty, newSuggestions)
)
sendModuleUpdate(notification)
module.setIndexed(true)
if (!module.indexed()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Now I see that because it is effectively the only place that marks the module as indexed, just synchronizing around the indexed flag should be enough 👍

Copy link
Member

@JaroslavTulach JaroslavTulach Apr 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Line 44 is setting an internal module field and leaving synchronized section...

Now we are changing the field again. Meanwhile the state wasn't synchronized. What makes you believe the result of such operation is going to be consistent? Btw.

What is the invariant to be consistent with?

ctx.executionService.getLogger
.log(Level.FINEST, s"Analyzing module in scope ${module.getName}")
analyzeModuleInScope(module);
} else {
sendModuleUpdate(notification)
}
}
}

Expand Down
8 changes: 6 additions & 2 deletions ...strument-common/src/main/scala/org/enso/interpreter/instrument/job/AnalyzeModuleJob.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ object AnalyzeModuleJob {
)
sendModuleUpdate(notification)
} else {
module.indexing()
ctx.executionService.getLogger
.log(Level.FINEST, s"Analyzing not-indexed module ${module.getName}")
val newSuggestions =
Expand All @@ -88,8 +89,11 @@ object AnalyzeModuleJob {
exports = ModuleExportsDiff.compute(prevExports, newExports),
updates = SuggestionDiff.compute(Tree.empty, newSuggestions)
)
sendModuleUpdate(notification)
module.setIndexed(true)
if (module.indexed()) {
sendModuleUpdate(notification)
} else {
doAnalyzeModule(module, changeset)
}
}
}

Expand Down
42 changes: 36 additions & 6 deletions engine/runtime/src/main/java/org/enso/interpreter/runtime/Module.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,15 @@ public final class Module implements EnsoObject {
private final Map<Source, Module> allSources = new WeakHashMap<>();
private final Package<TruffleFile> pkg;
private CompilationStage compilationStage = CompilationStage.INITIAL;
private boolean isIndexed = false;

enum IndexState {
NotIndexed,
NeedsIndexing,
Indexed
}

private IndexState indexState;
Copy link
Member

@JaroslavTulach JaroslavTulach Apr 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Let me assume the point of this IndexState is to have a tri-state status which can be invalidated, scheduled for recomputation and consumed, when it is available. I believe such a concept is best expressed by a Future. If we change the code in Module to just:

private volatile Future<Object> index;

public final void indexing(Future<Object> newIndex) {
  this.index = newIndex; 
}

public final Future<Object> index() {
  return this.index;
}

I'll consider it to be encapsulated enough (for now). @hubertp, is such a future based interface enough for your purposes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't believe that encapsulated the whole logic as it wouldn't express a situation when in-progress indexing would be marked as dirty and require re-indexing. I can be obviously wrong but there was a reason why logic was added to Analyze*Job to deal with this scenario without locking everything, as it was.

Copy link
Member

Choose a reason for hiding this comment

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

I continue to believe Future is the best concept to represent the desired behavior.

it wouldn't express a situation when in-progress indexing would be marked as dirty and require re-indexing

No, (cancellable) Future represents such concepts neatly, I believe.

private Object indexingLock = new Object();
private org.enso.compiler.core.ir.Module ir;
private Map<UUID, IR> uuidsMap;
private QualifiedName name;
Expand Down Expand Up @@ -495,15 +503,37 @@ public void renameProject(String newName) {
}

/**
* @return the indexed flag.
* @return true, if module has been indexed. False otherwise.
*/
public boolean isIndexed() {
return isIndexed;
synchronized (indexingLock) {
hubertp marked this conversation as resolved.
Show resolved Hide resolved
return indexState == IndexState.Indexed;
}
}

/** Marks the module as fully indexed. */
public boolean indexed() {
synchronized (indexingLock) {
if (indexState == IndexState.NeedsIndexing) {
return false;
}
indexState = IndexState.Indexed;
return true;
}
}

/** Marks the module as requiring indexing. */
public void needsIndexing() {
synchronized (indexingLock) {
indexState = IndexState.NeedsIndexing;
}
}

/** Set the indexed flag. */
public void setIndexed(boolean indexed) {
isIndexed = indexed;
/** Marks the module as not indexed, should be set at the beggining of the indexing operation. */
public void indexing() {
hubertp marked this conversation as resolved.
Show resolved Hide resolved
synchronized (indexingLock) {
indexState = IndexState.NotIndexed;
}
}

/**
Expand Down
2 changes: 1 addition & 1 deletion engine/runtime/src/main/java/org/enso/interpreter/runtime/TruffleCompilerContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -609,7 +609,7 @@ private scala.Option<SuggestionsCache.CachedSuggestions> deserializeSuggestionsI
return scala.Option.apply(loaded.get());
} else {
logSerializationManager(
Level.FINE, "Unable to load suggestions for library [{0}].", libraryName);
Level.WARNING, "Unable to load suggestions for library [{0}].", libraryName);
return scala.Option.empty();
}
}
Expand Down
Loading