Skip to content

perf: avoid AST traversal to fill the IdToEObjectMap in the rootNode #1085

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

Closed
rubenporras wants to merge 1 commit into from
Closed

perf: avoid AST traversal to fill the IdToEObjectMap in the rootNode #1085

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
131 changes: 118 additions & 13 deletions ...com/avaloq/tools/ddk/xtext/resource/persistence/DirectLinkingResourceStorageLoadable.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,12 @@
import java.io.InputStreamReader;
import java.nio.charset.StandardCharsets;
import java.util.ArrayDeque;
import java.util.ArrayList;
import java.util.Collections;
import java.util.Deque;
import java.util.HashSet;
import java.util.Map;
import java.util.Set;
import java.util.zip.ZipInputStream;

import org.apache.logging.log4j.LogManager;
Expand Down Expand Up @@ -65,16 +68,16 @@
* - count the number of EObjects added to the Resource
*/
private final class EObjectInputStreamExtension extends BinaryResourceImpl.EObjectInputStream {
private int objectCount;
private static final Logger LOGGER = LogManager.getLogger(EObjectInputStreamExtension.class);

private boolean recordObjects;

private final Set<EObject> isContained = new HashSet<>();

private EObjectInputStreamExtension(final InputStream inputStream, final Map<?, ?> options) throws IOException {
super(inputStream, options);
}

public int eObjectCount() {
return objectCount;
}

@Override
public int readCompressedInt() throws IOException {
// HACK! null resource set, to avoid usage of resourceSet's package registry
Expand All @@ -86,14 +89,103 @@
public InternalEObject loadEObject() throws IOException {
final InternalEObject result = super.loadEObject();
handleLoadEObject(result, this);
objectCount++;
return result;
}

private InternalEObject loadContainedObject() throws IOException {
int previousSize = eObjectList.size();

InternalEObject eObject = loadEObject();

int newSize = eObjectList.size();
if (newSize > previousSize && eObjectList.get(previousSize) == eObject) { // NOPMD
isContained.add(eObject);
} else {
recordObjects = false;
}
return eObject;
}

public void loadContainedEObjects(final InternalEList<InternalEObject> internalEObjects) throws IOException {
// Read all the values into an array.
//
int size = readCompressedInt();
InternalEObject[] values = allocateInternalEObjectArray(size);
for (int i = 0; i < size; ++i) {
values[i] = loadContainedObject(); // changed
}
int existingSize = internalEObjects.size();

// If the list is empty, we need to add all the objects,
// otherwise, the reference is bidirectional and the list is at least partially populated.
//
if (existingSize == 0) {
internalEObjectList.setData(size, values);
internalEObjects.addAllUnique(0, internalEObjectList);
} else {
InternalEObject[] existingValues = allocateInternalEObjectArray(existingSize);
internalEObjects.basicToArray(existingValues);
int[] indices = allocateIntArray(existingSize);
int duplicateCount = 0;
LOOP: for (int i = 0; i < size; ++i) {
InternalEObject internalEObject = values[i];
for (int j = 0, count = duplicateCount; j < existingSize; ++j) {

Check warning on line 132 in com.avaloq.tools.ddk.xtext/src/com/avaloq/tools/ddk/xtext/resource/persistence/DirectLinkingResourceStorageLoadable.java

View workflow job for this annotation

GitHub Actions / pmd

Too many control variables in the for statement 

Having a lot of control variables in a 'for' loop makes it harder to see what range of values
the loop iterates over. By default this rule allows a regular 'for' loop with only one variable.

ForLoopVariableCount (Priority: 3, Ruleset: Best Practices)
https://docs.pmd-code.org/pmd-doc-7.7.0/pmd_rules_java_bestpractices.html#forloopvariablecount
InternalEObject existingInternalEObject = existingValues[j];
if (existingInternalEObject == internalEObject) {
if (duplicateCount != count) {
internalEObjects.move(duplicateCount, count);
}
indices[duplicateCount] = i;
++duplicateCount;
existingValues[j] = null;
continue LOOP;
} else if (existingInternalEObject != null) {
++count;

Check warning on line 143 in com.avaloq.tools.ddk.xtext/src/com/avaloq/tools/ddk/xtext/resource/persistence/DirectLinkingResourceStorageLoadable.java

View workflow job for this annotation

GitHub Actions / pmd

Avoid reassigning the loop control variable 'count' 

Reassigning loop variables can lead to hard-to-find bugs. Prevent or limit how these variables can be changed.

In foreach-loops, configured by the `foreachReassign` property:
- `deny`: Report any reassignment of the loop variable in the loop body. _This is the default._
- `allow`: Don't check the loop variable.
- `firstOnly`: Report any reassignments of the loop variable, except as the first statement in the loop body.
            _This is useful if some kind of normalization or clean-up of the value before using is permitted, but any other change of the variable is not._

In for-loops, configured by the `forReassign` property:
- `deny`: Report any reassignment of the control variable in the loop body. _This is the default._
- `allow`: Don't check the control variable.
- `skip`: Report any reassignments of the control variable, except conditional increments/decrements (`++`, `--`, `+=`, `-=`).
            _This prevents accidental reassignments or unconditional increments of the control variable._

AvoidReassigningLoopVariables (Priority: 3, Ruleset: Best Practices)
https://docs.pmd-code.org/pmd-doc-7.7.0/pmd_rules_java_bestpractices.html#avoidreassigningloopvariables
}
}

values[i - duplicateCount] = internalEObject;
}

size -= existingSize;
internalEObjectList.setData(size, values);
internalEObjects.addAllUnique(0, internalEObjectList);
for (int i = 0; i < existingSize; ++i) {
int newPosition = indices[i];
int oldPosition = size + i;
if (newPosition != oldPosition) {
internalEObjects.move(newPosition, oldPosition);
}
}
recycle(existingValues);
recycle(indices);
}
recycle(values);
}

@Override
protected void loadFeatureValue(final InternalEObject internalEObject, final EStructuralFeatureData eStructuralFeatureData) throws IOException {
try {
super.loadFeatureValue(internalEObject, eStructuralFeatureData);
if (recordObjects) {
switch (eStructuralFeatureData.kind) {
case EOBJECT_CONTAINMENT:
case EOBJECT_CONTAINMENT_PROXY_RESOLVING: {
internalEObject.eSet(eStructuralFeatureData.featureID, loadContainedObject());
break;
}
case EOBJECT_CONTAINMENT_LIST:
case EOBJECT_CONTAINMENT_LIST_PROXY_RESOLVING: {
@SuppressWarnings("unchecked")
InternalEList<InternalEObject> internalEList = (InternalEList<InternalEObject>) internalEObject.eGet(eStructuralFeatureData.featureID, false, true);
loadContainedEObjects(internalEList);
break;
}
default:
super.loadFeatureValue(internalEObject, eStructuralFeatureData);
}
} else {
super.loadFeatureValue(internalEObject, eStructuralFeatureData);
}
// CHECKSTYLE:OFF
} catch (Exception e) {
StringBuilder infoMessage = new StringBuilder(100);
Expand All @@ -107,8 +199,7 @@
}
}

@Override
public void loadResource(final Resource resource) throws IOException {
private void loadResource(final Resource resource, final ArrayList<EObject> firstContainerObjects) throws IOException { // NOPMD LooseCoupling
this.resource = resource;
resourceSet = resource.getResourceSet();
URI uri = resource.getURI();
Expand All @@ -123,7 +214,21 @@
}
InternalEObject[] values = allocateInternalEObjectArray(size);
for (int i = 0; i < size; ++i) {
recordObjects = i == 0;
values[i] = loadEObject();
if (i == 0) {
if (recordObjects) {
isContained.add(values[i]);
firstContainerObjects.ensureCapacity(isContained.size());
for (InternalEObject eObject : eObjectList) {
if (isContained.contains(eObject)) {
firstContainerObjects.add(eObject);
}
}
} else {
LOGGER.info("Out of order feature while loading the proxy node model for " + resource.getURI()); //$NON-NLS-1$
}
}
}
internalEObjectList.setData(size, values);
@SuppressWarnings("unchecked")
Expand All @@ -146,7 +251,7 @@

private ResourceLoadMode mode;

private int eObjectCount;
private ArrayList<EObject> objects; // NOPMD LooseCoupling

public DirectLinkingResourceStorageLoadable(final InputStream in, final boolean loadNodeModel, final boolean splitContents, final ITraceSet traceSet) {
super(in, loadNodeModel);
Expand Down Expand Up @@ -271,7 +376,7 @@
case SKIP:
break;
case PROXY:
ProxyCompositeNode.installProxyNodeModel(resource, eObjectCount);
ProxyCompositeNode.installProxyNodeModel(resource, objects);
break;
case LOAD:
readNodeModel(resource, new NonLockingBufferInputStream(zipIn), content);
Expand Down Expand Up @@ -378,8 +483,8 @@
@Override
protected void readContents(final StorageAwareResource resource, final InputStream inputStream) throws IOException {
final EObjectInputStreamExtension in = new EObjectInputStreamExtension(inputStream, Collections.emptyMap());
in.loadResource(resource);
eObjectCount = in.eObjectCount();
objects = new ArrayList<>();
in.loadResource(resource, objects);
}

}
23 changes: 19 additions & 4 deletions ...ols.ddk.xtext/src/com/avaloq/tools/ddk/xtext/resource/persistence/ProxyCompositeNode.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,17 +72,32 @@
* @param expectedEObjectCount
* the expected EObject count of the resource
*/
static void installProxyNodeModel(final Resource resource, final int expectedEObjectCount) {
static void installProxyNodeModel(final Resource resource, final List<EObject> objects) {
if (resource.getContents().isEmpty()) {
return;
}

EObject root = resource.getContents().get(0);

ProxyCompositeNode rootNode = installProxyNodeModel(root);
rootNode.idToEObjectMap = new ArrayList<>(expectedEObjectCount);
fillIdToEObjectMap(root, rootNode.idToEObjectMap);

List<EObject> other = new ArrayList<>();
fillIdToEObjectMap(root, other);
if (!objects.isEmpty() && !other.equals(objects)) {
if (other.size() != objects.size()) {
System.err.println("Differences in size " + resource.getURI() + " " + other.size() + " vs " + objects.size()); //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$

Check failure on line 87 in com.avaloq.tools.ddk.xtext/src/com/avaloq/tools/ddk/xtext/resource/persistence/ProxyCompositeNode.java

View workflow job for this annotation

GitHub Actions / pmd

Usage of System.out/err 

References to System.(out|err).print are usually intended for debugging purposes and can remain in
the codebase even in production code. By using a logger one can enable/disable this behaviour at
will (and by priority) and avoid clogging the Standard out log.

SystemPrintln (Priority: 2, Ruleset: Best Practices)
https://docs.pmd-code.org/pmd-doc-7.7.0/pmd_rules_java_bestpractices.html#systemprintln
} else {
for (int i = 0; i < other.size(); i++) {
if (!other.get(i).equals(objects.get(i))) {
System.err.println("Differences in element" + i); //$NON-NLS-1$

Check failure on line 91 in com.avaloq.tools.ddk.xtext/src/com/avaloq/tools/ddk/xtext/resource/persistence/ProxyCompositeNode.java

View workflow job for this annotation

GitHub Actions / pmd

Usage of System.out/err 

References to System.(out|err).print are usually intended for debugging purposes and can remain in
the codebase even in production code. By using a logger one can enable/disable this behaviour at
will (and by priority) and avoid clogging the Standard out log.

SystemPrintln (Priority: 2, Ruleset: Best Practices)
https://docs.pmd-code.org/pmd-doc-7.7.0/pmd_rules_java_bestpractices.html#systemprintln
}
}
}
}
if (objects.isEmpty()) {
rootNode.idToEObjectMap = other;
} else {
rootNode.idToEObjectMap = objects;
}
if (resource instanceof XtextResource) {
((XtextResource) resource).setParseResult(new ParseResult(root, rootNode, false));
}
Expand Down
Loading