Skip to content
This repository was archived by the owner on Jan 13, 2023. It is now read-only.

Commit e7dc577

Browse files
author
Mustafa ÖNCEL
committed
Make logger thread safe, fix performance issues
1 parent 136ce87 commit e7dc577

3 files changed

Lines changed: 96 additions & 33 deletions

File tree

‎src/main/java/ch/njol/skript/classes/data/DefaultChangers.java‎

Lines changed: 54 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222

2323
package ch.njol.skript.classes.data;
2424

25+
import ch.njol.skript.Skript;
2526
import ch.njol.skript.aliases.ItemType;
2627
import ch.njol.skript.bukkitutil.PlayerUtils;
2728
import ch.njol.skript.classes.Changer;
@@ -43,6 +44,8 @@
4344
*/
4445
public final class DefaultChangers {
4546

47+
public static final Material[] cachedMaterials = Material.values();
48+
4649
public static final Changer<Entity> entityChanger = new Changer<Entity>() {
4750
@SuppressWarnings("unchecked")
4851
@Override
@@ -175,7 +178,6 @@ public void change(final Item[] what, final @Nullable Object[] delta, final Chan
175178
};
176179

177180
public static final Changer<Inventory> inventoryChanger = new Changer<Inventory>() {
178-
@SuppressWarnings("unchecked")
179181
@Override
180182
@Nullable
181183
public Class<?>[] acceptChange(final ChangeMode mode) {
@@ -188,7 +190,6 @@ public Class<?>[] acceptChange(final ChangeMode mode) {
188190
return CollectionUtils.array(ItemType[].class, Inventory[].class);
189191
}
190192

191-
@SuppressWarnings("deprecation")
192193
@Override
193194
public void change(final Inventory[] invis, final @Nullable Object[] delta, final ChangeMode mode) {
194195
for (final Inventory invi : invis) {
@@ -212,6 +213,7 @@ public void change(final Inventory[] invis, final @Nullable Object[] delta, fina
212213
//$FALL-THROUGH$
213214
case ADD:
214215
assert delta != null;
216+
/*
215217
for (final Object d : delta) {
216218
if (d instanceof Inventory) {
217219
for (final ItemStack i : (Inventory) d) {
@@ -222,10 +224,59 @@ public void change(final Inventory[] invis, final @Nullable Object[] delta, fina
222224
((ItemType) d).addTo(invi);
223225
}
224226
}
227+
*/
228+
if (delta instanceof ItemStack[]) { // Old behavior - legacy code (is it used? no idea)
229+
final ItemStack[] items = (ItemStack[]) delta;
230+
if (items.length > 36) {
231+
return;
232+
}
233+
for (final Object d : delta) {
234+
if (d instanceof Inventory) {
235+
for (final ItemStack i : (Inventory) d) {
236+
if (i != null)
237+
invi.addItem(i);
238+
}
239+
} else {
240+
((ItemType) d).addTo(invi);
241+
}
242+
}
243+
} else {
244+
for (final Object d : delta) {
245+
if (d instanceof ItemStack) {
246+
new ItemType((ItemStack) d).addTo(invi); // Can't imagine why would be ItemStack, but just in case...
247+
} else if (d instanceof ItemType) {
248+
((ItemType) d).addTo(invi);
249+
} else if (d instanceof Block) {
250+
new ItemType((Block) d).addTo(invi);
251+
} else {
252+
Skript.error("Can't " + d.toString() + " to an inventory!");
253+
}
254+
}
255+
}
225256
break;
226257
case REMOVE:
227258
case REMOVE_ALL:
228259
assert delta != null;
260+
if (delta.length == cachedMaterials.length) {
261+
// Potential fast path: remove all items -> clear inventory
262+
boolean equal = true;
263+
for (int i = 0; i < delta.length; i++) {
264+
if (!(delta[i] instanceof ItemType)) {
265+
equal = false;
266+
break; // Not an item, take slow path
267+
}
268+
if (((ItemType) delta[i]).getMaterial() != cachedMaterials[i]) {
269+
equal = false;
270+
break;
271+
}
272+
}
273+
if (equal) { // Take fast path, break out before slow one
274+
invi.clear();
275+
break;
276+
}
277+
}
278+
279+
// Slow path
229280
for (final Object d : delta) {
230281
if (d instanceof Inventory) {
231282
assert mode == ChangeMode.REMOVE;
@@ -243,8 +294,7 @@ public void change(final Inventory[] invis, final @Nullable Object[] delta, fina
243294
}
244295
final InventoryHolder holder = invi.getHolder();
245296
if (holder instanceof Player) {
246-
// FIXME Change with PlayerUtils#updateInventory?
247-
((Player) holder).updateInventory();
297+
PlayerUtils.updateInventory((Player) holder);
248298
}
249299
}
250300
}
@@ -262,7 +312,6 @@ public Class<?>[] acceptChange(final ChangeMode mode) {
262312
return CollectionUtils.array(ItemType[].class, Inventory[].class);
263313
}
264314

265-
@SuppressWarnings("deprecation")
266315
@Override
267316
public void change(final Block[] blocks, final @Nullable Object[] delta, final ChangeMode mode) {
268317
for (final Block block : blocks) {

‎src/main/java/ch/njol/skript/log/SkriptLogger.java‎

Lines changed: 41 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -29,9 +29,7 @@
2929
import org.eclipse.jdt.annotation.Nullable;
3030
import org.fusesource.jansi.Ansi;
3131

32-
import java.util.ArrayList;
33-
import java.util.Collection;
34-
import java.util.List;
32+
import java.util.*;
3533
import java.util.logging.Level;
3634
import java.util.logging.Logger;
3735

@@ -49,14 +47,14 @@ public final class SkriptLogger {
4947
@SuppressWarnings("null")
5048
public static final Logger LOGGER = Bukkit.getServer() != null ? Bukkit.getLogger() : Logger.getLogger(Logger.GLOBAL_LOGGER_NAME); // cannot use Bukkit in tests
5149
private static final HandlerList handlers = new HandlerList();
52-
private static final List<LogEntry> suppressed = new ArrayList<>();
50+
private static final List<LogEntry> suppressed = Collections.synchronizedList(new ArrayList<>());
5351
static boolean debug;
5452
@Nullable
5553
private static Node node;
5654
private static Verbosity verbosity = Verbosity.NORMAL;
57-
private static boolean suppressing;
58-
private static boolean suppressWarnings;
59-
private static boolean suppressErrors;
55+
private static volatile boolean suppressing;
56+
private static volatile boolean suppressWarnings;
57+
private static volatile boolean suppressErrors;
6058

6159
/**
6260
* Shorthand for <tt>{@link #startLogHandler(LogHandler) startLogHandler}(new {@link RetainingLogHandler}());</tt>
@@ -101,23 +99,29 @@ public static final ParseLogHandler startParseLogHandler() {
10199
* @see RedirectingLogHandler
102100
*/
103101
public static final <T extends LogHandler> T startLogHandler(final T h) {
104-
handlers.add(h);
102+
synchronized (handlers) {
103+
handlers.add(h);
104+
}
105105
return h;
106106
}
107107

108108
static final void removeHandler(final LogHandler h) {
109-
if (!handlers.contains(h))
110-
return;
111-
if (!h.equals(handlers.remove())) {
112-
int i = 1;
113-
while (!h.equals(handlers.remove()))
114-
i++;
115-
LOGGER.severe("[Skript] " + i + " log handler" + (i == 1 ? " was" : "s were") + " not stopped properly! (at " + getCaller() + ") [if you're a server admin and you see this message please create a bug report at " + Skript.ISSUES_LINK + " if there is not already one]");
109+
synchronized (handlers) {
110+
if (!handlers.contains(h))
111+
return;
112+
if (!h.equals(handlers.remove())) {
113+
int i = 1;
114+
while (!h.equals(handlers.remove()))
115+
i++;
116+
LOGGER.severe("[Skript] " + i + " log handler" + (i == 1 ? " was" : "s were") + " not stopped properly! (at " + getCaller() + ") [if you're a server admin and you see this message please create a bug report at " + Skript.ISSUES_LINK + " if there is not already one]");
117+
}
116118
}
117119
}
118120

119121
static final boolean isStopped(final LogHandler h) {
120-
return !handlers.contains(h);
122+
synchronized (handlers) {
123+
return !handlers.contains(h);
124+
}
121125
}
122126

123127
/**
@@ -154,8 +158,9 @@ static final StackTraceElement getCaller() {
154158
*/
155159
@Nullable
156160
public static final StackTraceElement findCaller(final @Nullable String... exclusions) {
157-
for (final StackTraceElement e : Thread.currentThread().getStackTrace()) {
158-
if (!e.getClassName().startsWith(SkriptLogger.class.getPackage().getName()) && !e.toString().contains("java.lang.Thread.getStackTrace") && !e.toString().contains("java.lang.Thread.currentThread")) {
161+
// Thread.currentThread().getStackTrace() is more memory friendly, but slower
162+
for (final StackTraceElement e : new Throwable().getStackTrace()) {
163+
if (!e.getClassName().startsWith(SkriptLogger.class.getPackage().getName())) {
159164
if (exclusions != null && exclusions.length > 0) {
160165
for (final String exclusion : exclusions)
161166
if (!e.getClassName().startsWith(exclusion))
@@ -211,19 +216,28 @@ public static final void log(final @Nullable LogEntry entry) {
211216
}
212217
if (Skript.testing() && node != null && node.debug())
213218
System.out.print("---> " + entry.level + "/" + ErrorQuality.get(entry.quality) + ": " + entry.getMessage() + " ::" + LogEntry.findCaller());
214-
for (final LogHandler h : handlers) {
215-
final LogResult r = h.log(entry);
216-
switch (r) {
217-
case CACHED:
218-
return;
219-
case DO_NOT_LOG:
220-
entry.discarded("denied by " + h);
221-
return;
222-
case LOG:
219+
synchronized (handlers) {
220+
final Iterator<LogHandler> iterator = handlers.iterator();
221+
synchronized (iterator) {
222+
while (iterator.hasNext()) {
223+
final LogHandler h = iterator.next();
224+
final LogResult r = h.log(entry);
225+
226+
switch (r) {
227+
case CACHED:
228+
return;
229+
case DO_NOT_LOG:
230+
entry.discarded("denied by " + h);
231+
return;
232+
case LOG:
233+
}
234+
}
223235
}
224236
}
225237
if (suppressing) {
238+
suppressed.remove(entry);
226239
suppressed.add(entry);
240+
227241
return;
228242
}
229243
final Level level = entry.getLevel();

‎version‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
2.2.16-pre2
1+
2.2.16

0 commit comments

Comments
 (0)