From 1812e1cfe81e1e5772162ade6368c6e9f9b9d758 Mon Sep 17 00:00:00 2001 From: odisseus Date: Wed, 4 Dec 2024 12:06:24 +0100 Subject: [PATCH 1/5] Request sync of particular files --- .../qsync/QuerySyncAsyncFileListener.java | 10 +++- .../qsync/QuerySyncAsyncFileListenerTest.java | 57 ++++++++++++------- 2 files changed, 44 insertions(+), 23 deletions(-) diff --git a/base/src/com/google/idea/blaze/base/qsync/QuerySyncAsyncFileListener.java b/base/src/com/google/idea/blaze/base/qsync/QuerySyncAsyncFileListener.java index 14839a4bcea..8bd9cb86e61 100644 --- a/base/src/com/google/idea/blaze/base/qsync/QuerySyncAsyncFileListener.java +++ b/base/src/com/google/idea/blaze/base/qsync/QuerySyncAsyncFileListener.java @@ -38,6 +38,7 @@ import com.intellij.openapi.vfs.newvfs.events.VFilePropertyChangeEvent; import com.intellij.ui.EditorNotifications; import java.nio.file.Path; +import java.util.Collection; import java.util.List; import java.util.Optional; import java.util.concurrent.atomic.AtomicBoolean; @@ -119,7 +120,8 @@ public void afterVfsChange() { } if (syncOnFileChanges()) { - syncRequester.requestSync(); + syncRequester.requestSync(eventsRequiringSync.stream() + .map(VFileEvent::getFile).toList()); } EditorNotifications.getInstance(project).updateAllNotifications(); }); @@ -154,7 +156,7 @@ private boolean requiresSync(VFileEvent event) { /** Interface for requesting project syncs. */ public interface SyncRequester { - void requestSync(); + void requestSync(Collection files); } /** @@ -192,7 +194,8 @@ public void afterQuerySync(Project project, BlazeContext context) { } @Override - public void requestSync() { + public void requestSync(Collection files) { + // FIXME use the files arg if (changePending.compareAndSet(false, true)) { if (!BlazeSyncStatus.getInstance(project).syncInProgress()) { requestSyncInternal(); @@ -204,6 +207,7 @@ private void requestSyncInternal() { QuerySyncManager.getInstance(project) .deltaSync( QuerySyncActionStatsScope.create(QuerySyncAsyncFileListener.class, null), + //QuerySyncActionStatsScope.createForFiles() TaskOrigin.AUTOMATIC); changePending.set(false); } diff --git a/base/tests/integrationtests/com/google/idea/blaze/base/qsync/QuerySyncAsyncFileListenerTest.java b/base/tests/integrationtests/com/google/idea/blaze/base/qsync/QuerySyncAsyncFileListenerTest.java index 97cb187bc86..08b7c0e9799 100644 --- a/base/tests/integrationtests/com/google/idea/blaze/base/qsync/QuerySyncAsyncFileListenerTest.java +++ b/base/tests/integrationtests/com/google/idea/blaze/base/qsync/QuerySyncAsyncFileListenerTest.java @@ -17,6 +17,7 @@ import static com.google.common.truth.Truth.assertThat; import static java.nio.charset.StandardCharsets.UTF_8; +import static org.mockito.ArgumentMatchers.*; import static org.mockito.Mockito.atLeastOnce; import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; @@ -32,10 +33,14 @@ import com.intellij.testFramework.fixtures.LightJavaCodeInsightFixtureTestCase4; import com.intellij.testFramework.PlatformTestUtil; import java.nio.file.Path; +import java.util.Arrays; +import java.util.Collection; + import org.junit.Rule; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; +import org.mockito.ArgumentMatcher; import org.mockito.Mock; import org.mockito.junit.MockitoJUnit; import org.mockito.junit.MockitoRule; @@ -61,7 +66,7 @@ public void projectFileAdded_requestsSync() { .setAutoSync(true); VirtualFileManager.getInstance() .addAsyncFileListener(fileListener, getFixture().getTestRootDisposable()); - verify(mockSyncRequester, never()).requestSync(); + verify(mockSyncRequester, never()).requestSync(any()); getFixture() .addFileToProject( @@ -69,7 +74,7 @@ public void projectFileAdded_requestsSync() { "package com.example;public class Class1{}"); ApplicationManager.getApplication() .invokeAndWait(PlatformTestUtil::dispatchAllEventsInIdeEventQueue); - verify(mockSyncRequester, atLeastOnce()).requestSync(); + verify(mockSyncRequester, atLeastOnce()).requestSync(argThat(hasFiles("/src/my/project"))); } @Test @@ -87,7 +92,7 @@ public void projectFileAdded_autoSyncDisabled_neverRequestsSync() { "package com.example;public class Class1{}"); ApplicationManager.getApplication() .invokeAndWait(PlatformTestUtil::dispatchAllEventsInIdeEventQueue); - verify(mockSyncRequester, never()).requestSync(); + verify(mockSyncRequester, never()).requestSync(any()); } @Test @@ -98,14 +103,14 @@ public void nonProjectFileAdded_neverRequestsSync() { .setAutoSync(true); VirtualFileManager.getInstance() .addAsyncFileListener(fileListener, getFixture().getTestRootDisposable()); - verify(mockSyncRequester, never()).requestSync(); + verify(mockSyncRequester, never()).requestSync(any()); getFixture() .addFileToProject( "some/other/path/Class1.java", "package some.other.path;public class Class1{}"); ApplicationManager.getApplication() .invokeAndWait(PlatformTestUtil::dispatchAllEventsInIdeEventQueue); - verify(mockSyncRequester, never()).requestSync(); + verify(mockSyncRequester, never()).requestSync(any()); } @Test @@ -124,17 +129,15 @@ public void projectFileMoved_requestsSync() throws Exception { .setAutoSync(true); VirtualFileManager.getInstance() .addAsyncFileListener(fileListener, getFixture().getTestRootDisposable()); - verify(mockSyncRequester, never()).requestSync(); + verify(mockSyncRequester, never()).requestSync(any()); + String sourcePath = INCLUDED_DIRECTORY.resolve("java/com/example/Class1.java").toString(); + String destinationPath = INCLUDED_DIRECTORY.resolve("submodule/java/com/example").toString(); WriteAction.runAndWait( - () -> - getFixture() - .moveFile( - INCLUDED_DIRECTORY.resolve("java/com/example/Class1.java").toString(), - INCLUDED_DIRECTORY.resolve("submodule/java/com/example").toString())); + () -> getFixture().moveFile(sourcePath, destinationPath)); ApplicationManager.getApplication() .invokeAndWait(PlatformTestUtil::dispatchAllEventsInIdeEventQueue); - verify(mockSyncRequester, atLeastOnce()).requestSync(); + verify(mockSyncRequester, atLeastOnce()).requestSync(argThat(hasFiles("/src/" + destinationPath + "/Class1.java"))); } @Test @@ -150,7 +153,7 @@ public void projectFileModified_nonBuildFile_doesNotRequestSync() throws Excepti .setAutoSync(true); VirtualFileManager.getInstance() .addAsyncFileListener(fileListener, getFixture().getTestRootDisposable()); - verify(mockSyncRequester, never()).requestSync(); + verify(mockSyncRequester, never()).requestSync(any()); VirtualFile vf = getFixture() @@ -163,15 +166,14 @@ public void projectFileModified_nonBuildFile_doesNotRequestSync() throws Excepti ApplicationManager.getApplication() .invokeAndWait(PlatformTestUtil::dispatchAllEventsInIdeEventQueue); - verify(mockSyncRequester, never()).requestSync(); + verify(mockSyncRequester, never()).requestSync(any()); assertThat(fileListener.hasModifiedBuildFiles()).isFalse(); } @Test public void projectFileModified_buildFile_requestsSync() throws Exception { - getFixture() - .addFileToProject( - INCLUDED_DIRECTORY.resolve("BUILD").toString(), "java_library(name=\"java\",srcs=[])"); + String buildFilePath = INCLUDED_DIRECTORY.resolve("BUILD").toString(); + getFixture().addFileToProject(buildFilePath, "java_library(name=\"java\",srcs=[])"); TestListener fileListener = new TestListener(getFixture().getProject(), mockSyncRequester) @@ -179,9 +181,9 @@ public void projectFileModified_buildFile_requestsSync() throws Exception { .setAutoSync(true); VirtualFileManager.getInstance() .addAsyncFileListener(fileListener, getFixture().getTestRootDisposable()); - verify(mockSyncRequester, never()).requestSync(); + verify(mockSyncRequester, never()).requestSync(any()); - VirtualFile vf = getFixture().findFileInTempDir(INCLUDED_DIRECTORY.resolve("BUILD").toString()); + VirtualFile vf = getFixture().findFileInTempDir(buildFilePath); WriteAction.runAndWait( () -> vf.setBinaryContent( @@ -189,10 +191,25 @@ public void projectFileModified_buildFile_requestsSync() throws Exception { ApplicationManager.getApplication() .invokeAndWait(PlatformTestUtil::dispatchAllEventsInIdeEventQueue); - verify(mockSyncRequester, atLeastOnce()).requestSync(); + verify(mockSyncRequester, atLeastOnce()).requestSync(argThat(hasFiles("/src/" + buildFilePath))); assertThat(fileListener.hasModifiedBuildFiles()).isTrue(); } + private static ArgumentMatcher> hasFiles(String... paths) { + return new ArgumentMatcher<>() { + @Override + public boolean matches(Collection virtualFiles) { + var actualPaths = virtualFiles.stream().map(VirtualFile::getPath).sorted().toList(); + return actualPaths.equals(Arrays.stream(paths).sorted().toList()); + } + + @Override + public Class type() { + return Collection.class; + } + }; + } + /** * Implementation of {@link QuerySyncAsyncFileListener} for testing. Has a configurable single * project include directory and setting for requesting syncs. From ec50e6cc505f9a36986cb31036cecaabeba0caa5 Mon Sep 17 00:00:00 2001 From: odisseus Date: Thu, 5 Dec 2024 10:07:58 +0100 Subject: [PATCH 2/5] Queue for files that need to be synced --- .../qsync/QuerySyncAsyncFileListener.java | 24 ++++++++++++------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/base/src/com/google/idea/blaze/base/qsync/QuerySyncAsyncFileListener.java b/base/src/com/google/idea/blaze/base/qsync/QuerySyncAsyncFileListener.java index 8bd9cb86e61..d0acfc4b260 100644 --- a/base/src/com/google/idea/blaze/base/qsync/QuerySyncAsyncFileListener.java +++ b/base/src/com/google/idea/blaze/base/qsync/QuerySyncAsyncFileListener.java @@ -18,6 +18,7 @@ import static com.google.common.collect.ImmutableList.toImmutableList; import com.google.common.annotations.VisibleForTesting; +import com.google.common.collect.ImmutableCollection; import com.google.common.collect.ImmutableList; import com.google.idea.blaze.base.lang.buildfile.language.BuildFileType; import com.google.idea.blaze.base.logging.utils.querysync.QuerySyncActionStatsScope; @@ -37,10 +38,13 @@ import com.intellij.openapi.vfs.newvfs.events.VFileMoveEvent; import com.intellij.openapi.vfs.newvfs.events.VFilePropertyChangeEvent; import com.intellij.ui.EditorNotifications; +import org.jetbrains.annotations.NotNull; + import java.nio.file.Path; import java.util.Collection; import java.util.List; import java.util.Optional; +import java.util.concurrent.ConcurrentLinkedQueue; import java.util.concurrent.atomic.AtomicBoolean; import javax.annotation.Nullable; @@ -156,7 +160,7 @@ private boolean requiresSync(VFileEvent event) { /** Interface for requesting project syncs. */ public interface SyncRequester { - void requestSync(Collection files); + void requestSync(@NotNull Collection files); } /** @@ -167,6 +171,7 @@ private static class QueueingSyncRequester implements SyncRequester { private final Project project; private final AtomicBoolean changePending = new AtomicBoolean(false); + private final ConcurrentLinkedQueue unprocessedChanges = new ConcurrentLinkedQueue<>(); public QueueingSyncRequester(Project project) { this.project = project; @@ -185,7 +190,7 @@ public void afterQuerySync(Project project, BlazeContext context) { return; } if (requester.changePending.get()) { - requester.requestSyncInternal(); + requester.requestSyncInternal(ImmutableList.of()); } } }, @@ -194,20 +199,23 @@ public void afterQuerySync(Project project, BlazeContext context) { } @Override - public void requestSync(Collection files) { - // FIXME use the files arg + public void requestSync(@NotNull Collection files) { + unprocessedChanges.addAll(files); if (changePending.compareAndSet(false, true)) { if (!BlazeSyncStatus.getInstance(project).syncInProgress()) { - requestSyncInternal(); + var changesToProcess = ImmutableList.copyOf(unprocessedChanges); + if(!changesToProcess.isEmpty()) { + unprocessedChanges.clear(); + requestSyncInternal(changesToProcess); + } } } } - private void requestSyncInternal() { + private void requestSyncInternal(ImmutableCollection files) { QuerySyncManager.getInstance(project) .deltaSync( - QuerySyncActionStatsScope.create(QuerySyncAsyncFileListener.class, null), - //QuerySyncActionStatsScope.createForFiles() + QuerySyncActionStatsScope.createForFiles(QuerySyncAsyncFileListener.class, null, files), TaskOrigin.AUTOMATIC); changePending.set(false); } From a4d450cb7d40dd4a5c0d381e9a34b4dd1b442c0f Mon Sep 17 00:00:00 2001 From: odisseus Date: Thu, 5 Dec 2024 15:43:41 +0100 Subject: [PATCH 3/5] logging --- .../idea/blaze/base/qsync/QuerySyncAsyncFileListener.java | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/base/src/com/google/idea/blaze/base/qsync/QuerySyncAsyncFileListener.java b/base/src/com/google/idea/blaze/base/qsync/QuerySyncAsyncFileListener.java index d0acfc4b260..ea9ab372059 100644 --- a/base/src/com/google/idea/blaze/base/qsync/QuerySyncAsyncFileListener.java +++ b/base/src/com/google/idea/blaze/base/qsync/QuerySyncAsyncFileListener.java @@ -29,6 +29,7 @@ import com.google.idea.blaze.base.sync.status.BlazeSyncStatus; import com.intellij.openapi.Disposable; import com.intellij.openapi.application.ApplicationManager; +import com.intellij.openapi.diagnostic.Logger; import com.intellij.openapi.project.Project; import com.intellij.openapi.vfs.AsyncFileListener; import com.intellij.openapi.vfs.VirtualFile; @@ -51,6 +52,8 @@ /** {@link AsyncFileListener} for monitoring project changes requiring a re-sync */ public class QuerySyncAsyncFileListener implements AsyncFileListener { + private static final Logger logger = Logger.getInstance(QuerySyncAsyncFileListener.class); + private final Project project; private final SyncRequester syncRequester; @@ -200,6 +203,7 @@ public void afterQuerySync(Project project, BlazeContext context) { @Override public void requestSync(@NotNull Collection files) { + logger.info(String.format("Putting %d files into sync queue", files.size())); unprocessedChanges.addAll(files); if (changePending.compareAndSet(false, true)) { if (!BlazeSyncStatus.getInstance(project).syncInProgress()) { @@ -213,6 +217,7 @@ public void requestSync(@NotNull Collection files) { } private void requestSyncInternal(ImmutableCollection files) { + logger.info(String.format("Requesting sync of %d files", files.size())); QuerySyncManager.getInstance(project) .deltaSync( QuerySyncActionStatsScope.createForFiles(QuerySyncAsyncFileListener.class, null, files), From 564fa09fbf42bfdf09690b3ecac3ff0b3602cf33 Mon Sep 17 00:00:00 2001 From: odisseus Date: Tue, 10 Dec 2024 06:39:54 +0100 Subject: [PATCH 4/5] Synchronised add and clear --- .../base/qsync/QuerySyncAsyncFileListener.java | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/base/src/com/google/idea/blaze/base/qsync/QuerySyncAsyncFileListener.java b/base/src/com/google/idea/blaze/base/qsync/QuerySyncAsyncFileListener.java index ea9ab372059..dd23f2880d8 100644 --- a/base/src/com/google/idea/blaze/base/qsync/QuerySyncAsyncFileListener.java +++ b/base/src/com/google/idea/blaze/base/qsync/QuerySyncAsyncFileListener.java @@ -204,16 +204,19 @@ public void afterQuerySync(Project project, BlazeContext context) { @Override public void requestSync(@NotNull Collection files) { logger.info(String.format("Putting %d files into sync queue", files.size())); - unprocessedChanges.addAll(files); - if (changePending.compareAndSet(false, true)) { - if (!BlazeSyncStatus.getInstance(project).syncInProgress()) { - var changesToProcess = ImmutableList.copyOf(unprocessedChanges); - if(!changesToProcess.isEmpty()) { + ImmutableList changesToProcess = ImmutableList.of(); + synchronized (unprocessedChanges) { + unprocessedChanges.addAll(files); + if (changePending.compareAndSet(false, true)) { + if (!BlazeSyncStatus.getInstance(project).syncInProgress()) { + changesToProcess = ImmutableList.copyOf(unprocessedChanges); unprocessedChanges.clear(); - requestSyncInternal(changesToProcess); } } } + if(!changesToProcess.isEmpty()) { + requestSyncInternal(changesToProcess); + } } private void requestSyncInternal(ImmutableCollection files) { From 7187a8c70478d24a6930b63b4830476816c94e30 Mon Sep 17 00:00:00 2001 From: odisseus Date: Thu, 12 Dec 2024 15:27:34 +0100 Subject: [PATCH 5/5] Unify the logic for requesting sync --- .../blaze/base/qsync/QuerySyncAsyncFileListener.java | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/base/src/com/google/idea/blaze/base/qsync/QuerySyncAsyncFileListener.java b/base/src/com/google/idea/blaze/base/qsync/QuerySyncAsyncFileListener.java index dd23f2880d8..ad67384d257 100644 --- a/base/src/com/google/idea/blaze/base/qsync/QuerySyncAsyncFileListener.java +++ b/base/src/com/google/idea/blaze/base/qsync/QuerySyncAsyncFileListener.java @@ -173,7 +173,6 @@ public interface SyncRequester { private static class QueueingSyncRequester implements SyncRequester { private final Project project; - private final AtomicBoolean changePending = new AtomicBoolean(false); private final ConcurrentLinkedQueue unprocessedChanges = new ConcurrentLinkedQueue<>(); public QueueingSyncRequester(Project project) { @@ -192,9 +191,7 @@ public void afterQuerySync(Project project, BlazeContext context) { if (!requester.project.equals(project)) { return; } - if (requester.changePending.get()) { - requester.requestSyncInternal(ImmutableList.of()); - } + requester.requestSync(ImmutableList.of()); } }, parentDisposable); @@ -206,8 +203,9 @@ public void requestSync(@NotNull Collection files) { logger.info(String.format("Putting %d files into sync queue", files.size())); ImmutableList changesToProcess = ImmutableList.of(); synchronized (unprocessedChanges) { + // TODO aggregate multiple events into one sync request unprocessedChanges.addAll(files); - if (changePending.compareAndSet(false, true)) { + if (!unprocessedChanges.isEmpty()) { if (!BlazeSyncStatus.getInstance(project).syncInProgress()) { changesToProcess = ImmutableList.copyOf(unprocessedChanges); unprocessedChanges.clear(); @@ -220,12 +218,11 @@ public void requestSync(@NotNull Collection files) { } private void requestSyncInternal(ImmutableCollection files) { - logger.info(String.format("Requesting sync of %d files", files.size())); + logger.info(String.format("Requesting sync of files: %s", files)); QuerySyncManager.getInstance(project) .deltaSync( QuerySyncActionStatsScope.createForFiles(QuerySyncAsyncFileListener.class, null, files), TaskOrigin.AUTOMATIC); - changePending.set(false); } }