Hi,
inlining the new method actually messed up microbenchmark scores a
lot (15-80% overhead). I settled for simplifying the duplicate
permsMap.get in the outer method in a way that keeps scores neutral:
http://cr.openjdk.java.net/~redestad/8247995/open.01/
Benchmark Mode Cnt Score Error
Units
PermissionsImplies.withPermission avgt 15 27.857 ± 0.137
ns/op
PermissionsImplies.withUnresolvedPermission avgt 15 27.918 ± 0.178
ns/op
PermissionsImplies.withoutPermission avgt 15 4.559 ± 0.115
ns/op
Thanks!
/Claes
On 2020-06-22 23:54, Claes Redestad wrote:
Hi Roger,
I prototyped it as such, and saw a 0.4ns/op overhead in the
PermissionImplies.withoutPermission. Thinking about it a bit now, this
should be the rare path taken, since it means the permission is not
implied and the performance largely irrelevant. I'll simplify as you
suggested.
/Claes
On 2020-06-22 23:03, Roger Riggs wrote:
Hi Claes,
Its correct as is but I would have written it without duplicating the
'permsMap.get(p.getClass())' invocation
and as a single method (unless the inlining of
getPermissionCollection(p,create)) is important.
A patch on top of yours:
diff --git
a/src/java.base/share/classes/java/security/Permissions.java
b/src/java.base/share/classes/java/security/Permissions.java
--- a/src/java.base/share/classes/java/security/Permissions.java
+++ b/src/java.base/share/classes/java/security/Permissions.java
@@ -229,20 +229,16 @@ implements Serializable
*/
private PermissionCollection getPermissionCollection(Permission p,
boolean createEmpty) {
+ PermissionCollection pc = permsMap.get(p.getClass());
if (!hasUnresolved && !createEmpty) {
// Collection not to be created
- return permsMap.get(p.getClass());
+ return pc;
}
- PermissionCollection pc = permsMap.get(p.getClass());
if (pc != null) {
// Collection already created
return pc;
}
- return createPermissionCollection(p, createEmpty);
- }
- private PermissionCollection
createPermissionCollection(Permission p,
- boolean createEmpty) {
synchronized (permsMap) {
Class<?> c = p.getClass();
PermissionCollection pc = permsMap.get(c);
Thanks, Roger
On 6/22/20 11:04 AM, Claes Redestad wrote:
Hi,
this patch fixes a corner-case performance issue with
Permissions.implies(Permission) by not needing to allocate a mapper
function (or lambda) on each invocation of getPermissionCollection
when there are unresolved permissions present.
Bug: https://bugs.openjdk.java.net/browse/JDK-8247995
Patch: http://cr.openjdk.java.net/~redestad/8247995/open.00/
Testing: tier1-2
Thanks!
/Claes
