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.

Sign up for GitHub

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

Jump to bottom

Change initial capacity of options Hashmap #44

Merged
merged 1 commit into from
Sep 16, 2018
Merged

Change initial capacity of options Hashmap #44

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion subprojects/plugin/src/main/java/ca/coglinc/gradle/plugins/javacc/JavaccPlugin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


options.put(Task.TASK_TYPE, type);
options.put(Task.TASK_DESCRIPTION, description);
Expand Down