You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@felix.apache.org by "stataru8 (via GitHub)" <gi...@apache.org> on 2026/10/01 18:52:52 UTC
[PR] FELIX-6863: cache ResourceImpl.hashCode() [felix-dev]
stataru8 opened a new pull request, #564:
URL: https://github.com/apache/felix-dev/pull/564
`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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@felix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FELIX-6863: cache ResourceImpl.hashCode() [felix-dev]
Posted by "paulrutter (via GitHub)" <gi...@apache.org>.
paulrutter merged PR #564:
URL: https://github.com/apache/felix-dev/pull/564
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@felix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FELIX-6863: cache ResourceImpl.hashCode() [felix-dev]
Posted by "paulrutter (via GitHub)" <gi...@apache.org>.
paulrutter commented on PR #564:
URL: https://github.com/apache/felix-dev/pull/564#issuecomment-5938745109
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??
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@felix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FELIX-6863: cache ResourceImpl.hashCode() [felix-dev]
Posted by "stataru8 (via GitHub)" <gi...@apache.org>.
stataru8 commented on PR #564:
URL: https://github.com/apache/felix-dev/pull/564#issuecomment-5955464874
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](https://issues.apache.org/jira/browse/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**.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@felix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FELIX-6863: cache ResourceImpl.hashCode() [felix-dev]
Posted by "paulrutter (via GitHub)" <gi...@apache.org>.
paulrutter commented on code in PR #564:
URL: https://github.com/apache/felix-dev/pull/564#discussion_r4181505129
##########
utils/src/main/java/org/apache/felix/utils/resource/ResourceImpl.java:
##########
@@ -129,7 +141,16 @@ public boolean equals(Object o) {
@Override
public int hashCode() {
- return Objects.hash(caps, reqs);
+ int h = hash;
Review Comment:
The cache is only invalidated in the four add* methods, but `caps` and `reqs` are protected and `getCapabilities(null)` / `getRequirements(null)` return the live internal lists. If a subclass or caller mutates them directly after `hashCode()` has been called, the cached hash no longer matches `equals` (which still compares the lists), and the resource can get lost in a HashSet/HashMap. Before this change the hash was always recomputed, so this is new.
Suggestion: return an unmodifiable view from the no-namespace case, e.g. `Collections.unmodifiableList(caps)` / `Collections.unmodifiableList(reqs)`, so the add* methods are the only way to mutate and the invalidation is reliable. (Subclasses can still reach the protected fields, so those would ideally be private or documented as not to be mutated directly.)
##########
utils/src/test/java/org/apache/felix/utils/resource/ResourceImplTest.java:
##########
@@ -0,0 +1,199 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.felix.utils.resource;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotEquals;
+import static org.junit.Assert.assertTrue;
+
+import java.util.*;
+import org.junit.Test;
+import org.osgi.framework.Version;
+import org.osgi.framework.namespace.IdentityNamespace;
+import org.osgi.resource.Capability;
+import org.osgi.resource.Requirement;
+
+public class ResourceImplTest {
+
+ private static CapabilityImpl newCapability(ResourceImpl res, String ns) {
+ return new CapabilityImpl(res, ns, new HashMap<>(), new HashMap<>());
+ }
+
+ private static RequirementImpl newRequirement(ResourceImpl res, String ns) {
+ return new RequirementImpl(res, ns, new HashMap<>(), new HashMap<>());
+ }
+
+ /**
+ * Wrapper method to avoid direct field reference
+ */
+ private static List<Capability> getCaps(ResourceImpl res) {
+ return res.getCapabilities(null);
+ }
+
+ /**
+ * Wrapper method to avoid direct field reference
+ */
+ private static List<Requirement> getReqs(ResourceImpl res) {
+ return res.getRequirements(null);
+ }
+
+ private static int expectedHash(ResourceImpl res) {
Review Comment:
None of the tests reach the `h == 0` / `hashIsZero` branch, which is the only non-trivial logic added. An empty resource hashes to 993, so it is never hit. It would be worth covering, e.g. with a Capability/Requirement stub whose `hashCode()` is chosen so `Objects.hash(caps, reqs)` is 0, asserting repeated calls return 0.
Also, the `assertNotEquals` checks on hash values assume no collisions. Capability/Requirement hashes are identity-based here, so there is a tiny chance of a flaky failure.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@felix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FELIX-6863: cache ResourceImpl.hashCode() [felix-dev]
Posted by "stataru8 (via GitHub)" <gi...@apache.org>.
stataru8 commented on code in PR #564:
URL: https://github.com/apache/felix-dev/pull/564#discussion_r4159266342
##########
utils/src/main/java/org/apache/felix/utils/resource/ResourceImpl.java:
##########
@@ -129,7 +141,16 @@ public boolean equals(Object o) {
@Override
public int hashCode() {
- return Objects.hash(caps, reqs);
+ int h = hash;
Review Comment:
the conditions logic is copied from the String class in Java 9+
##########
utils/src/main/java/org/apache/felix/utils/resource/ResourceImpl.java:
##########
Review Comment:
not worth protecting the hash here, in the Resource interface for this method:
```
Returns:
An unmodifiable list containing the declared Capabilitys from the specified namespace. The returned list will be empty if this resource declares no capabilities in the specified namespace.
```
##########
utils/src/main/java/org/apache/felix/utils/resource/ResourceImpl.java:
##########
Review Comment:
the same applies to `getRequirements`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@felix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FELIX-6863: cache ResourceImpl.hashCode() [felix-dev]
Posted by "stataru8 (via GitHub)" <gi...@apache.org>.
stataru8 commented on PR #564:
URL: https://github.com/apache/felix-dev/pull/564#issuecomment-6016460389
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@felix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FELIX-6863: cache ResourceImpl.hashCode() [felix-dev]
Posted by "paulrutter (via GitHub)" <gi...@apache.org>.
paulrutter commented on PR #564:
URL: https://github.com/apache/felix-dev/pull/564#issuecomment-6016527461
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 (5a07d17328).
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@felix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FELIX-6863: cache ResourceImpl.hashCode() [felix-dev]
Posted by "paulrutter (via GitHub)" <gi...@apache.org>.
paulrutter commented on PR #564:
URL: https://github.com/apache/felix-dev/pull/564#issuecomment-6015958222
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@felix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org