Update the marker in BasicMarkerUpdater::updateMarker in one operation

[fedejeanne]

What it does

The current implementation of BasicMarkerUpdater::updateMarker triggers up to 3 Workspace-Operations to change a marker in a class when saving, those are:

• One operation to change the line of the marker
• One operation to change where it begins
• One operation to change where it ends

This PR lets BasicMarkerUpdater::updateMarker gather all changes that need to be done to a marker in a Map and then apply all those changes in one single Workspace-Operation. This reduces the unnecessary overhead of opening/closing operations and all that it implies like looking for cycles in the OrderedLock class and especially reducing the internal graph used to search for them (DeadlockDetector::reduceGraph).

Contributes to #983

How to test

• Open a Java class
• If the class does not have any markers, create some e.g. call a non-existing method
• Make some changes in the class
• Save --> Everything should work as usual:
  • The existing markers of Errors/Warnings in the class should be updated in the Problems view if their position (line) has changed
  • A build should be triggered (if the auto-build is enabled)
  • etc

[github-actions]

Test Results

     852 files  ±0       852 suites  ±0   1h 16m 59s ⏱️ + 4m 48s
  7 207 tests ±0    7 057 ✔️ +1  149 💤 ±0  1 ❌  - 1 
22 767 runs  ±0  22 279 ✔️ +1  487 💤 ±0  1 ❌  - 1 

For more details on these failures, see this check.

Results for commit 4fee33f. ± Comparison against base commit 1bd79e4.

[iloveeclipse]

Note, build failure is due #981.

[iloveeclipse]

Good catch.

[fedejeanne]

Good catch.

Thank you very much :-)

Maybe I can pick your brain regarding a possible further optimization (in another PR)?
I noticed that after my optimization, a class with X markers will still create up to X Workspace-Operations. Wouldn't it make sense to do at most 1 operation and update all markers in the class at once? If I'm reading the code correctly, this shouldn't have any impact since all WorkspaceOperation::endOperation does is broadcasting a post-change (via broadcastPostChange), firing up a build (if necessary) and doing a snapshot (if necessary).

Disclaimer: I haven't done the described optimization in this PR because it would require an API change and I wanted to keep this PR as small as possible while providing a quick win.

[iloveeclipse]

Wouldn't it make sense to do at most 1 operation and update all markers in the class at once

Yes, but we should discuss this on a different PR :)

[DenisUngemach]

Hello,

this change causes a severe error in the breakpoint markers.

Since now the method markers. setAttributes(Map<String, ? extends Object> attributes) will be used, all existing markers will be deleted and only the 3 markers char_start, char_end and line_number are set.

The following bug comes up:

In eclipse:
-> In a java class, set a breakpoint.
-> one line before the breakpoint add new lines with enter =>, the breakpoint moves down with its selected line
-> Now save the editor => the breakpoint will be disabled!

I recommend to create and use a MarkerUtilities method which updates the marker instead of the marker.setAttributes(attributesChanged) call.

An example see below.

Best regards,
Denis Ungemach

Put this method in MarkerUtilities:

/**
 * Changes the given attribute key-value pairs of the map on this marker. The values must be
 * <code>null</code> or an instance of one of the following classes: <code>String</code>,
 * <code>Integer</code>, or <code>Boolean</code>. If a value is <code>null</code>, the new value
 * of the attribute is considered to be undefined.
 *
 * <p>
 * The values of the attributes cannot be <code>String</code> whose UTF encoding exceeds 65535
 * bytes. On persistent markers this limit is enforced by an assertion.
 * </p>
 *
 * <p>
 * This method changes resources; these changes will be reported in a subsequent resource change
 * event, including an indication that this marker has been modified.
 * </p>
 * 
 * @param marker the marker
 * @param attributeChanges map with to be executed attribute changes
 * @see IMarker#setAttributes(String[], Object[])
 */
public static void changeAttributes(IMarker marker, Map<String, Object> attributeChanges) {

	Set<Entry<String, Object>> entries= attributeChanges.entrySet();
	int size= entries.size();

	String[] keys= new String[size];
	Object[] values= new Object[size];

	int i= 0;
	for (Entry<String, Object> e : entries) {
		keys[i]= e.getKey();
		values[i]= e.getValue();
		i++;
	}

	try {
		marker.setAttributes(keys, values);
	} catch (CoreException e) {
		handleCoreException(e);
	}

}

[laeubi]

@DenisUngemach do you like to propose a PR?

[iloveeclipse]

@DenisUngemach : thanks and sorry, I've overlooked that completely.

I've set a PR: #994

[DenisUngemach]

@iloveeclipse @fedejeanne Thanks for the fix and the performance optimizations! A batch marker update also sounds good.