FELIX-6863: cache ResourceImpl.hashCode()

[stataru8]

Fixes FELIX-6863.

org.apache.felix.utils.resource is embedded in

• org.apache.karaf.features.core-4.4.6.jar
• org.apache.karaf.profile.core-4.4.6.jar

[stataru8]

the conditions logic is copied from the String class in Java 9+

[paulrutter]

Two things:

1. Are the changes made measurable when ran in isolation in a red/green fashion? I would assume Object.hashCode would be highly optimised already
2. Can you add this module to the CI workflow so tests are ran upon PRs??

[stataru8]

Hello, thanks for taking the time to check this PR.

Are the changes made measurable when ran in isolation in a red/green fashion? I would assume Object.hashCode would be highly optimised already

Yes, it's measurable, but only in a specific JVM state. All the details are in FELIX-6863.

For the red/green comparison, the ticket has a reproducer with before/after numbers in the same scenario:

Kars installed	feature:uninstall before restart	after restart	after restart, with the fix
1	~1.3s	~2.3s	~1.4s
300	~4s	~22s	~3.5s

(https://issues.apache.org/jira/secure/attachment/13084874/feature-uninstall-measurements_300_kars.md)

You're right that Object.hashCode is normally very cheap. What I observed is that after a JVM restart, it can end up on a much slower path when called from ResourceImpl.hashCode: in the JFR recordings, almost all samples of the features thread are inside the native Object.hashCode, called from ResourceImpl.hashCode. My understanding is that this depends on how the JIT compiled that call site at startup.

I first ran into this issue on Karaf 4.4.6, and I was able to reproduce it on 4.4.11.

[paulrutter]

Thanks for the change, the caching logic itself looks correct (including the zero-hash handling). Two comments below.

[paulrutter]

Thanks, the changes address my comments. Making caps/reqs private closes the stale-hash gap for subclasses, and I agree there's no need to wrap the returned lists, since the Resource javadoc already says they're unmodifiable. testHashCodeZero now covers the zero-hash path, and the counting requirement makes the caching explicit. One small thing to check: if any downstream subclass (e.g. in Karaf) uses the formerly protected caps/reqs fields, it will stop compiling, so it may be worth a quick look there. Otherwise LGTM.

[stataru8]

In the sources of https://mvnrepository.com/artifact/org.osgi/osgi.core/8.0.0, I still see "An unmodifiable list" in org.osgi.resource.Resource.
When checking the code, I was under the impression that the code intentionally doesn't respect the interface contract for performance reasons.

I'll try to propose a bump of this artifact in Karaf.

[paulrutter]

Agreed, and thanks for checking the osgi.core 8.0.0 sources. The contract is broken in ResourceImpl.getCapabilities(String) and getRequirements(String). With a null namespace they return the live internal caps / reqs list, while Resource documents the result as "an unmodifiable list". With a non-null namespace they return a fresh copy, so only the null case is affected. That behaviour predates this PR; it comes from the original "optimized resource" implementation (5a07d17).

This PR doesn't introduce the violation, but it makes it matter: the cached hash is only invalidated by the add* methods, so a caller that mutates the list returned for null bypasses the invalidation. I'm fine with leaving it as is, since returning the live list avoids an allocation per call. It may be worth a short comment on those two methods saying the null-namespace result is the live list and must not be modified.

Bumping this in Karaf sounds right, since it's the downstream consumer and the place a subclass using the formerly protected fields would break.

https://repository.apache.org/content/repositories/snapshots/org/apache/felix/org.apache.felix.utils/