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

Fix reloading channel config changes in .items file #4075

Merged
merged 3 commits into from
Feb 4, 2024
Merged

Fix reloading channel config changes in .items file #4075

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2835214
Apply channel config changes in .items file
jimtng Jan 31, 2024
e06dda3
Add a comment about hashmap's initial capacity
jimtng Jan 31, 2024
35277cb
simplify loop
jimtng Jan 31, 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
26 changes: 15 additions & 11 deletions ...model.thing/src/org/openhab/core/model/thing/internal/GenericItemChannelLinkProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
package org.openhab.core.model.thing.internal;

import java.util.Collection;
import java.util.HashMap;
import java.util.HashSet;
import java.util.Map;
import java.util.Optional;
Expand Down Expand Up @@ -45,7 +46,7 @@ public class GenericItemChannelLinkProvider extends AbstractProvider<ItemChannel

private final Logger logger = LoggerFactory.getLogger(GenericItemChannelLinkProvider.class);
/** caches binding configurations. maps itemNames to {@link ItemChannelLink}s */
protected Map<String, Set<ItemChannelLink>> itemChannelLinkMap = new ConcurrentHashMap<>();
protected Map<String, Map<ChannelUID, ItemChannelLink>> itemChannelLinkMap = new ConcurrentHashMap<>();

/**
* stores information about the context of items. The map has this content
Expand Down Expand Up @@ -98,15 +99,20 @@ private void createItemChannelLink(String context, String itemName, String chann
previousItemNames.remove(itemName);
}

Set<ItemChannelLink> links = itemChannelLinkMap.get(itemName);
Map<ChannelUID, ItemChannelLink> links = itemChannelLinkMap.get(itemName);
if (links == null) {
itemChannelLinkMap.put(itemName, links = new HashSet<>());
// Create a HashMap with an initial capacity of 2 (the default is 16) to save memory
// because most items have only one channel. A capacity of 2 is enough to avoid
// resizing the HashMap in most cases, whereas 1 would trigger a resize as soon as
// one element is added.
itemChannelLinkMap.put(itemName, links = new HashMap<>(2));
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
itemChannelLinkMap.put(itemName, links = new HashMap<>(2));
itemChannelLinkMap.put(itemName, links = new HashMap<>(2));

Why 2?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The default is 16 with .75 load factor. That seems like a waste of memory because most items have just one link.

If I set it to one, as soon as one element is added, it would trigger a rehash and capacity expansion anyway, because it would have exceeded the load factor. So 2 is chosen here.

Tbh this is just a guess based on reading about the description of initial capacity and load factor from hashmap's doc. I haven't done actual memory analysis tests to verify.

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've added a comment in the code for future reference

}
if (!links.contains(itemChannelLink)) {
links.add(itemChannelLink);

ItemChannelLink oldLink = links.put(channelUIDObject, itemChannelLink);
if (oldLink == null) {
notifyListenersAboutAddedElement(itemChannelLink);
} else {
notifyListenersAboutUpdatedElement(itemChannelLink, itemChannelLink);
notifyListenersAboutUpdatedElement(oldLink, itemChannelLink);
}
}

Expand All @@ -128,18 +134,16 @@ public void stopConfigurationUpdate(String context) {
}
for (String itemName : previousItemNames) {
// we remove all binding configurations that were not processed
Set<ItemChannelLink> links = itemChannelLinkMap.remove(itemName);
Map<ChannelUID, ItemChannelLink> links = itemChannelLinkMap.remove(itemName);
if (links != null) {
for (ItemChannelLink removedItemChannelLink : links) {
notifyListenersAboutRemovedElement(removedItemChannelLink);
}
links.values().forEach(this::notifyListenersAboutRemovedElement);
}
}
Optional.ofNullable(contextMap.get(context)).ifPresent(ctx -> ctx.removeAll(previousItemNames));
}

@Override
public Collection<ItemChannelLink> getAll() {
return itemChannelLinkMap.values().stream().flatMap(Collection::stream).toList();
return itemChannelLinkMap.values().stream().flatMap(m -> m.values().stream()).toList();
}
}
Loading