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

DelegatingScheduler singletons in modern style #349

Open
magicprinc opened this issue Nov 26, 2022 · 9 comments · May be fixed by #350
Open

DelegatingScheduler singletons in modern style #349

magicprinc opened this issue Nov 26, 2022 · 9 comments · May be fixed by #350

Comments

@magicprinc
Copy link

DelegatingScheduler uses an old singleton idiom with double volatile check and synchronized.
Bill Pugh Singleton Implementation is better, shorter and uses (in some cases) less memory.
Plus fields become "static final" so JVM can do some other optimizations.

@magicprinc
Copy link
Author

magicprinc commented Nov 26, 2022

Plus it creates (if needed) ForkJoinPool with asyncMode=true (which is recommended for normal Runnables and Callables) and it uses Math.max(Runtime.getRuntime().availableProcessors(), 2) for parallelism.
ForkJoinPool.commonPool() can have less parallelism because of 'java.util.concurrent.ForkJoinPool.common.parallelism' is set, and actual number of CPU can be high.

Second commit contains:

  1. bug fix in DelegatingScheduler#schedule: (delay == 0) → (delay <= 0)

  2. fix, if user's executor is actually commonPool() without parallelism. Similar to CompletableFuture.screenExecutor

  3. Use less memory: don't capture variable if executor is commonPool (Lambda can access it without capturing, through static field)

@Tembrel
Copy link
Contributor

Tembrel commented Nov 28, 2022

The associated PR is a lot to take on faith, and it's very hard to test. Is anyone reporting problems due to the current implementation?

@magicprinc
Copy link
Author

magicprinc commented Nov 28, 2022

There are only 225 lines of code.
Now with almost 100% test code coverage (I have written tests to cover all lines, even not "mine").
What line is unclear?

I work with large sets, so every byte in RAM counts.

With last commit, there is no more "fat" in code.
Every byte – is required and works.

@magicprinc
Copy link
Author

magicprinc commented Nov 28, 2022

I report problem so to say :-)
It was too many moving parts:
Double-checked locking singleton pattern is outdated.
8+4+1 extra (unused) bytes per every task (in reality even more: 8 byte object alignment of JVM or even more if 64bit pointers are used).
Full row of checks every time.
Bug with delay: if (delay == 0)
Accepting commonPool that cannot support parallelism (now protected, similar to CompletableFuture)
Extra bonus: now some fields are static final and JVM can inline them and do other optimizations.

@magicprinc
Copy link
Author

Last possible optimization:
Replace juicy DelegatingScheduler.ScheduledCompletableFuture with a lightweight, almost empty implementation. CompletableFuture is quite fat if used only as dumb "placeholder".

@Tembrel
Copy link
Contributor

Tembrel commented Nov 28, 2022

Simply having tests to cover lines of code doesn't give me confidence in the correctness of the code. The risk involved in replacing this machinery without feedback from experts could be substantial, but getting that feedback takes time and attention.

I could understand undertaking it if people were reporting actual problems, e.g., "I ran out of memory because the tasks were too big." "I'm seeing significant performance degradation due to the extra checks." Has anyone experienced problems that might stem from a need to optimize DelegatingScheduler?

If not, this could be premature optimization.

Side note: if (delay == 0) is more of a specification problem in Scheduler, right? Should negative delay values be accepted in the first place?

@magicprinc
Copy link
Author

magicprinc commented Nov 28, 2022

Side note: if (delay == 0) is more of a specification problem in Scheduler, right? Should negative delay values be accepted in the first place?

JDK's ScheduledThreadPoolExecutor accepts them (treats negative values as 0 = run immediately).
In your case, it is a small bug: you schedule then task in the wrong executor.

This is what I mean, when I say: "too many moving parts"

Maybe you could show the code to the devs?

premature optimization

Greta Thunberg won't love you: All this extra work adds to global warming :-)

@Tembrel
Copy link
Contributor

Tembrel commented Nov 28, 2022

I am not a maintainer of this project, so I can't "show the code to the devs" beyond making comments here that interested parties might pick up on.

How about making the if (delay == 0) -> if (delay <= 0) change into a separate PR?

Greta Thunberg would probably be in favor of thinking about whether the extra work isn't dominated by other concerns.

@magicprinc
Copy link
Author

I did a good job.

And I hope the maintainer will find my changes interesting.

It is easy to see the difference: simply generate 1 billion scheduled "events" and see.

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