jbonofre commented on code in PR #739:
URL: https://github.com/apache/camel-karaf/pull/739#discussion_r3872608047
##########
core/camel-core-osgi/src/main/java/org/apache/camel/karaf/core/OsgiTypeConverter.java:
##########
@@ -238,10 +314,13 @@ public TypeConverterExists getTypeConverterExists() {
@Override
public void setTypeConverterExists(TypeConverterExists
typeConverterExists) {
- getDelegate().setTypeConverterExists(typeConverterExists);
+ register(TYPE_CONVERTER_EXISTS_KEY, registry ->
registry.setTypeConverterExists(typeConverterExists));
}
- public DefaultTypeConverter getDelegate() {
+ // fully synchronized rather than double checked: the delegate is not
immutable after publication -
+ // removedService stops and replaces it - so a lock free read of the field
buys a race for no real gain,
+ // conversion work dwarfing an uncontended monitor either way
+ public synchronized DefaultTypeConverter getDelegate() {
Review Comment:
**Making `getDelegate()` fully synchronized turns the conversion hot path
into a global serialization point, and blocks every conversion in the container
for the full duration of a rebuild.**
`convertTo`, `tryConvertTo`, `mandatoryConvertTo`, `lookup`, `allowNull`,
`size`, `getStatistics` and `getInjector` all funnel through here. Previously
`delegate` was `volatile` and read without a lock, so the hot path was
contention-free; now every conversion on every route thread contends one
monitor (and biased locking is gone as of JDK 15, so it inflates).
The rebuild case is the sharper one: when a single `TypeConverterLoader`
unregisters, the next `getDelegate()` holds this monitor across `init()` +
`loadCoreAndFastTypeConverters()` (bundle resource scanning) + `loader.load()`
for every tracked loader + the replay. Every conversion in the container blocks
for that whole span. `registrationCannotInterleaveWithARebuild` demonstrates
precisely this with its 2 s stall.
The comment's "conversion work dwarfing an uncontended monitor" only holds
while uncontended. Keeping a volatile fast-path read and synchronizing only the
build gives the same build-once guarantee at no hot-path cost:
```java
DefaultTypeConverter d = delegate;
if (d != null) {
return d;
}
synchronized (this) { ... }
```
##########
core/camel-core-osgi/src/main/java/org/apache/camel/karaf/core/OsgiTypeConverter.java:
##########
@@ -238,10 +314,13 @@ public TypeConverterExists getTypeConverterExists() {
@Override
public void setTypeConverterExists(TypeConverterExists
typeConverterExists) {
- getDelegate().setTypeConverterExists(typeConverterExists);
+ register(TYPE_CONVERTER_EXISTS_KEY, registry ->
registry.setTypeConverterExists(typeConverterExists));
}
- public DefaultTypeConverter getDelegate() {
+ // fully synchronized rather than double checked: the delegate is not
immutable after publication -
+ // removedService stops and replaces it - so a lock free read of the field
buys a race for no real gain,
+ // conversion work dwarfing an uncontended monitor either way
+ public synchronized DefaultTypeConverter getDelegate() {
Review Comment:
**Making `getDelegate()` fully synchronized turns the conversion hot path
into a global serialization point, and blocks every conversion in the container
for the full duration of a rebuild.**
`convertTo`, `tryConvertTo`, `mandatoryConvertTo`, `lookup`, `allowNull`,
`size`, `getStatistics` and `getInjector` all funnel through here. Previously
`delegate` was `volatile` and read without a lock, so the hot path was
contention-free; now every conversion on every route thread contends one
monitor (and biased locking is gone as of JDK 15, so it inflates).
The rebuild case is the sharper one: when a single `TypeConverterLoader`
unregisters, the next `getDelegate()` holds this monitor across `init()` +
`loadCoreAndFastTypeConverters()` (bundle resource scanning) + `loader.load()`
for every tracked loader + the replay. Every conversion in the container blocks
for that whole span. `registrationCannotInterleaveWithARebuild` demonstrates
precisely this with its 2 s stall.
The comment's "conversion work dwarfing an uncontended monitor" only holds
while uncontended. Keeping a volatile fast-path read and synchronizing only the
build gives the same build-once guarantee at no hot-path cost:
```java
DefaultTypeConverter d = delegate;
if (d != null) {
return d;
}
synchronized (this) { ... }
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
