Skip to main content
aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorAndrew Johnson2019-08-26 02:17:57 -0400
committerAndrew Johnson2019-08-26 02:50:39 -0400
commit11244ade3970e00ed64a7c295b9bb5e6267b2311 (patch)
tree74146b6e7e521af2fc3a95f734a685750197ad1d
parent4e556c1e0bdf5e4f6b09c964f2bc3d4773254457 (diff)
downloadorg.eclipse.mat-11244ade3970e00ed64a7c295b9bb5e6267b2311.tar.gz
org.eclipse.mat-11244ade3970e00ed64a7c295b9bb5e6267b2311.tar.xz
org.eclipse.mat-11244ade3970e00ed64a7c295b9bb5e6267b2311.zip
550167: Multi-threaded object marking can give incorrect results on OOME
Revert GarbageCleaner change to avoid conflicts with parallel parsing changes. Task-Url: https://bugs.eclipse.org/bugs/show_bug.cgi?id=550167 Change-Id: I6e534e20c7ef525e065385b62b0069fc0f4fd715
-rw-r--r--plugins/org.eclipse.mat.parser/src/org/eclipse/mat/parser/internal/Messages.java5
-rw-r--r--plugins/org.eclipse.mat.parser/src/org/eclipse/mat/parser/internal/messages.properties5
-rw-r--r--plugins/org.eclipse.mat.parser/src/org/eclipse/mat/parser/internal/snapshot/ObjectMarker.java269
-rw-r--r--plugins/org.eclipse.mat.tests/src/org/eclipse/mat/tests/CreatePerformanceDump.java102
4 files changed, 304 insertions, 77 deletions
diff --git a/plugins/org.eclipse.mat.parser/src/org/eclipse/mat/parser/internal/Messages.java b/plugins/org.eclipse.mat.parser/src/org/eclipse/mat/parser/internal/Messages.java
index a517c2a5..4af0c8e3 100644
--- a/plugins/org.eclipse.mat.parser/src/org/eclipse/mat/parser/internal/Messages.java
+++ b/plugins/org.eclipse.mat.parser/src/org/eclipse/mat/parser/internal/Messages.java
@@ -1,5 +1,5 @@
/*******************************************************************************
- * Copyright (c) 2009, 2011 SAP AG and IBM Corporation.
+ * Copyright (c) 2009, 2019 SAP AG and IBM Corporation.
* All rights reserved. This program and the accompanying materials
* are made available under the terms of the Eclipse Public License v1.0
* which accompanies this distribution, and is available at
@@ -91,6 +91,9 @@ public class Messages extends NLS
public static String SnapshotImpl_RetrievingDominators;
public static String ObjectArrayImpl_forArray;
public static String ObjectMarker_MarkingObjects;
+ public static String ObjectMarker_ErrorMarkingObjects;
+ public static String ObjectMarker_WarningMarkingObjects;
+ public static String ObjectMarker_ErrorMarkingObjectsSeeLog;
public static String Operation_Error_ArgumentOfUnknownClass;
public static String Operation_Error_CannotCompare;
public static String Operation_Error_NotInArgumentOfUnknownClass;
diff --git a/plugins/org.eclipse.mat.parser/src/org/eclipse/mat/parser/internal/messages.properties b/plugins/org.eclipse.mat.parser/src/org/eclipse/mat/parser/internal/messages.properties
index 11f70fe6..eab5fa0c 100644
--- a/plugins/org.eclipse.mat.parser/src/org/eclipse/mat/parser/internal/messages.properties
+++ b/plugins/org.eclipse.mat.parser/src/org/eclipse/mat/parser/internal/messages.properties
@@ -1,5 +1,5 @@
###############################################################################
-# Copyright (c) 2008, 2011 SAP AG and others.
+# Copyright (c) 2008, 2019 SAP AG and others.
# All rights reserved. This program and the accompanying materials
# are made available under the terms of the Eclipse Public License v1.0
# which accompanies this distribution, and is available at
@@ -84,6 +84,9 @@ SnapshotImpl_ReopeningParsedHeapDumpFile=Reopening parsed heap dump file
SnapshotImpl_RetrievingDominators=Retrieving dominators...
ObjectArrayImpl_forArray={0} for array {1}
ObjectMarker_MarkingObjects=Marking reachable objects
+ObjectMarker_ErrorMarkingObjects=Error marking reachable objects
+ObjectMarker_WarningMarkingObjects=Out of memory error while marking reachable objects, continuing with remaining threads
+ObjectMarker_ErrorMarkingObjectsSeeLog={0} errors marking reachable objects, check the error log for further details
Operation_Error_ArgumentOfUnknownClass=right argument to IN of unknown class {0}
Operation_Error_CannotCompare=IN: cannot compare left argument of type {0} to int[]
Operation_Error_NotInArgumentOfUnknownClass=right argument to NOT IN of unknown class {0}
diff --git a/plugins/org.eclipse.mat.parser/src/org/eclipse/mat/parser/internal/snapshot/ObjectMarker.java b/plugins/org.eclipse.mat.parser/src/org/eclipse/mat/parser/internal/snapshot/ObjectMarker.java
index b69bd06b..73de1cb6 100644
--- a/plugins/org.eclipse.mat.parser/src/org/eclipse/mat/parser/internal/snapshot/ObjectMarker.java
+++ b/plugins/org.eclipse.mat.parser/src/org/eclipse/mat/parser/internal/snapshot/ObjectMarker.java
@@ -1,5 +1,5 @@
/*******************************************************************************
- * Copyright (c) 2008, 2013 SAP AG and IBM Corporation.
+ * Copyright (c) 2008, 2019 SAP AG and IBM Corporation.
* All rights reserved. This program and the accompanying materials
* are made available under the terms of the Eclipse Public License v1.0
* which accompanies this distribution, and is available at
@@ -29,6 +29,8 @@ import org.eclipse.mat.snapshot.ISnapshot;
import org.eclipse.mat.snapshot.model.IObject;
import org.eclipse.mat.snapshot.model.NamedReference;
import org.eclipse.mat.util.IProgressListener;
+import org.eclipse.mat.util.MessageUtil;
+import org.eclipse.mat.util.IProgressListener.Severity;
public class ObjectMarker
{
@@ -360,8 +362,13 @@ public class ObjectMarker
push(z);
++pushed;
if (waitingThreads > 0)
+ {
// All or one?
- notifyAll();
+ if (size() > 1)
+ notifyAll();
+ else
+ notify();
+ }
return true;
}
return false;
@@ -421,14 +428,30 @@ public class ObjectMarker
}
// wait for all the threads to finish
+
+ int failed = 0;
+ Throwable failure = null;
for (int i = 0; i < numberOfThreads; i++)
{
threads[i].join();
+ if (!dfsthreads[i].completed)
+ {
+ ++failed;
+ if (failure == null)
+ {
+ failure = dfsthreads[i].failure;
+ }
+ }
}
if (progressListener.isCanceled())
return;
+ if (failed > 0)
+ {
+ throw new RuntimeException(MessageUtil.format(Messages.ObjectMarker_ErrorMarkingObjectsSeeLog, failed), failure);
+ }
+
progressListener.done();
if (DEBUG) System.out.println("Took "+(System.currentTimeMillis() - l)+"ms"); //$NON-NLS-1$//$NON-NLS-2$
}
@@ -436,9 +459,12 @@ public class ObjectMarker
public class DfsThread implements Runnable
{
+ boolean completed; // normal completion
int size = 0;
int[] data = new int[10 * 1024]; // start with 10k
IntStack rootsStack;
+ int current = -1;
+ Throwable failure; // Exception/Error
public DfsThread(IntStack roots)
{
@@ -447,58 +473,70 @@ public class ObjectMarker
public void run()
{
- while (true)
+ try
{
- synchronized (rootsStack)
+ while (true)
{
- progressListener.worked(1);
- if (progressListener.isCanceled())
- return;
-
- if (rootsStack.size() > 0) // still some roots are not
- // processed
- {
- data[0] = rootsStack.pop();
- size = 1;
- }
- else
- // the work is done
+ synchronized (rootsStack)
{
- break;
- }
- }
-
- int current;
+ progressListener.worked(1);
+ if (progressListener.isCanceled())
+ return;
- while (size > 0)
- {
- /* start stack.pop() */
- current = data[--size];
- /* end stack.pop */
+ if (rootsStack.size() > 0) // still some roots are not
+ // processed
+ {
+ data[0] = rootsStack.pop();
+ size = 1;
+ }
+ else
+ // the work is done
+ {
+ break;
+ }
+ }
- for (int child : outbound.get(current))
+ while (size > 0)
{
- /*
- * No synchronization here. It costs a lot of
- * performance It is possible that some bits are marked
- * more than once, but this is not a problem
- */
- if (!bits[child])
+ /* start stack.pop() */
+ current = data[--size];
+ /* end stack.pop */
+
+ for (int child : outbound.get(current))
{
- bits[child] = true;
- // stack.push(child);
- /* start stack.push() */
- if (size == data.length)
+ /*
+ * No synchronization here. It costs a lot of
+ * performance It is possible that some bits are marked
+ * more than once, but this is not a problem
+ */
+ if (!bits[child])
{
- int[] newArr = new int[data.length << 1];
- System.arraycopy(data, 0, newArr, 0, data.length);
- data = newArr;
+ bits[child] = true;
+ // stack.push(child);
+ /* start stack.push() */
+ if (size == data.length)
+ {
+ int[] newArr = new int[data.length << 1];
+ System.arraycopy(data, 0, newArr, 0, data.length);
+ data = newArr;
+ }
+ data[size++] = child;
+ /* end stack.push() */
}
- data[size++] = child;
- /* end stack.push() */
}
- }
- } // end of processing one GC root
+ } // end of processing one GC root
+ }
+ completed = true;
+ }
+ catch (RuntimeException e)
+ {
+ progressListener.sendUserMessage(Severity.ERROR, Messages.ObjectMarker_ErrorMarkingObjects, e);
+ failure = e;
+ }
+ catch (Error e)
+ {
+ progressListener.sendUserMessage(Severity.ERROR, Messages.ObjectMarker_ErrorMarkingObjects, e);
+ failure = e;
}
}
}
@@ -515,6 +553,8 @@ public class ObjectMarker
static final int MAXSTACK = 100 * 1024;
/** How many times to loop before checking the global stack */
private static final int CHECKCOUNT = 10;
+ /** How many times to loop on outbounds before checking the global stack */
+ private static final int CHECKCOUNT2 = 200;
int localRange;
final int localRangeLimit;
SoftReference<int[]> sr;
@@ -599,8 +639,6 @@ public class ObjectMarker
fillStack();
// Process the local stack and queue
- int current;
-
while (size > 0)
{
/* start stack.pop() */
@@ -611,35 +649,7 @@ public class ObjectMarker
if (check || checkCount++ >= CHECKCOUNT)
{
checkCount = 0;
- check = true;
- if (queue.size() > 0 && queue.size() + size > RESERVED)
- {
- int fromQueue;
- synchronized (rootsStack)
- {
- do
- {
- fromQueue = queue.get();
- check = rootsStack.pushIfWaiting(fromQueue);
- }
- while (check && queue.size() > 0 && queue.size() + size > RESERVED);
- }
- if (!check)
- queue.put(fromQueue);
- }
- else if (size > RESERVED)
- {
- synchronized (rootsStack)
- {
- do
- {
- check = rootsStack.pushIfWaiting(current);
- if (check)
- current = data[--size];
- }
- while (check && size > RESERVED);
- }
- }
+ check = fillRootsStack();
}
// Examine each outbound reference
@@ -654,6 +664,17 @@ public class ObjectMarker
if (!bits[child])
{
bits[child] = true;
+ // See if we have enough work and other threads need more work.
+ if (queue.size() + size > RESERVED && (check || checkCount++ >= CHECKCOUNT2))
+ {
+ checkCount = 0;
+ synchronized(rootsStack)
+ {
+ check = rootsStack.pushIfWaiting(child);
+ }
+ if (check)
+ continue;
+ }
if (size == 0)
{
// We have emptied the stack, so reset
@@ -693,12 +714,110 @@ public class ObjectMarker
} // end of processing one GC root
}
}
+ completed = true;
+ }
+ catch (RuntimeException e)
+ {
+ progressListener.sendUserMessage(Severity.ERROR, Messages.ObjectMarker_ErrorMarkingObjects, e);
+ failure = e;
+ }
+ catch (OutOfMemoryError e)
+ {
+ failure = e;
+ if (emptyLocalStacks())
+ {
+ progressListener.sendUserMessage(Severity.WARNING, Messages.ObjectMarker_WarningMarkingObjects, e);
+ // Mark as done as the remaining threads can continue to handle the remaining work
+ completed = true;
+ }
+ else
+ {
+ progressListener.sendUserMessage(Severity.ERROR, Messages.ObjectMarker_ErrorMarkingObjects, e);
+ }
+ }
+ catch (Error e)
+ {
+ progressListener.sendUserMessage(Severity.ERROR, Messages.ObjectMarker_ErrorMarkingObjects, e);
+ failure = e;
}
finally
{
rootsStack.unlinkThread();
}
}
+
+ /**
+ * Fill the root stack with some of the local items
+ * if other threads are waiting and there are still
+ * enough items on the local stack/queue.
+ * @return
+ */
+ private boolean fillRootsStack()
+ {
+ boolean check;
+ check = true;
+ if (queue.size() > 0 && queue.size() + size > RESERVED)
+ {
+ int fromQueue;
+ synchronized (rootsStack)
+ {
+ do
+ {
+ fromQueue = queue.get();
+ check = rootsStack.pushIfWaiting(fromQueue);
+ }
+ while (check && queue.size() > 0 && queue.size() + size > RESERVED);
+ }
+ if (!check)
+ queue.put(fromQueue);
+ }
+ else if (size > RESERVED)
+ {
+ synchronized (rootsStack)
+ {
+ do
+ {
+ check = rootsStack.pushIfWaiting(current);
+ if (check)
+ current = data[--size];
+ }
+ while (check && size > RESERVED);
+ }
+ }
+ return check;
+ }
+
+ /**
+ * Empty the local stacks to the global stack.
+ * @return If this was successful.
+ */
+ boolean emptyLocalStacks()
+ {
+ synchronized (rootsStack)
+ {
+ if (DEBUG) System.out.println("emptyLocalStacks "+rootsStack.totalThreads+" "+current+" "+size+" "+queue.size()); //$NON-NLS-1$//$NON-NLS-2$//$NON-NLS-3$//$NON-NLS-4$
+ // Only can requeue if another thread is still running
+ if (rootsStack.totalThreads >= 2)
+ {
+ if (current != -1)
+ {
+ rootsStack.push(current);
+ current = -1;
+ }
+ while (size > 0)
+ {
+ rootsStack.push(data[--size]);
+ }
+ while (queue.size() > 0)
+ {
+ rootsStack.push(queue.get());
+ }
+ }
+ }
+ if (DEBUG) System.out.println("emptyLocalStacks "+current+" "+size+" "+queue.size()); //$NON-NLS-1$//$NON-NLS-2$//$NON-NLS-3$
+ return current == -1 && size == 0 && queue.size() == 0;
+ }
+
/**
* Is the objectId in range for the local stack?
* @param val
diff --git a/plugins/org.eclipse.mat.tests/src/org/eclipse/mat/tests/CreatePerformanceDump.java b/plugins/org.eclipse.mat.tests/src/org/eclipse/mat/tests/CreatePerformanceDump.java
new file mode 100644
index 00000000..77b9d2d3
--- /dev/null
+++ b/plugins/org.eclipse.mat.tests/src/org/eclipse/mat/tests/CreatePerformanceDump.java
@@ -0,0 +1,102 @@
+/*******************************************************************************
+ * Copyright (c) 2019 IBM Corporation.
+ * All rights reserved. This program and the accompanying materials
+ * are made available under the terms of the Eclipse Public License v1.0
+ * which accompanies this distribution, and is available at
+ * http://www.eclipse.org/legal/epl-v10.html
+ *
+ * Contributors:
+ * Andrew Johnson (IBM Corporation) - initial API and implementation
+ *******************************************************************************/
+package org.eclipse.mat.tests;
+
+/**
+ * Creates many arrays and initializes them.
+ * Used to stress test object marking, HPROF parsing,
+ * and dominator tree building.
+ *
+ */
+public class CreatePerformanceDump
+{
+ public static final int DEFAULT_M = 200000;
+ public static final int DEFAULT_N = 200;
+ final String a[][];
+ final int numStrings;
+
+ /**
+ * Create lots of arrays of Strings.
+ * @param m size of each sub-array
+ * @param n number of sub-arrays
+ * @param mode How to initialize the arrays.
+ * 0 first sub-array is initalized with different strings, others use same strings
+ * 1 first sub-array is initalized with different strings, others use strings from
+ * first sub-array or sometimes new copies of the string
+ * 2 first sub-array is initalized with different strings, others use strings from
+ * the previous sub-array or sometimes new copies of the string
+ * 3 first sub-array is initalized with different strings, others use strings from
+ * previous sub-arrays or sometimes new copies of the string
+ * 4 sub-arrays filled with just one string
+ *
+ * Mode 0 creates m strings
+ * Modes 1,2,3 create m + m*(n-1)/n strings = m*(2-1/n) strings
+ * Mode 4 creates a single string
+ */
+ public CreatePerformanceDump(int m, int n, int mode) {
+ a = new String[n][m];
+ int s = 0;
+ for (int i = 0; i < m; ++i)
+ {
+ if (mode == 4 && i > 0)
+ {
+ a[0][i] = a[0][i - 1];
+ }
+ else
+ {
+ a[0][i] = "S"+i;
+ ++s;
+ }
+ }
+ for (int j = 1; j < n; ++j)
+ {
+ for (int i = 0; i < m; ++i)
+ {
+ // fill in the array entries
+ int prev = 0;
+ if (mode == 2)
+ prev = j - 1;
+ else if (mode == 3)
+ prev = (int)((long)i * j / m);
+
+ if (i % n == j && mode > 0 && mode != 4)
+ {
+ // sometimes with brand new Strings
+ a[j][i] = new String(a[prev][i]);
+ ++s;
+ }
+ else
+ {
+ a[j][i] = a[prev][i];
+ }
+ }
+ }
+ numStrings = s;
+ }
+
+ public static void main(String[] args) throws Exception
+ {
+ int m = args.length > 0 ? Integer.parseInt( args[0]) : DEFAULT_M;
+ int n = args.length > 1 ? Integer.parseInt(args[1]) : DEFAULT_N;
+ int mode = args.length > 2 ? Integer.parseInt(args[2]) : 0;
+ CreatePerformanceDump b = new CreatePerformanceDump(m, n, mode);
+ System.out.println("Acquire Heap Dump NOW (then press any key to terminate program)");
+ int c = System.in.read();
+ // Control-break causes read to return early, so try again for another key to wait
+ // for the dump to complete
+ if (c == -1)
+ c = System.in.read();
+
+ System.out.println("Created array of size " + n + " with each entry holding an array of size " + m
+ + " of String with " + b.numStrings + " different strings using mode: " + mode
+ + ". Last entry: " + b.a[b.a.length - 1][b.a[b.a.length - 1].length - 1]);
+ }
+}

Back to the top