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

add option not to start any threads #10

Closed
subes opened this issue Sep 30, 2021 · 11 comments
Closed

add option not to start any threads #10

subes opened this issue Sep 30, 2021 · 11 comments

Comments

@subes
Copy link

subes commented Sep 30, 2021

please provide an option to disable startup of any threads:
grafik
I am using:

if (StaticComponentContainer.Modules != null) {
    StaticComponentContainer.Modules.exportAllToAll();
}

And afterwards there is no way to shutdown all threads. I don't see an option to prevent the threads from being started as well. Maybe start the threads lazy the first time when a BackgroundExecutor call is made? Or provide a BackgroundExecutor implementation that directly invokes runnables instead of delegating to threads. Or allow someone to provide his own implementation for the BackgroundExecutor that works without threads. Same applies to FileSystemHelper.

@subes
Copy link
Author

subes commented Sep 30, 2021

Or just provide an option to disable individual classes from being initialized in StaticComponentContainer that spawn threads (fields for these components will stay null). If user wants to use threads, he has to initialize his own ComponentContainer.

org.burningwave.core.classes.Modules.create().exportAllToAll();

does not work as well because any instance of Modules initializes the StaticComponentContainer.

@Roberto-Gentili
Copy link
Member

Sorry but except for the task monitoring thread which can be closed by calling StaticComponentContainer.BackgroundExecutor.stopAllTasksMonitoring () or by setting the property background-executor.all-tasks-monitoring.enabled in the burningwave.static.properties file to false, the other threads are critical to the functioning of Burningwave Core and cannot be stopped. However, 3 of these 4 threads are required by the BackgroundExecutor to start developer-created tasks and are in a wait state until the developer creates a new task and do not consume resources . The "File Scavenger" thread is a low priority thread needed to clean up temporary files generated by Burningwave Core. Also, all threads created by Burningwave are daemon threads and therefore non-blocking.

@subes
Copy link
Author

subes commented Sep 30, 2021

Or maybe make the functionality of org.burningwave.core.classes.Modules.create().exportAllToAll(); available in https://github.com/toolfactory/jvm-driver ?

@subes
Copy link
Author

subes commented Sep 30, 2021

Also this is causing unnecessary load during idle:
grafik

@Roberto-Gentili
Copy link
Member

Roberto-Gentili commented Sep 30, 2021

I will work to make possible to disable the autostart of these threads from the configuration file. When I'll finish I will answer here: stay tuned

@Roberto-Gentili
Copy link
Member

Roberto-Gentili commented Oct 1, 2021

Solved with the introduction of the deferred initialization of the BackgroundExecutor in the version just released (10.0.0): now the threads are started only when the library or developer creates tasks via the BackgroundExecutor. You need also to add these properties to your burningwave.static.properties file:

priority-of-this-configuration-file=1
background-executor.all-tasks-monitoring.enabled=false
modules.export-all-to-all=false
static-component-container.on-close.close-all-component-containers=false
static-component-container.on-close.close-file-system-helper=false

I wait your feedback

@subes
Copy link
Author

subes commented Oct 1, 2021

looks good, thanks!
grafik

Off Topic
A consideration about performance/design:

  • using lists and initializing iterable/iterators for each reflection call is a bit much overhead => instead use the arrays that are provided
  • doing lots of small things inside executors in parallel also causes quite some overhead, it is often better to parallelize larger tasks that are split by chunks, or avoid parallelization if not really needed
    • also an application should have the responsibility about threads being started/stopped and managed, a library (especially a library for libraries :D ) should not impose anything like that, only provide options and allow overrides
    • this should still hold true with fibers, just that fibers make it a bit less costly and handle I/O waits very well
  • having one "burningwave.static.properties" read from the classpath is a bit limiting, imagine an application that uses multiple libraries that use different "burningwave.static.properties". Which settings should be chosen? Thus it would be better to encapsulate this somehow.

Thus the library is good for one off tasks, but to repetitively call the same method/field accessor it is better to reduce the overhead as much as possible.

Anyhow, great work with the significant JVM hacks! Adding add-opens during development/testing is quite bothersome.

@subes subes closed this as completed Oct 1, 2021
@Roberto-Gentili
Copy link
Member

Roberto-Gentili commented Oct 1, 2021

Burningwave Core caches the collection of the member retrieved through reflection and the cache can also be cleared, for the problem of multiple burningwave.static.properties file you can solve through the new property priority-of-this-configuration that must be inserted in the file: Burningwave Core merges all the files found giving priority to the file based on this new property. As for the speech thread, it is possible to control its number and priority through the ThreadSupplier that continuously expands and contracts as defined in the configuration.

Thanks for the feedback!

@subes
Copy link
Author

subes commented Oct 1, 2021

I guess the priority is a fine solution. Thanks for clarifying.

Initializing an iterator on each access is still an allocation on the path of a simple call.
Looking up a cached value from a map is also slower than statically remembering the accessor in client code.
But these things can be unwrapped/optimized for hot paths by the client.

@Roberto-Gentili
Copy link
Member

Do you suggest using array then?

@subes
Copy link
Author

subes commented Oct 1, 2021

I think the internal storage would not matter much as long as the user is able to lookup the methodhandle/box, store that in a variable and use that for calling afterwards (without any more allocations, lookups, hashcode calculations).

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

No branches or pull requests

2 participants