Skip to content

Make Trace instances as static final to give JIT a chance to optimize #156

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

Open
turbanoff wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Make Trace instances as static final to give JIT a chance to optimize #156

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
2 changes: 1 addition & 1 deletion loadtime/src/main/java/org/aspectj/weaver/loadtime/Aj.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ public class Aj implements ClassPreProcessor {
*/
private static ReferenceQueue adaptorQueue = new ReferenceQueue();

private static Trace trace = TraceFactory.getTraceFactory().getTrace(Aj.class);
private static final Trace trace = TraceFactory.getTraceFactory().getTrace(Aj.class);

public Aj() {
this(null);
Expand Down
2 changes: 1 addition & 1 deletion loadtime/src/main/java/org/aspectj/weaver/loadtime/ClassLoaderWeavingAdaptor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ public class ClassLoaderWeavingAdaptor extends WeavingAdaptor {

private List<ConcreteAspectCodeGen> concreteAspects = new ArrayList<>();

private static Trace trace = TraceFactory.getTraceFactory().getTrace(ClassLoaderWeavingAdaptor.class);
private static final Trace trace = TraceFactory.getTraceFactory().getTrace(ClassLoaderWeavingAdaptor.class);

public ClassLoaderWeavingAdaptor() {
super();
Expand Down
2 changes: 1 addition & 1 deletion loadtime/src/main/java/org/aspectj/weaver/loadtime/DefaultWeavingContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ public class DefaultWeavingContext implements IWeavingContext {
protected BcelWeakClassLoaderReference loaderRef;
private String shortName;

private static Trace trace = TraceFactory.getTraceFactory().getTrace(DefaultWeavingContext.class);
private static final Trace trace = TraceFactory.getTraceFactory().getTrace(DefaultWeavingContext.class);

/**
* Construct a new WeavingContext to use the specified ClassLoader This is the constructor which should be used.
Expand Down
2 changes: 1 addition & 1 deletion loadtime/src/main/java/org/aspectj/weaver/loadtime/WeavingURLClassLoader.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ public class WeavingURLClassLoader extends ExtensibleURLClassLoader implements W
private boolean initializingAdaptor;
private Map generatedClasses = new HashMap(); /* String -> byte[] */

private static Trace trace = TraceFactory.getTraceFactory().getTrace(WeavingURLClassLoader.class);
private static final Trace trace = TraceFactory.getTraceFactory().getTrace(WeavingURLClassLoader.class);

/*
* This constructor is needed when using "-Djava.system.class.loader".
Expand Down
2 changes: 1 addition & 1 deletion org.aspectj.matcher/src/main/java/org/aspectj/weaver/Dump.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ public class Dump {

private static boolean preserveOnNextReset = false;

private static Trace trace = TraceFactory.getTraceFactory().getTrace(Dump.class);
private static final Trace trace = TraceFactory.getTraceFactory().getTrace(Dump.class);

/**
* for testing only, so that we can verify dump contents after compilation has completely finished
Expand Down
2 changes: 1 addition & 1 deletion org.aspectj.matcher/src/main/java/org/aspectj/weaver/Lint.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ public class Lint {
public final Kind missingAspectForReweaving = new Kind("missingAspectForReweaving",
"aspect {0} cannot be found when reweaving {1}");

private static Trace trace = TraceFactory.getTraceFactory().getTrace(Lint.class);
private static final Trace trace = TraceFactory.getTraceFactory().getTrace(Lint.class);

public Lint(World world) {
if (trace.isTraceEnabled()) {
Expand Down
2 changes: 1 addition & 1 deletion org.aspectj.matcher/src/main/java/org/aspectj/weaver/World.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ public abstract class World implements Dump.INode {

public int infoMessagesEnabled = 0; // 0=uninitialized, 1=no, 2=yes

private static Trace trace = TraceFactory.getTraceFactory().getTrace(World.class);
private static final Trace trace = TraceFactory.getTraceFactory().getTrace(World.class);

private boolean errorThreshold;
private boolean warningThreshold;
Expand Down
79 changes: 37 additions & 42 deletions org.aspectj.matcher/src/main/java/org/aspectj/weaver/tools/TraceFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,14 @@ public abstract class TraceFactory {
public final static String DEFAULT_FACTORY_NAME = "default";

protected static boolean debug = getBoolean(DEBUG_PROPERTY,false);
private static TraceFactory instance;
private static final TraceFactory instance;

public Trace getTrace (Class clazz) {
return instance.getTrace(clazz);
public Trace getTrace(Class clazz) {
return instance.getTrace(clazz);
}

public static TraceFactory getTraceFactory () {
return instance;
public static TraceFactory getTraceFactory() {
return instance;
}

protected static boolean getBoolean(String name, boolean def) {
Expand All @@ -33,46 +33,41 @@ protected static boolean getBoolean(String name, boolean def) {
return Boolean.parseBoolean(value);
}

static {

/*
* Allow user to override default behaviour or specify their own factory
*/
String factoryName = System.getProperty(FACTORY_PROPERTY);
if (factoryName != null) try {
if (factoryName.equals(DEFAULT_FACTORY_NAME)) {
instance = new DefaultTraceFactory();
}
else {
Class<?> factoryClass = Class.forName(factoryName);
instance = (TraceFactory)factoryClass.getDeclaredConstructor().newInstance();
}
}
catch (Throwable th) {
if (debug) th.printStackTrace();
}
static {
instance = createInstance();
if (debug) System.err.println("TraceFactory.instance=" + instance);
}

/*
* Try to load external trace infrastructure using supplied factories
*/
if (instance == null) try {
{
Class<?> factoryClass = Class.forName("org.aspectj.weaver.tools.Jdk14TraceFactory");
instance = (TraceFactory)factoryClass.getDeclaredConstructor().newInstance();
}
}
catch (Throwable th) {
if (debug) th.printStackTrace();
}
private static TraceFactory createInstance() {
/*
* Allow user to override default behaviour or specify their own factory
*/
String factoryName = System.getProperty(FACTORY_PROPERTY);
if (factoryName != null) try {
if (factoryName.equals(DEFAULT_FACTORY_NAME)) {
return new DefaultTraceFactory();
} else {
Class<?> factoryClass = Class.forName(factoryName);
return (TraceFactory)factoryClass.getDeclaredConstructor().newInstance();
}
} catch (Throwable th) {
if (debug) th.printStackTrace();
}

/*
* Use default trace
*/
if (instance == null) {
instance = new DefaultTraceFactory();
}
/*
* Try to load external trace infrastructure using supplied factories
*/
try {
Class<?> factoryClass = Class.forName("org.aspectj.weaver.tools.Jdk14TraceFactory");
return (TraceFactory)factoryClass.getDeclaredConstructor().newInstance();
} catch (Throwable th) {
if (debug) th.printStackTrace();
}

if (debug) System.err.println("TraceFactory.instance=" + instance);
/*
* Use default trace
*/
return new DefaultTraceFactory();
}

}
2 changes: 1 addition & 1 deletion weaver/src/main/java/org/aspectj/weaver/bcel/BcelClassWeaver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@

class BcelClassWeaver implements IClassWeaver {

private static Trace trace = TraceFactory.getTraceFactory().getTrace(BcelClassWeaver.class);
private static final Trace trace = TraceFactory.getTraceFactory().getTrace(BcelClassWeaver.class);

// Name of helper method generated by JDT compiler. Javac uses a separate inner class.
private static final String SWITCH_TABLE_SYNTHETIC_METHOD_PREFIX = "$SWITCH_TABLE$";
Expand Down
2 changes: 1 addition & 1 deletion weaver/src/main/java/org/aspectj/weaver/bcel/BcelGenericSignatureToTypeXConverter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@
*/
public class BcelGenericSignatureToTypeXConverter {

private static Trace trace = TraceFactory.getTraceFactory().getTrace(BcelGenericSignatureToTypeXConverter.class);
private static final Trace trace = TraceFactory.getTraceFactory().getTrace(BcelGenericSignatureToTypeXConverter.class);

public static ResolvedType classTypeSignature2TypeX(GenericSignature.ClassTypeSignature aClassTypeSignature,
GenericSignature.FormalTypeParameter[] typeParams, World world) throws GenericSignatureFormatException {
Expand Down
2 changes: 1 addition & 1 deletion weaver/src/main/java/org/aspectj/weaver/bcel/BcelWeaver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ public class BcelWeaver {
public static final String CLOSURE_CLASS_PREFIX = "$Ajc";
public static final String SYNTHETIC_CLASS_POSTFIX = "$ajc";

private static Trace trace = TraceFactory.getTraceFactory().getTrace(BcelWeaver.class);
private static final Trace trace = TraceFactory.getTraceFactory().getTrace(BcelWeaver.class);

private transient final BcelWorld world;
private final CrosscuttingMembersSet xcutSet;
Expand Down
2 changes: 1 addition & 1 deletion weaver/src/main/java/org/aspectj/weaver/bcel/BcelWorld.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ public class BcelWorld extends World implements Repository {
private WeavingXmlConfig xmlConfiguration;
private List<TypeDelegateResolver> typeDelegateResolvers;

private static Trace trace = TraceFactory.getTraceFactory().getTrace(BcelWorld.class);
private static final Trace trace = TraceFactory.getTraceFactory().getTrace(BcelWorld.class);

public BcelWorld() {
this("");
Expand Down
2 changes: 1 addition & 1 deletion weaver/src/main/java/org/aspectj/weaver/bcel/ClassPathManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@
*/
public class ClassPathManager {

private static Trace trace = TraceFactory.getTraceFactory().getTrace(ClassPathManager.class);
private static final Trace trace = TraceFactory.getTraceFactory().getTrace(ClassPathManager.class);

private static int maxOpenArchives = -1;

Expand Down
2 changes: 1 addition & 1 deletion weaver/src/main/java/org/aspectj/weaver/tools/WeavingAdaptor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ public class WeavingAdaptor implements IMessageContext {
private static final int WEAVE_JAVA_PACKAGE = 0x2;
private static final int WEAVE_JAVAX_PACKAGE = 0x4;

private static Trace trace = TraceFactory.getTraceFactory().getTrace(WeavingAdaptor.class);
private static final Trace trace = TraceFactory.getTraceFactory().getTrace(WeavingAdaptor.class);

protected WeavingAdaptor() {
}
Expand Down
8 changes: 1 addition & 7 deletions weaver/src/main/java/org/aspectj/weaver/tools/cache/SimpleCache.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,10 +120,7 @@ private static class StoreableCachingMap extends HashMap {
private static int DEF_STORING_TIMER = 60000; //ms
private int storingTimer;

private transient Trace trace;
private void initTrace(){
trace = TraceFactory.getTraceFactory().getTrace(StoreableCachingMap.class);
}
private static final Trace trace = TraceFactory.getTraceFactory().getTrace(StoreableCachingMap.class);
Copy link
Contributor

@kriegaex kriegaex Apr 15, 2022

Choose a reason for hiding this comment

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

I am fine with the other changes. Here, I am not sure if @aclement had anything special in mind by choosing a transient instance field rather than a static one. Andy, I know 2012 was long ago. Can you remember anything in the context of Bugzilla 386341? If you are OK with this particular change here, I would merge it.

Copy link
Contributor Author

@turbanoff turbanoff Jan 19, 2023

Choose a reason for hiding this comment

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

Would be ok for merge if I revert this one?

Copy link
Contributor

@kriegaex kriegaex Jan 27, 2023

Choose a reason for hiding this comment

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

@turbanoff, I am reaching out to tell you that I am not ignoring your PRs because I do not like them or so. I simply would prefer Andy to review and merge them. But currently being the only active maintainer and busy with my normal job as well, I simply prioritise scratching my own itches, fixing bugs or working on little improvements of my personal interest. I am sure your improvements are valuable, but assessing whether they might have undesired side effects in parts of the code I do not know is simply time I currently choose to spend elsewhere, be it on AspectJ or privately. But your work is not lost. Please do not feel discouraged to open PRs. This is about my own situation, not about the quality of your contributions. Please accept my apologies. 🙂


// private StoreableCachingMap(String folder) {
// this.folder = folder;
Expand All @@ -132,7 +129,6 @@ private void initTrace(){

private StoreableCachingMap(String folder, int storingTimer){
this.folder = folder;
initTrace();
this.storingTimer = storingTimer;
}

Expand All @@ -149,11 +145,9 @@ public static StoreableCachingMap init(String folder, int storingTimer) {
new FileInputStream(file));
// Deserialize the object
StoreableCachingMap sm = (StoreableCachingMap) in.readObject();
sm.initTrace();
in.close();
return sm;
} catch (Exception e) {
Trace trace = TraceFactory.getTraceFactory().getTrace(StoreableCachingMap.class);
trace.error("Error reading Storable Cache", e);
}
}
Expand Down