Migrate internal classes to use new resource implementation (ref-point migration 1/10) #1032

tobous · 2020-06-22T13:12:21Z

Migrates the internal logic of Saros/E to use the new resource model introduced in #1023. The UI and filesystem handler logic is migrated separately in follow-up PRs. Outdated javadoc and method names that are not directly related to the changed logic will be updated in a separate clean-up PR at the end.

Most of the changes made in this PR are relatively simple as they just replace the usage of the old resource implementation with the new one and usage of the ResourceAdapterFactory with ResourceConverter.

Reviewing this PR

I would suggest reviewing this PR commit by commit.

Commits

[INTERNAL][E] Migrate FolderActivityConsumer to new resource impl

[INTERNAL][E] Partially migrate CollaborationUtils to new resource impls

Migrates the internal logic of CollaborationUtils to use the new
resource implementations. The logic dealing with the UI and old sharing
assumptions (e.g. that only projects are shared) will be migrated in a
followup commit.

[REFACTOR][E] Replace manual unwrapping with calls to ResourceConverter

[INTERNAL][E] Migrate FileActivityConsumer to new resource impls

[INTERNAL][E] Migrate EditorPool to new resource impls

[FIX][E] Correctly tear down Editor pool mapping

Adjusts EditorPool.removeAllEditors() to correctly tear down the inner
data structures, ensuring that no mapping is held after the call.

[INTERNAL][E] Migrate SharedDocumentProvider to new resource impls

[INTERNAL][E] Use ResourceConverter in EclipsePathFactory

[INTERNAL][E] Migrate EclipseWorkspaceImpl to new resource impls

[INTERNAL][E] Migrate FileContentNotifierBridge to new resource impls

This migration uncovered further issues in the separation between the
plugin and session context. The checksum cache is instantiated inside
the plugin context as it is used by the resource negotiations, whose
factories are also part of the plugin context.

With the new logic, the notifier only works if there is a running
session that has a valid reference point mapping as we otherwise can't
compute Saros file objects for the modified Eclipse files. This is due
to Saros file objects being relative to their reference point.

In a future patch, the separation between the plugin in session context
should be improved by moving the resource negotiation factories to a
separate class located in the session context. This would also allow the
EditorManager to be moved into the session context.

As the checksum cache implementation also can't be used before a valid
reference point mapping is established, it we could also consider
splitting the resource negotiation into a resource mapping negotiation
and then a resource content negotiation to better differentiate between
the two phases and avoid illegal usage of the checksum cache.

To avoid cyclic dependency issues with the Saros session manager, it has
to be injected later on.

[INTERNAL][E] Migrate EclipseCollaborationUtilsImpl

[INTERNAL][STF] Migrate Eclipse PackageExplorerView

Adds a nullcheck for the obtained Eclipse resource object as the new
converter no longer accepts null values.

[INTERNAL][E] Remove EditorAPI.getEditorFile(IEditorPart)

Removes EditorAPI.getEditorFile(IEditorPart). The method only offers
additional logic to convert from Eclipse resource objects to Saros
resource objects. This is not the responsibility of the editor API.

Adds convenience method convertToFile(IFile) to EditorManager. The
method can be used to convert Eclipse file objects into Saros file
objects using the current set of shared reference points and
ResourceConverter.convertToFile(Set, IFile).

Replaces the usage of EditorAPI.getEditorFile(IEditorPart) with calling
EditorAPI.getEditorResource(IEditorPart) and subsequently converting it
using convertToFile(IFile).

[INTERNAL][E] Migrate EditorManager to new resource impls

eclipse/src/saros/editor/EditorPool.java

srossbach · 2020-06-23T15:52:48Z

eclipse/src/saros/filesystem/checksum/FileContentNotifierBridge.java

@@ -51,8 +74,28 @@ public void resourceChanged(IResourceChangeEvent event) {

        final IFile file = delta.getResource().getAdapter(IFile.class);

-        for (IFileContentChangedListener listener : fileContentChangedListeners)
-          listener.fileContentChanged(ResourceAdapterFactory.create(file));
+        final ISarosSession sarosSession = sarosSessionManager.getSession();


So you are basically breaking the cache implementation ... congratulation !

Does the interface states it operates only on SHARED files ? I do not think so.

Yes, with the new logic, the caching only works on shared files. Currently, it is only used for shared files and I could not think of a use case where we would be interested in non-shared files.

The big issue here is that the caching logic is located in the core. As a result, it has to operate on the resource interface. But, with the current implementation, only shared resources are supported. This is due to the nature of reference-point-relative resource implementations. If any container can be a reference point, how am I supposed to chose the correct reference point object to define the resource with?

As a workaround, I could define all resources for the caching logic through project reference points as this would allow for a strictly defined reference point to use for all resources. But this would lead to the weird situation where the caching logic operates on a complete different usage of the resource implementation.

My suggestion (which is also mentioned in the commit message) would be to improve the plugin and context separation and move the caching logic into the session context. This would ensure that it is never called without a running session.

But I don't think this should be done as part of this migration as it is already large enough. And, to be completely honest, I have already been working on my thesis for long enough and I would like to get to the actual writing part, so I am hesitant to increase the scope of my work.

Start a session. Stop it. Modify some files that were part of the former sharing. Start a session again. You now have dirty checksums.. And if the dirty checksums match the remote checksums (which is likely) you will produce inconsistencies.

Well, that is annoying. Hmmm. I guess I will have to deal with this after all.

The other option I mentioned – just having the notifier use project-based reference points) does not work – does not work as the caching logic does not know the concept of "projects" (as it is Eclipse specific).

The only alternative to make the logic work with non-shared files would be making the caching logic in the core abstract and generic. That way, each IDE-implementation could specify the resource representation. This would mean they they would also have to provide a mapping method converting reference points to the specified resource class, but that should not be a problem.

I still think that moving the checksum logic to the session context is the better solution. I will check how much work it is to move it into the session context and then reconsider my options.

I guess moving it into session scope can be pretty annoying if a session crashes for what reason. You should not assume that everyone is using SSDs or 16 Gigabytes of system memory (OS cache).

I would also question the usefulness of the cache in general. If the user were to be working on huge projects, their hardware would have to match anyways as they otherwise could not load the project into the IDE or work on it in any sensible/timely manner.
But I am still a bit torn on this as I do see the utility during a session. Saros already hogs a lot of resources during a session, so actually using up more resources by creating checksums for all shared resources might push it over the edge (i.e. make the system noticeably slow down). On the other hand, the added cache still produces additional overhead during the session by keeping the checksums up-to-date, even if no further resource negotiation is planned, so removing it might benefit the general Saros performance (but not by a whole lot).

This would be a whole different discussion if the cache could also be used by the watchdog during the session as it would then serve a far broader purpose in this case. But this is currently not possible/not an option as the negotiation cache works on the file content on disc and the watchdog works on the editor content. Working on editor content is important for the watchdog to always work on the most current information, so using the content on disc is not an option. And using the editor content for the negotiation cache is not an option either (for now) as the negotiation could contain binary files which can't be opened in an editor. These caches could, however, be unified if we were to introduce the unified content handling (i.e. adding a binary check and always using the editor content for non-binary files, even for the negotiations).

If we want to keep the cache, I still disagree that moving it to the session context removes most of its benefits. I think it is much more likely that more participants are invited during the session than that the user is starting a new session on the same resources. (Yes, the session crashing/timing out is an exception to this, but I don't think this can be categorized as a default/common case.)

Also, as already mentioned, I am unhappy with the cache still running and using up system resources even though there is no Saros session running. In general, this is a very unexpected behavior from the users perspective and, if this were to negatively impact the system performance, could lead to lots of complaints/uninstalls.
Even if this is an extreme case that might not be that common (or even very uncommon), I would still question the benefits of the cache compared to its drawbacks, especially outside of the session. The overhead gets worse with increases in project size and decreases in hardware resources/power, so the users that would most benefit from this cache would also be impacted most negatively by its overhead.

As mentioned in the standups I think it makes sense to move the cache into the session context. It is a tradeoff and not suitable for all cases, but as @tobous mentioned I don't think its good that Saros still updates checksums outside of sessions, burning resources without use. But keeping it inside the session context is good for multiple invites especially for Saros Server.

The cache do not update checksums. It just invalidates existing stored checksums for files that were changed.

The cache do not update checksums. It just invalidates existing stored checksums for files that were changed.

You are right. I misread the code. But it still creates overhead with calculating the path hashes, but it isn't as bad as I thought.

I still think it is at its most useful during the session as I would assume it is more likely to start a session with multiple participants or add participants to a running session (where the cache would still work for all non-modified shared files) than starting another session on the same resources in the same application lifecycle.

Furthermore, starting a new session requires a break in the workflow anyways, so additional time to re-populate the cache should not be that much of an issue in my opinion.

Resolved in #1072.

eclipse/src/saros/filesystem/checksum/FileContentNotifierBridge.java

stefaus · 2020-06-23T17:40:25Z

eclipse/src/saros/filesystem/checksum/FileContentNotifierBridge.java

+        }
+
+        for (IFileContentChangedListener listener : fileContentChangedListeners) {
+          listener.fileContentChanged(fileWrapper);


problem was here before, but a try-catch would be nice

I am not quite sure what you mean. I try-catch for what?

In case some listener throws an exception every listener afterwards would not be notified.

Ok, and what "exception" should I catch in particular? A specific exceptions type? All checked exceptions? Runtime exceptions? Errors? Everything?

I guess RuntimeException. In fileContentChanged the method create128BitMurmur3Hash throws a RuntimeException.
However, if we want to catch this exception I would prefer to catch it in fileContentChanged and (probably) use a checked exception.

I don't think this is something that should be resolved as part of this PR. As far as I can tell, this issue applies to most (if not all) of our listener implementations.

In general, the question is how we want to handle unexpected exceptions. Is it worth it still informing other listeners, i.e. do we think that the plugin is still in an operable state where informing the other listeners servers any purpose?

Sorry for my late response, to clarify:
There are many code places not handling this, but its a good defensive programming approach to ignore broken listeners without breaking the rest. Thats why it should be catched here and not in the listener.

saros/core/src/saros/net/xmpp/XMPPConnectionService.java

Lines 389 to 395 in 0120f79

for (IConnectionListener listener : listeners) {

try {

listener.connectionStateChanged(connection, state);

} catch (Exception e) {

log.error("internal error in listener: " + listener, e);

}

}

As I think @srossbach and myself where mentioning this in other reviews, I guessed this was already common sense. I think it is especially a good practice if something important happens after this block (which could be added in the future).

@tobous This is still unresolved. Otherwise, I would approve the PR. What is your opinion?

tobous · 2020-06-24T21:07:09Z

Rebased onto current base branch without any interaction.

tobous · 2020-06-24T23:29:46Z

Adjusted EditorPool according to review comments.

eclipse/src/saros/editor/EditorManager.java

m273d15 · 2020-06-24T09:38:34Z

eclipse/src/saros/filesystem/EclipseWorkspaceImpl.java

@@ -81,7 +82,11 @@ public void run(IWorkspaceRunnable runnable, IResource[] resources)
        new EclipseRunnableAdapter(runnable);

    final List<org.eclipse.core.resources.IResource> eclipseResources =
-        ResourceAdapterFactory.convertBack(resources != null ? Arrays.asList(resources) : null);
+        resources == null
+            ? null


You are already checking for null. I would prefer to return an empty list.

Suggested change

? null

? Collections.emptyList()

With this return value you could remove the additional null check in l.93.

Will change it. I generally tried to keep the changes to a minimum. The logic as it is (including the double null-check) was already present in the previous implementation.

m273d15 · 2020-07-07T12:41:43Z

eclipse/src/saros/filesystem/checksum/FileContentNotifierBridge.java

+        }
+
+        for (IFileContentChangedListener listener : fileContentChangedListeners) {
+          listener.fileContentChanged(fileWrapper);


I guess RuntimeException. In fileContentChanged the method create128BitMurmur3Hash throws a RuntimeException.
However, if we want to catch this exception I would prefer to catch it in fileContentChanged and (probably) use a checked exception.

m273d15 · 2020-07-07T13:29:50Z

eclipse/src/saros/filesystem/checksum/FileContentNotifierBridge.java

@@ -51,8 +74,28 @@ public void resourceChanged(IResourceChangeEvent event) {

        final IFile file = delta.getResource().getAdapter(IFile.class);

-        for (IFileContentChangedListener listener : fileContentChangedListeners)
-          listener.fileContentChanged(ResourceAdapterFactory.create(file));
+        final ISarosSession sarosSession = sarosSessionManager.getSession();


I think moving the cache in the session context reduces the benefit and leads to the question "Do we need this cache?".

@srossbach Do you know the history of this cache (who requested the optimization and why)?

If we want to/should keep the cache I think we should use the second alternative (make the cache generic; even if I would like to remove the circular dependency). Otherwise, the benefit seems to be too small.

srossbach · 2020-07-07T21:17:57Z

I agree. If you move the cache into the session context you can discard the whole logic.

I added this cache to increase the speedup when working over time, e.g you invite someone later at a given point or just stop the session and continue somewhere in the future.

I do not know how bad the speed lost would be as there are many factors (beside from CPU load which will be high if you remove the cache) like are you using an SSD or and HDD (notebook hdd are very bad) and how much memory the OS will use for file caching. From my experiences developer PC of a company are always to old and always lack memory as the IDE isn't the only program that is currently running, so expect a memory usage of about 80-90 unless you are blessed with 16 GByte and more. And last but not least do not forget the famous Antivirus-Realtime protection.

I guess you have to play around with realistic project sizes and "normal developer PCs" to decide if the cache is still useful or not.

tobous · 2020-08-04T03:16:40Z

Rebased onto current master without any interactions.

tobous · 2020-08-04T03:22:01Z

Added try catch block for FileContentNotificationBridge listener notification.

srossbach · 2020-08-04T21:11:05Z

eclipse/src/saros/filesystem/checksum/FileContentNotifierBridge.java

+        for (IFileContentChangedListener listener : fileContentChangedListeners) {
+          try {
+            listener.fileContentChanged(fileWrapper);
+          } catch (Exception e) {


Just RuntimeException

srossbach · 2020-08-04T21:16:41Z

eclipse/src/saros/editor/EditorManager.java

+   * @throws NullPointerException if the given file delegate is <code>null</code>
+   * @see ResourceConverter#convertToFile(Set, IFile)
+   */
+  private saros.filesystem.IFile convertToFile(IFile fileDelegate) {


I have seen the logic 3 times in the whole patch set. Any reason not to move the logic to the ResourceConverter and adding (a) method(s) that accepts a session object and an IResource (IFile, IFolder) object ?

All this method does is check whether the given session object is null, request the currently shared reference points and call ResourceConverter.convertToFile(...) with it.

In most other cases, the check whether the session object is valid is already done beforehand by different logic. In general, I don't think it is necessary to introduce a separate method for this in the ResourceConverter.

Regardless, this is a very minor change that can also be done after the fact, so I don't think it should hold back this PR.

tobous · 2020-08-11T06:04:58Z

Rebased onto current master without any interactions.

Caching fix moved to PR #1072

Migrates the internal logic of CollaborationUtils to use the new resource implementations. The logic dealing with the UI and old sharing assumptions (e.g. that only projects are shared) will be migrated in a followup commit.

Adjusts EditorPool.removeAllEditors() to correctly tear down the inner data structures, ensuring that no mapping is held after the call.

This migration uncovered further issues in the separation between the plugin and session context. The checksum cache is instantiated inside the plugin context as it is used by the resource negotiations, whose factories are also part of the plugin context. With the new logic, the notifier only works if there is a running session that has a valid reference point mapping as we otherwise can't compute Saros file objects for the modified Eclipse files. This is due to Saros file objects being relative to their reference point. In a future patch, the separation between the plugin in session context should be improved by moving the resource negotiation factories to a separate class located in the session context. This would also allow the EditorManager to be moved into the session context. As the checksum cache implementation also can't be used before a valid reference point mapping is established, it we could also consider splitting the resource negotiation into a resource mapping negotiation and then a resource content negotiation to better differentiate between the two phases and avoid illegal usage of the checksum cache. To avoid cyclic dependency issues with the Saros session manager, it has to be injected later on.

Adds a nullcheck for the obtained Eclipse resource object as the new converter no longer accepts null values.

Removes EditorAPI.getEditorFile(IEditorPart). The method only offers additional logic to convert from Eclipse resource objects to Saros resource objects. This is not the responsibility of the editor API. Adds convenience method convertToFile(IFile) to EditorManager. The method can be used to convert Eclipse file objects into Saros file objects using the current set of shared reference points and ResourceConverter.convertToFile(Set, IFile). Replaces the usage of EditorAPI.getEditorFile(IEditorPart) with calling EditorAPI.getEditorResource(IEditorPart) and subsequently converting it using convertToFile(IFile).

Moves the resolution of the reference point resource out of EditorPool. Instead adjusts add(...) to get the Saros IFile object to use as an argument.

tobous · 2020-10-19T09:32:42Z

Rebased onto current master without any interactions.

saros-infrastructure · 2020-10-19T09:36:43Z

Here is an overview of what got changed by this pull request:

Complexity increasing per file
==============================
- stf/src/saros/stf/server/rmi/superbot/component/view/eclipse/impl/PackageExplorerView.java  1
- eclipse/src/saros/filesystem/checksum/FileContentNotifierBridge.java  8
- eclipse/src/saros/util/EclipseCollaborationUtilsImpl.java  1

See the complete overview on Codacy

tobous added Area: Eclipse Issue affecting Saros for Eclipse (Saros/E) Breaks Compatibility Change breaking compatibility with previous releases labels Jun 22, 2020

tobous added this to the Saros/E Release NEXT milestone Jun 22, 2020

tobous self-assigned this Jun 22, 2020

srossbach previously requested changes Jun 23, 2020

View reviewed changes

stefaus reviewed Jun 24, 2020

View reviewed changes

tobous force-pushed the pr/eclipse/introduce-new-ref-point-resource-impl branch from a4104ae to 2f83cce Compare June 24, 2020 21:05

tobous force-pushed the pr/eclipse/ref-point-migration/1/inner-classes branch from e058483 to e17e5b6 Compare June 24, 2020 21:06

tobous force-pushed the pr/eclipse/ref-point-migration/1/inner-classes branch from e17e5b6 to a82c9fa Compare June 24, 2020 23:26

srossbach reviewed Jul 1, 2020

View reviewed changes

eclipse/src/saros/editor/EditorManager.java Show resolved Hide resolved

tobous requested a review from m273d15 July 2, 2020 19:19

m273d15 reviewed Jul 7, 2020

View reviewed changes

tobous force-pushed the pr/eclipse/introduce-new-ref-point-resource-impl branch from 2f83cce to 5172f51 Compare August 4, 2020 03:06

Base automatically changed from pr/eclipse/introduce-new-ref-point-resource-impl to master August 4, 2020 03:14

tobous force-pushed the pr/eclipse/ref-point-migration/1/inner-classes branch from a82c9fa to 75cabf4 Compare August 4, 2020 03:16

tobous force-pushed the pr/eclipse/ref-point-migration/1/inner-classes branch from 75cabf4 to 3e19f55 Compare August 4, 2020 03:20

tobous requested review from m273d15, stefaus and srossbach August 4, 2020 03:21

m273d15 previously approved these changes Aug 4, 2020

View reviewed changes

srossbach reviewed Aug 4, 2020

View reviewed changes

tobous dismissed m273d15’s stale review via cf26737 August 11, 2020 05:50

tobous force-pushed the pr/eclipse/ref-point-migration/1/inner-classes branch 2 times, most recently from cf26737 to a9b4bb8 Compare August 11, 2020 06:04

tobous requested a review from srossbach August 11, 2020 06:04

m273d15 previously approved these changes Aug 13, 2020

View reviewed changes

stefaus previously approved these changes Aug 31, 2020

View reviewed changes

tobous added 15 commits October 19, 2020 11:24

[INTERNAL][E] Migrate FolderActivityConsumer to new resource impl

d04f3e7

[REFACTOR][E] Replace manual unwrapping with calls to ResourceConverter

dcca501

[INTERNAL][E] Migrate FileActivityConsumer to new resource impls

6d0137b

[INTERNAL][E] Migrate EditorPool to new resource impls

7889568

[FIX][E] Correctly tear down Editor pool mapping

47c959b

Adjusts EditorPool.removeAllEditors() to correctly tear down the inner data structures, ensuring that no mapping is held after the call.

[INTERNAL][E] Migrate SharedDocumentProvider to new resource impls

1f3101c

[INTERNAL][E] Use ResourceConverter in EclipsePathFactory

581fd48

[INTERNAL][E] Migrate EclipseWorkspaceImpl to new resource impls

d790356

[INTERNAL][E] Migrate EclipseCollaborationUtilsImpl

75d30e9

[INTERNAL][STF] Migrate Eclipse PackageExplorerView

8d790b4

Adds a nullcheck for the obtained Eclipse resource object as the new converter no longer accepts null values.

[INTERNAL][E] Migrate EditorManager to new resource impls

bfc478a

[INTERNAL][E] Clean up EditorPool.add(...)

51a8b06

Moves the resolution of the reference point resource out of EditorPool. Instead adjusts add(...) to get the Saros IFile object to use as an argument.

tobous dismissed stale reviews from stefaus and m273d15 via 51a8b06 October 19, 2020 09:32

tobous force-pushed the pr/eclipse/ref-point-migration/1/inner-classes branch from a9b4bb8 to 51a8b06 Compare October 19, 2020 09:32

tobous merged commit 94f8670 into master Oct 21, 2020

tobous deleted the pr/eclipse/ref-point-migration/1/inner-classes branch October 21, 2020 18:14

	for (IConnectionListener listener : listeners) {
	try {
	listener.connectionStateChanged(connection, state);
	} catch (Exception e) {
	log.error("internal error in listener: " + listener, e);
	}
	}

Migrate internal classes to use new resource implementation (ref-point migration 1/10) #1032

Migrate internal classes to use new resource implementation (ref-point migration 1/10) #1032

Conversation

tobous commented Jun 22, 2020 • edited Loading

Reviewing this PR

Commits

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

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

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

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

tobous commented Jun 24, 2020

tobous commented Jun 24, 2020

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

srossbach commented Jul 7, 2020

tobous commented Aug 4, 2020

tobous commented Aug 4, 2020

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

tobous commented Aug 11, 2020

tobous commented Oct 19, 2020

saros-infrastructure commented Oct 19, 2020

tobous commented Jun 22, 2020 •

edited

Loading