Skip to content

Commit

Permalink
Fixes as per review comments by @chhsiao90
Browse files Browse the repository at this point in the history
- Several javadoc typos fixed
- Made ThreadMeta and ThreadInfo immutable
- Removed copy() methods from LeanNode
- Cleaned up and hardened VirtualMachine LogSource generation, reverting
to original logic if no agentpath or logPath parameters are defined in
the JVM args
  • Loading branch information
PhRX committed Feb 9, 2017
1 parent d16c81a commit 54f7910
Show file tree
Hide file tree
Showing 7 changed files with 43 additions and 42 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -154,8 +154,7 @@ public void handle(ThreadMeta newThreadMeta)
{
threadMap.compute(
newThreadMeta.getThreadId(),
(k, v) -> v == null ? new ThreadInfo(newThreadMeta)
: v.checkAndSetName(newThreadMeta.getThreadName()));
(k, v) -> v == null ? new ThreadInfo(newThreadMeta) : v.checkAndSetName(newThreadMeta));
emitProfileIfNeeded();
}

Expand Down Expand Up @@ -204,8 +203,7 @@ private void collectThreadDump()
*/
private void collectStackFrame(StackFrame stackFrame)
{
currentNode = currentNode
.add(nanosSpent, new FrameInfo(stackFrame), stackTrace.isEmpty());
currentNode = currentNode.add(nanosSpent, new FrameInfo(stackFrame), stackTrace.isEmpty());
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
public final class ThreadMeta implements LogEvent
{
private final long threadId;
private String threadName;
private final String threadName;

public ThreadMeta(long threadId, String name)
{
Expand Down Expand Up @@ -71,7 +71,7 @@ public ThreadMeta update(ThreadMeta newMeta)
{
if (newMeta.threadName != null && !newMeta.threadName.isEmpty())
{
threadName = newMeta.threadName;
return new ThreadMeta(newMeta.threadId, newMeta.threadName);
}

return this;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ private LeanNode(FrameInfo frame, long nanos, LeanNode parent)
* Copy constructor.
*
* @param source the source LeanNode which is being copied
* @param new Parent the parent of the copy (which itself generally is a copy)
* @param newParent the parent of the copy (which itself generally is a copy)
*/
protected LeanNode(LeanNode source, LeanNode newParent)
{
Expand All @@ -90,7 +90,7 @@ protected LeanNode(LeanNode source, LeanNode newParent)
this.parent = newParent;
this.childMap = new HashMap<>();
// The FrameInfo key is an immutable object, no need to copy it.
source.childMap.forEach((key, value) -> this.childMap.put(key, value.copy(this)));
source.childMap.forEach((key, value) -> this.childMap.put(key, new LeanNode(value, this)));
}

// Instance Accessors
Expand Down Expand Up @@ -159,7 +159,7 @@ public boolean isThreadNode()
// Aggregation Methods

/**
* Aggregates teh information from a child {@link StackFrame} into the children of this LeanNode, updating the total
* Aggregates the information from a child {@link StackFrame} into the children of this LeanNode, updating the total
* data for this LeanNode in the process.
*
* The add method is called from the LogCollector, on the parent with the next (potentially last) child from the
Expand Down Expand Up @@ -218,29 +218,6 @@ public Stream<LeanNode> flatten()
return concat(of(this), childMap.values().stream().flatMap(LeanNode::flatten));
}

// Copy Methods

/**
* Copy method for the top of the tree, which has no parent.
*
* @return a copy of this object
*/
public LeanNode copy()
{
return new LeanNode(this, null);
}

/**
* Copy method for children.
*
* @param newParent the new parent for the children
* @return a copy of this object
*/
public LeanNode copy(LeanNode newParent)
{
return new LeanNode(this, newParent);
}

// Debug Methods

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,6 @@ public void setThreadInfo(ThreadInfo threadInfo)

// Copy Methods

@Override
public LeanThreadNode copy()
{
return new LeanThreadNode(this);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ public class ThreadInfo
// Instance Properties

private final long id;
private String name;
private final String name;

// Instance Constructors

Expand Down Expand Up @@ -52,16 +52,18 @@ public String getName()
}

/**
* Sets the thread name, if it is not null or empty.
* Checks whether the name in the new {@link ThreadMeta} is empty. If it is, this (immutable) ThreadInfo is
* returned, otherwise a new one is returned based on the new {@link ThreadMeta}
*
* @param name the (new) thread name
* @param meta the {@link ThreadMeta} whose metadata will be stored
* @return this object
*/
public ThreadInfo checkAndSetName(String name)
public ThreadInfo checkAndSetName(ThreadMeta meta)
{
if (name != null && !name.isEmpty())
String newName = meta.getThreadName();
if (newName != null && !newName.isEmpty())
{
this.name = name;
return new ThreadInfo(meta);
}
return this;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,17 +66,42 @@ public boolean isAgentLoaded()
return agentLoaded;
}

public LogSource getLogSourceFromVmArgs()
public LogSource getLogSourceFromVmArgs() throws CantReadFromSourceException
{
// Bail out if agentpath is not defined.
if (vmArgs.indexOf("-agentpath") < 0)
{
return getLogSource();
}

// Extract the value of the agentpath parameter

int agentPathStart = vmArgs.indexOf("-agentpath") + "-agentpath".length();

// TODO FIX : if the logPath contains spaces, this will cause trouble.
int agentPathEnd = vmArgs.indexOf(' ', agentPathStart);

// If the agentpath is the last parameter in the VM Args, no space would be found.
agentPathEnd = (agentPathEnd < 0) ? vmArgs.length() : agentPathEnd;

String agentPath = vmArgs.substring(agentPathStart, agentPathEnd);

// Bail out if logPath is not defined.
if (agentPath.indexOf("logPath") < 0)
{
return getLogSource();
}

// Extract the value of the logPath parameter

int logPathStart = agentPath.indexOf("logPath=") + "logPath=".length();
int commaPos = agentPath.indexOf(",", logPathStart);
int spacePos = agentPath.indexOf(' ', logPathStart);
int logPathEnd = commaPos > 0 ? commaPos : (spacePos > 0 ? spacePos : agentPath.length());

return new FileLogSource(new File(agentPath.substring(logPathStart, logPathEnd)));
File result = new File(agentPath.substring(logPathStart, logPathEnd));

return result.exists() ? new FileLogSource(result) : getLogSource();
}

public LogSource getLogSource() throws CantReadFromSourceException
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ protected void initialize()
/**
* Sets the {@link ApplicationContext} and propagates it to any contained controllers.
*
* @param appCtx the {@link ApplicationContext}
* @param applicationContext the {@link ApplicationContext}
*/
@Override
public void setApplicationContext(ApplicationContext applicationContext)
Expand Down

0 comments on commit 54f7910

Please sign in to comment.