Change initial capacity of options Hashmap #44
Conversation
Codecov Report
@@ Coverage Diff @@
## master #44 +/- ##
==========================================
Coverage 87.73% 87.73%
- Complexity 0 149 +149
==========================================
Files 23 23
Lines 432 432
Branches 39 28 -11
==========================================
Hits 379 379
Misses 49 49
Partials 4 4
Continue to review full report at Codecov.
|
| @@ -60,7 +60,7 @@ private void addCompileJjdocTaskToProject(Project project, Configuration configu | |||
| } | |||
|
|
|||
| private void addTaskToProject(Project project, Class<? extends AbstractJavaccTask> type, String name, String description, String group, Configuration configuration) { | |||
| Map<String, Object> options = new HashMap<String, Object>(2); | |||
| Map<String, Object> options = new HashMap<String, Object>(3); | |||
There was a problem hiding this comment.
Below, three objects are put into options. Constructor of HashMap which takes an int is a bit confusing, and this change won't guarantee prevention of rehashing, as the description of the PR claims. The misleading constructor of HashMap is described in detail in https://bugs.openjdk.org/browse/JDK-8284377.
JDK-8284377 was released in Java 19, so it is not yet accessible to JavaCC due to max version limited to Java 8 (see README). A more correct way of calculating capacity for a pre-sized HashMap in Java versions before Java 19 is numberOfElements / 0.75, where 0.75 is the default load factor. See, for example, change for JDK-8284377 in HostLocaleProviderAdapterImpl.java.
In this particular case, 3 / 0.75 == 4, although other places in code of JDK also add 1 on top of that:
- Character.java:
private static Map<String, UnicodeBlock> map = new HashMap<>((int)(NUM_ENTITIES / 0.75f + 1.0f)); [...] aliases = new HashMap<>((int)(162 / 0.75f + 1.0f));
- Class.java:
directory = new HashMap<>((int)(universe.length / 0.75f) + 1);
There was a problem hiding this comment.
@rybak how did you find this issue? (It's probably not worth changing again since the impact is negligible, though it's good to know about the discrepancy.)
There was a problem hiding this comment.
I've found out about JDK-8284377 online – it was used as an example of a fairly fast contribution to JDK from a new contributor. It took less than a month between PR open and merge, and it included a CSR, which is a big deal, apparently. I stumbled upon it either on Twitter or the Inside Java podcast.
As for JavaccPlugin.java – I was reading the source of the plugin for $dayjob purposes. The exact reason is offtopic for this PR, but I might come back with a question in form of a new GitHub issue, or even a PR to improve docs, if it will be applicable.
This is a minor change that changes the initial capacity of a hashmap so there won't be a need for a rehashing operation.
Although three entries are added to the
optionshashmap, the initial capacity is set to two.