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.

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

Lower memory footprint #906

Closed
magicprinc opened this issue Nov 2, 2023 · 7 comments · Fixed by #913
Closed

Lower memory footprint #906

magicprinc opened this issue Nov 2, 2023 · 7 comments · Fixed by #913

Comments

@magicprinc
Copy link
Contributor

magicprinc commented Nov 2, 2023

Please, look at the series of steps, as a result of which I have reduced the amount of used memory by almost 2 times.

https://github.com/magicprinc/smallrye-fault-tolerance/commits/feature/compact-timer

Plus

  • more correct task-comparator behavior
  • support for graceful shutdown in standalone mode (registry of all timers, access to executor)
  • monitoring support (registry of all timers, timer queue size)
    and other micro cleanups (Collections.newSetFromMap(new ConcurrentHashMap<>())→ConcurrentHashMap.newKeySet())

I hope you find these useful
PR: #907

@magicprinc
Copy link
Contributor Author

magicprinc commented Nov 2, 2023

Two further extreme ideas:

  1. Remove ThreadTimer.Task#state at all (use Set.contains and remove): but I don't know how often isDone and cancel are called and how important are they.
  2. long startTime in nanos to int millis
  static final long FIRST_ACCESS_NANOS  = System.nanoTime();
...
  int time = (System.nanoTime() - FIRST_ACCESS_NANOS)>>19; // or / 1_000_000

Alternative option for 1: use two bits of long startTime for state (with -FIRST_ACCESS_NANOS correction, the bits will be always free from time)

@magicprinc
Copy link
Contributor Author

magicprinc commented Nov 2, 2023

Have you tried a custom priority queue (similar to ScheduledThreadPoolExecutor.DelayedWorkQueue) instead of ConcurrentSkipListSet for tasks?
Is there a documentation or benchmarks for this choice?

@Ladicek
Copy link
Contributor

Ladicek commented Nov 7, 2023

I have not tried a different implementation of a priority queue. I chose ConcurrentSkipListSet because it's a sorted set and is lock-free. The binary heap in DelayedWorkQueue needs to be protected by a lock, which is something I didn't want, though it probably doesn't make too much of a difference.

@magicprinc
Copy link
Contributor Author

magicprinc commented Nov 7, 2023

@Ladicek If you have time and interest, you can play with a custom PriorityQueue (early beta/draft/only for testing)
https://github.com/magicprinc/smallrye-fault-tolerance/commits/feature/pqueue

Less memory usage, blocking/non-blocking seems irrelevant in simple tests

The profiler shows that the memory cost of the current implementation (ConcurrentSkipListSet) is negligible compared to, for example, CompletableStage mechanics.

So this is a very theoretical subject.

@Ladicek
Copy link
Contributor

Ladicek commented Nov 8, 2023

I originally chose the ConcurrentSkipListSet because it's lock-free and being a sorted set, it can easily be used as a priority queue. I did realize it's not purpose-built priority queue, but it turned out it works just fine for modest amount of scheduled tasks (see https://github.com/smallrye/smallrye-fault-tolerance/blob/main/implementation/core/src/test/java/io/smallrye/faulttolerance/core/timer/ThreadTimerStressTest.java). So I didn't yet feel the need to replace it with anything else. (Plus, skip list is a really cool data structure!)

@magicprinc
Copy link
Contributor Author

I have tested with 10_000_000 scheduled events and current (ConcurrentSkipListSet) implementations works fine.

@Ladicek
Copy link
Contributor

Ladicek commented Nov 8, 2023

Even better! :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants