Format files with multiple threads - #1041
Conversation
This is a preparatory step for the implementation of concurrent formatting to rule out mutation of the options.
* Adds mojo parameter `threads` to control the number of threads used for formatting. * Java and Javascript formatters use thread locals
ctubbsii
left a comment
ctubbsii
left a comment
There was a problem hiding this comment.
Overall, it looks really good. I have a few suggestions. If you address those, I'd like to test it on a large project to see what the overall improvement is before merging.
One thing I'm not sure about is the thread-safety of the hash cache. Can you point me to the part of the code where it's clear that it's okay to use that concurrently in multiple threads?
| * the cfg | ||
| */ | ||
| void init(Map<String, String> options, ConfigurationSource cfg); | ||
| void init(ImmutableMap<String, String> options, ConfigurationSource cfg); |
There was a problem hiding this comment.
Is it really necessary to change this interface type? This PR can be a lot smaller.
There was a problem hiding this comment.
It is not really necessary to change that interface. However, using the ImmutableMap makes it obvious that these options are not a problem for the thread safety. In principle, we could achieve the same by using ImmutableMap just inside the formatters and taking a copy of the options passed via Formatter#init. Nevertheless, it seems natural to me to require that options are not modified by the initialization. Let me know what you prefer.
There was a problem hiding this comment.
I prefer the smaller diff, and to avoid the Guava-specific type in the APIs. It gives more flexibility for maintenance in the future, and makes it easier to verify the changeset today. I really like Google's implementation of ImmutableMap, with its builders and everything, but I just don't want to be tied to it.
There was a problem hiding this comment.
I added a commit that avoided these changes. A review of the immutability of the configs as they get passed around can be done in a subsequent change with careful consideration. I just don't think it's necessary to do that in this PR, as it's a distraction from reviewing the essential changes.
| /** | ||
| * Number of threads to use for formatting files in parallel. Defaults to {@code 1}, which preserves the historical | ||
| * single-threaded behavior. A value of {@code 0} or any negative value selects a value equal to the number of | ||
| * available processors (i.e. {@link Runtime#availableProcessors()}). Larger projects may benefit substantially from | ||
| * parallel formatting. | ||
| * | ||
| * @since 2.30.0 | ||
| */ | ||
| @Parameter(defaultValue = "1", property = "formatter.threads") | ||
| private int threads; | ||
|
|
There was a problem hiding this comment.
It might be nice to specify 1.5C or 2C or 0.75C to specify the number of cores by some multiplier factor, with the result being a minimum of 1 core.
There was a problem hiding this comment.
I have extended the threads options such that additionally to the fixed number of threads it also permits specifying a multiplier. Is this what you had in mind?
There was a problem hiding this comment.
Yes, exactly. I added a commit that extended your implementation a bit, though, so it's a little more flexible.
| // Html Setup | ||
| if (this.configHtmlFile != null) { | ||
| this.htmlFormatter.init(this.getOptionsFromPropertiesFile(this.configHtmlFile), this); | ||
| this.htmlFormatter.init(ImmutableMap.copyOf(this.getOptionsFromPropertiesFile(this.configHtmlFile)), this); |
There was a problem hiding this comment.
Pretty sure these configs are read only anyway. We don't need to make a copy in an ImmutableMap. We can just make sure that the method returns an immutable type (Collections.unmodifiable wrapper or Map.copyOf())
There was a problem hiding this comment.
The options/configs returned by FormatterMojo#getOptionsFromPropertiesFile are almost read-only. In the case of the XML and the JSON parser the options are extended by an option before being passed to Formatter#init. That is why I did not propagate the ImmutableMap further into getOptionsFromPropertiesFile.
There was a problem hiding this comment.
It's just unnecessary copying. The configs are read directly from their source, and used once when launching the respective formatter right away. They are never written again. I understand that it would be "safer" to pass them around as immutables, but I just don't think it's worth it to make all those extra copies. These are internal APIs only, so I'm not worried about them being modified by another thread. We control all the threads internally that have access to them.
|
@ctubbsii Concerning the thread-safety: The hash cache is a This alone does not yet guarantee thread-safety. However, we use the hash cash in a special way: Each thread processes one file at a time and each file is, of course, processed only once. The hash cache is accessed at the beginning of the processing of a file (in That said, I do not particularly like this construction and it would be a lot cleaner to pass just a file's cached hash to the formatting of that file and to collect all the new hashes after formatting instead of bundling the hashes in the |
* Revert unneeded defensive immutable copies of maps; the configs are read once, optionally modified by the formatter plugin code, and passed directly to the formatter to read. They don't need all that extra copying. * Expand regex for parsing the new parameter, and make it handle more cases (added tests).
|
@jvierling wrote:
@hazendaz knows more about the cache than I do, and would probably be better suited to evaluate that change. That said, I think those changes can be done in a new PR, separate from these. |
2 participants
Implements multi-threaded formatting using a fixed-size thread pool (resolves #537). The number of threads can be configured by a Mojo parameter and defaults to 1. The formatters that may not be thread-safe are instantiated as ThreadLocals. As preparatory step the type of the formatter options are narrowed to ImmutableMap to rule out any mutation.