zsmartsystems / com.zsmartsystems.zigbee

ZigBee Cluster Library Java framework supporting multiple dongles
Eclipse Public License 1.0
138 stars 87 forks source link

Optimize thread pools to spare resource consumption #1392

Closed amitjoy closed 1 year ago

amitjoy commented 1 year ago

Is your feature request related to a problem? Please describe. A leak has been encountered in which a scheduled thread pool comprises > 400.000 tasks and > 300.000 tasks were in cancelled state but still remained in the thread pool. After analyzing the heapdump, it is recommended to optimize the thread pools efficiently by introducing several thread pool executor flags.

Describe the solution you'd like The following flags should be set to better optimize an executor:

Describe alternatives you've considered Usual way of creating threads out of a thread pool is not optimized for memory constrained devices, hence, there is no other alternative than configuring the aforementioned options for thread pools

Additional context The changes are required to be introduced in ZigBeeExecutors

amitjoy commented 1 year ago

FYI @cdjackson @TomTravaglino

ViToni commented 1 year ago

Thanks @amitjoy for raising this issue. After a bit of research I found one potential cause, which might be resulting in the issue observed. The ScheduledExecutors created by ZigBeeExecutors use the default ScheduledExecutor which does not remove on cancel by default, see here. Maybe setting this policy would already solve the issue.

amitjoy commented 1 year ago

@ViToni Yeah, I have already specified this policy in the issue description as well. This will certainly be useful to spare memory consumption to a greater extent.

ViToni commented 1 year ago

I have a theorie what happened:

  1. Before #1381 there was a somehow unconditinal creation of a future task in https://github.com/zsmartsystems/com.zsmartsystems.zigbee/blob/ef09ca1b772bf9b4f3154de534ab3c1c7290ea8c/com.zsmartsystems.zigbee/src/main/java/com/zsmartsystems/zigbee/transaction/ZigBeeTransactionManager.java#L696-L707 when the queues were empty.
  2. This future task was scheduled with a delay of Long.MAX_VALUE millis.
  3. On the next send the task would have been cancelled and a new one created.
  4. Go to 1.

As ScheduledThreadPoolExecutor retains cancelled tasks until they would be due to avoid potential expensive clean up operations of the backlog, the cancelled task would linger there "forever".

The change #1381 avoids creating tasks when all transaction queue are empty, but the ScheduledThreadPoolExecutors still retain cancelled tasks. Hopefully that's fixed by #1393.

cdjackson commented 1 year ago

Thanks @amitjoy and also @ViToni for the PR. Hopefully that will fix this issue...