Skip to content
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.

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

Remove no longer needed garbage collector meddling. #971

Open
wants to merge 3 commits into
base: next
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
18 changes: 12 additions & 6 deletions src/freenet/config/SubConfig.java
Original file line number Diff line number Diff line change
Expand Up @@ -172,47 +172,53 @@ public int getInt(String optionName) {
synchronized(this) {
o = (IntOption) map.get(optionName);
}
return o.getValue();
// return fallback value for ignored options (null). This avoids breaking plugins which try to get ignored options.
return o == null ? -1 : o.getValue();
Bombe marked this conversation as resolved.
Show resolved Hide resolved
Comment on lines 173 to +176
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we replace the ternaries with the more descriptive Map.getOrDefault?

}

public long getLong(String optionName) {
LongOption o;
synchronized(this) {
o = (LongOption) map.get(optionName);
}
return o.getValue();
// return fallback value for ignored options (null). This avoids breaking plugins which try to get ignored options.
return o == null ? -1L : o.getValue();
}

public boolean getBoolean(String optionName) {
BooleanOption o;
synchronized(this) {
o = (BooleanOption) map.get(optionName);
}
return o.getValue();
// return fallback value for ignored options (null). This avoids breaking plugins which try to get ignored options.
return o == null ? false : o.getValue();
}

public String getString(String optionName) {
StringOption o;
synchronized(this) {
o = (StringOption) map.get(optionName);
}
return o.getValue().trim();
// return fallback value for ignored options (null). This avoids breaking plugins which try to get ignored options.
return o == null ? "" : o.getValue().trim();
}

public String[] getStringArr(String optionName) {
StringArrOption o;
synchronized(this) {
o = (StringArrOption) map.get(optionName);
}
return o.getValue();
// return fallback value for ignored options (null). This avoids breaking plugins which try to get ignored options.
return o == null ? new String[]{} : o.getValue();
}

public short getShort(String optionName) {
ShortOption o;
synchronized(this) {
o = (ShortOption) map.get(optionName);
}
return o.getValue();
// return fallback value for ignored options (null). This avoids breaking plugins which try to get ignored options.
return o == null ? -1 : o.getValue();
}

public Option<?> removeOption(String optionName) {
Expand Down
50 changes: 1 addition & 49 deletions src/freenet/node/MemoryChecker.java
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this class still serve any purpose (aside from scheduling itself 😄) after removing pretty much all of its code?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haha, that is an excellent observation! 😄 Kill it with 🔥! 🥳

Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,7 @@ public class MemoryChecker implements Runnable {
private volatile boolean goon = false;
private final Ticker ps;
private int aggressiveGCModificator;
private RunningAverage avgFreeMemory;


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The field aggressiveGCModificator can be removed, it’s always -1 now.

public MemoryChecker(Ticker ps, int modificator){
this.ps = ps;
this.aggressiveGCModificator = modificator;
Expand Down Expand Up @@ -48,61 +47,14 @@ public void run() {

long totalMemory = r.totalMemory();
long freeMemory = r.freeMemory();
long maxMemory = r.maxMemory();

Logger.normal(this, "Memory in use: "+SizeUtil.formatSize((totalMemory-freeMemory)));

if (totalMemory == maxMemory || maxMemory == Long.MAX_VALUE) {
// jvm have allocated maximum memory
// totalMemory never decrease, so check it only for once
if (avgFreeMemory == null)
avgFreeMemory = new SimpleRunningAverage(3, freeMemory);
else
avgFreeMemory.report(freeMemory);

if (avgFreeMemory.countReports() >= 3 && avgFreeMemory.currentValue() < 4 * 1024 * 1024) {// average free memory < 4 MB
Logger.normal(this, "Reached threshold, checking for low memory ...");
System.gc();
System.runFinalization();

try {
Thread.sleep(10); // Force a context switch, finalization need a CS to complete
} catch (InterruptedException e) {
}

freeMemory = r.freeMemory();
avgFreeMemory.report(freeMemory);
}
}

int sleeptime = aggressiveGCModificator;
if(sleeptime <= 0) { // We are done
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aggressiveGCModificator going away means this also doesn’t need an if anymore.

ps.queueTimedJob(this, 120 * 250); // 30 sec
return;
} else
ps.queueTimedJob(this, 120L * sleeptime);

// FIXME
// Do not remove until all known memory issues fixed,
// Especially #66
// This probably reduces performance, but it makes
// memory usage *more predictable*. This will make
// tracking down the sort of nasty unpredictable OOMs
// we are getting much easier.
if(aggressiveGCModificator > 0) {
boolean logMINOR = Logger.shouldLog(LogLevel.MINOR, this);
long beforeGCUsedMemory = (r.totalMemory() - r.freeMemory());
if(logMINOR) Logger.minor(this, "Memory in use before GC: "+beforeGCUsedMemory);
long beforeGCTime = System.currentTimeMillis();
System.gc();
System.runFinalization();
long afterGCTime = System.currentTimeMillis();
long afterGCUsedMemory = (r.totalMemory() - r.freeMemory());
if(logMINOR) {
Logger.minor(this, "Memory in use after GC: "+afterGCUsedMemory);
Logger.minor(this, "GC completed after "+(afterGCTime - beforeGCTime)+"ms and \"recovered\" "+(beforeGCUsedMemory - afterGCUsedMemory)+" bytes, leaving "+afterGCUsedMemory+" bytes used");
}
}

}
}
16 changes: 1 addition & 15 deletions src/freenet/node/NodeStats.java
Original file line number Diff line number Diff line change
Expand Up @@ -407,21 +407,7 @@ public void set(Integer val) throws InvalidConfigValueException {
threadLimit = statsConfig.getInt("threadLimit");

// Yes it could be in seconds insteed of multiples of 0.12, but we don't want people to play with it :)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment can also be removed. 🙂

statsConfig.register("aggressiveGC", aggressiveGCModificator, sortOrder++, true, false, "NodeStat.aggressiveGC", "NodeStat.aggressiveGCLong",
new IntCallback() {
@Override
public Integer get() {
return aggressiveGCModificator;
}
@Override
public void set(Integer val) throws InvalidConfigValueException {
if (get().equals(val))
return;
Logger.normal(this, "Changing aggressiveGCModificator to "+val);
aggressiveGCModificator = val;
}
},false);
aggressiveGCModificator = statsConfig.getInt("aggressiveGC");
statsConfig.registerIgnoredOption("aggressiveGC");

myMemoryChecker = new MemoryChecker(node.getTicker(), aggressiveGCModificator);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aggressiveGCModificator should also be removed from this class, including the comment above its declaration. 🙂

statsConfig.register("memoryChecker", true, sortOrder++, true, false, "NodeStat.memCheck", "NodeStat.memCheckLong",
Expand Down