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.

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

Optimize map accesses in several places #1644

Merged
merged 2 commits into from
Sep 23, 2024
Merged

Optimize map accesses in several places #1644

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
972f5e2
Optimize map access in Hashtable
rPraml Sep 23, 2024
5dc0961
Optimize map access in JavaMembers
rPraml Sep 23, 2024
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
33 changes: 19 additions & 14 deletions rhino/src/main/java/org/mozilla/javascript/Hashtable.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,20 +123,25 @@ public int size() {
public void put(Object key, Object value) {
final Entry nv = new Entry(key, value);

if (!map.containsKey(nv)) {
// New value -- insert to end of doubly-linked list
map.put(nv, nv);
if (first == null) {
first = last = nv;
} else {
last.next = nv;
nv.prev = last;
last = nv;
}
} else {
// Update the existing value and keep it in the same place in the list
map.get(nv).value = value;
}
map.compute(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we will save one hash computation, but have the price of an additional invocation of a lambda expression:
I did this short test:

	@Test
	public void speed() {
		for (long i = 0L; i < 10_000_000L; i++) {
			ht.put("one", 1);
			ht.put("two", 2);
			ht.put("three", 3);
			ht.put("four", 4);
			ht.put("five", 5);
			ht.put("six", 6);
			ht.put("seven", 7);
			ht.put("eight", 8);
			ht.put("nine", 9);
			ht.put("ten", 10);

		}
	}

it runs with the old code in apporx 8,5sec and with the new code approx 5,5 sec

nv,
(k, existing) -> {
if (existing == null) {
// New value -- insert to end of doubly-linked list
if (first == null) {
first = last = nv;
} else {
last.next = nv;
nv.prev = last;
last = nv;
}
return nv;
} else {
// Update the existing value and keep it in the same place in the list
existing.value = value;
return existing;
}
});
}

/**
Expand Down
62 changes: 31 additions & 31 deletions rhino/src/main/java/org/mozilla/javascript/JavaMembers.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -327,9 +327,15 @@ private void discoverAccessibleMethods(

if (isPublic(mods) || isProtected(mods) || includePrivate) {
MethodSignature sig = new MethodSignature(method);
if (!map.containsKey(sig)) {
if (includePrivate) method.trySetAccessible();
map.put(sig, method);
if (includePrivate) {
map.computeIfAbsent(
sig,
k -> {
method.trySetAccessible();
return method;
});
} else {
map.putIfAbsent(sig, method);
}
}
}
Expand All @@ -346,7 +352,7 @@ private void discoverAccessibleMethods(
Method[] methods = clazz.getMethods();
for (Method method : methods) {
MethodSignature sig = new MethodSignature(method);
if (!map.containsKey(sig)) map.put(sig, method);
map.putIfAbsent(sig, method);
}
break; // getMethods gets superclass methods, no
// need to loop any more
Expand Down Expand Up @@ -387,9 +393,7 @@ void discoverPublicMethods(Class<?> clazz, Map<MethodSignature, Method> map) {
static void registerMethod(Map<MethodSignature, Method> map, Method method) {
MethodSignature sig = new MethodSignature(method);
// Array may contain methods with same signature but different return value!
if (!map.containsKey(sig)) {
map.put(sig, method);
}
map.putIfAbsent(sig, method);
}

static final class MethodSignature {
Expand Down Expand Up @@ -595,23 +599,21 @@ private void reflect(
NativeJavaMethod setters = null;
String setterName = "set".concat(nameComponent);

if (ht.containsKey(setterName)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we can safely remove containsKey here, as we call get in the next line, which is checked for null by the instanceOf test.

// Is this value a method?
Object member = ht.get(setterName);
if (member instanceof NativeJavaMethod) {
NativeJavaMethod njmSet = (NativeJavaMethod) member;
if (getter != null) {
// We have a getter. Now, do we have a matching
// setter?
Class<?> type = getter.method().getReturnType();
setter = extractSetMethod(type, njmSet.methods, isStatic);
} else {
// No getter, find any set method
setter = extractSetMethod(njmSet.methods, isStatic);
}
if (njmSet.methods.length > 1) {
setters = njmSet;
}
// Is this value a method?
Object member = ht.get(setterName);
if (member instanceof NativeJavaMethod) {
NativeJavaMethod njmSet = (NativeJavaMethod) member;
if (getter != null) {
// We have a getter. Now, do we have a matching
// setter?
Class<?> type = getter.method().getReturnType();
setter = extractSetMethod(type, njmSet.methods, isStatic);
} else {
// No getter, find any set method
setter = extractSetMethod(njmSet.methods, isStatic);
}
if (njmSet.methods.length > 1) {
setters = njmSet;
}
}
// Make the property.
Expand Down Expand Up @@ -687,13 +689,11 @@ private Field[] getAccessibleFields(boolean includeProtected, boolean includePri
private static MemberBox findGetter(
boolean isStatic, Map<String, Object> ht, String prefix, String propertyName) {
String getterName = prefix.concat(propertyName);
if (ht.containsKey(getterName)) {
// Check that the getter is a method.
Object member = ht.get(getterName);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

same here. Do not hit map twice

if (member instanceof NativeJavaMethod) {
NativeJavaMethod njmGet = (NativeJavaMethod) member;
return extractGetMethod(njmGet.methods, isStatic);
}
// Check that the getter is a method.
Object member = ht.get(getterName);
if (member instanceof NativeJavaMethod) {
NativeJavaMethod njmGet = (NativeJavaMethod) member;
return extractGetMethod(njmGet.methods, isStatic);
}
return null;
}
Expand Down
Loading