8193031: Collections.addAll is likely to perform worse than Collection.addAll #1764
Conversation
|
|
stsypanov
changed the title
stsypanov
changed the title
|
@stsypanov The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
|
/issue JDK-8193031 |
openjdk
bot
changed the title
|
@stsypanov The primary solved issue for a PR is set through the PR title. Since the current title does not contain an issue reference, it will now be updated. |
|
Looks like I've found the original ticket: https://bugs.openjdk.java.net/browse/JDK-8193031 |
Webrevs
|
| @@ -5587,11 +5586,9 @@ public static boolean disjoint(Collection<?> c1, Collection<?> c2) { | |||
| * @since 1.5 | |||
| */ | |||
| @SafeVarargs | |||
| @SuppressWarnings("varargs") | |||
There was a problem hiding this comment.
I don't think you need a SuppressWarnings here
There was a problem hiding this comment.
Hi, without this I get failed build:
Compiling 3057 files for java.base
Compiling 89 properties into resource bundles for java.desktop
/home/s.tsypanov/IdeaProjects/jdk-github/src/java.base/share/classes/java/util/Collections.java:5590: warning: [varargs] Varargs method could cause heap pollution from non-reifiable varargs parameter elements
return c.addAll(Arrays.asList(elements));
^
error: warnings found and -Werror specified
1 error
1 warning
make[3]: *** [/home/s.tsypanov/IdeaProjects/jdk-github/build/linux-x86_64-server-release/jdk/modules/java.base/_the.java.base_batch] Error 1
CompileJavaModules.gmk:604: recipe for target '/home/s.tsypanov/IdeaProjects/jdk-github/build/linux-x86_64-server-release/jdk/modules/java.base/_the.java.base_batch' failed
make[2]: *** [java.base-java] Error 2
make[2]: *** Waiting for unfinished jobs....
make/Main.gmk:193: recipe for target 'java.base-java' failed
There was a problem hiding this comment.
ok, it means there is a bug in the compiler, the analysis done for unsafe varargs (with -Xlint:varargs) doesn't check that the called method (here Arrays.asList()) is tagged with @SafeVarargs.
There was a problem hiding this comment.
Should I then wait for the fix of that bug to remove @SupressWarnings?
There was a problem hiding this comment.
I don't think so, the code is fine, for me.
There was a problem hiding this comment.
I've looked into com.sun.tools.javac.comp.Check and it seems the warning is produced here:
@Override
public void warn(LintCategory lint) {
boolean warned = this.warned;
super.warn(lint);
if (warned) return; // suppress redundant diagnostics
switch (lint) {
case UNCHECKED:
Check.this.warnUnchecked(pos(), Warnings.ProbFoundReq(diags.fragment(uncheckedKey), found, expected));
break;
case VARARGS:
if (method != null &&
method.attribute(syms.trustMeType.tsym) != null &&
isTrustMeAllowedOnMethod(method) &&
!types.isReifiable(method.type.getParameterTypes().last())) {
Check.this.warnUnsafeVararg(pos(), Warnings.VarargsUnsafeUseVarargsParam(method.params.last()));
}
break;
default:
throw new AssertionError("Unexpected lint: " + lint);
}
}Can we somehow read @SaveVarargs from signature of Arrays.asList() to avoid the warning?
There was a problem hiding this comment.
@forax I've sent a mail into compiler-dev list: http://mail.openjdk.java.net/pipermail/compiler-dev/2020-December/015857.html
Let's see what they decide.
There was a problem hiding this comment.
FYI, despite the confusing (historical) name, syms.trustMeType is the SafeVarargs annotation. See Symtab.java, line 579.
|
Apart from the @SuppressWarnings, this looks good to me. |
|
Mailing list message from Simon Roberts on core-libs-dev: When updating this, might you take the opportunity to remove the ambiguous A more specific comment on the lines of "Collection.addAll is likely to run Perhaps that's the intended solution anyway, in which case, I apologize for On Thu, Dec 17, 2020 at 3:38 AM R?mi Forax <github.com+ -- |
|
Mailing list message from Сергей Цыпанов on core-libs-dev: Hi Simon, I've removed 'but this method is likely to run significantly faster under most implementations.' so the doc is cleaner now Cheers, 17.12.2020, 16:38, "Simon Roberts" <simon at dancingcloudservices.com>:
|
|
Hi all, According to the document of SafeVarargs.
The If you still think it is the bug of compiler. I suggest that you move the bug discussion to the compiler-dev@openjdk.java.net to solve the bug as soon as possible. |
|
|
|
This message is purely informational: I may have found a JBS comment that provides historical context for that "this method is likely to run significantly faster under most implementations" phrase. Here: https://bugs.openjdk.java.net/browse/JDK-4822887?focusedCommentId=12241154&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-12241154 |
|
@pavelrappo the problem is that people still believe this message and use this utility method widely: see e.g. spring-projects/spring-framework#23478 They expect it to improve performance, but in fact it degradates. Also for some cases it degrade dramatically, e.g. when instead of |
|
@pavelrappo also see this not very old comment: spring-projects/spring-framework#24636 (review) |
|
"This message" referred to the entirety of that very comment of mine #1764 (comment) I prepended that message with that clause (that is, the wording to the left of the colon) to make it clear that I didn't want to review or argue with anything said before that in that thread; it seems that clause made more harm than good. |
|
Hi,
So I would preferably do one of two things here. Either change the javadoc to describe current behaviour better, or use some other immutable List wrapper different than Arrays.asList() which is mutable. WDYT? |
|
Hint: you could use |
|
On a second thought, using |
|
@plevart hi, I've decided to revert implementation change and keep only the change in JavaDoc because it's performance-related part is often referenced to while being incorrect in most of cases. |
|
Hi @stsypanov,
What about:
...since it is not entirely identical. The outcome is usually identical because collections usually adhere to the specification, but we can not claim the behaviour is identical if the target collection does not adhere. |
|
...but we could employ this method to guarantee more than
This means that the method guarantees that the passed in |
|
@plevart done. Should I also rename this PR to e.g. 'Fix performance-related notation in Collections.addAll'? |
I think the title of JIRA ticket has to be the same as that of PR (with JIRA bug number prepended) in order to pass all checks needed for integrating. So If you modify both, it would be fine. |
|
I cannot modify JIRA ticket without account there, so let's keep it as is |
|
|
@stsypanov This change now passes all automated pre-integration checks. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 7 new commits pushed to the
Please see this link for an up-to-date comparison between the source branch of this pull request and the As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@plevart) but any other Committer may sponsor as well.
|
|
/integrate |
|
@stsypanov |
|
Hearing no objections, I'll sponsor this for Sergei. |
|
/sponsor |
openjdk
bot
added
integrated
|
@plevart @stsypanov Since your change was applied there have been 26 commits pushed to the
Your commit was automatically rebased without conflicts. Pushed as commit 27a39c8. |
Hello, I feel like this was previously discussed in https://mail.openjdk.java.net/pipermail/core-libs-dev/ but since I cannot find original mail I post this here.
Currently
Collections.addAll()is implemented and documented as:But it practice the notation
this method is likely to run significantly faster under most implementationsis completely wrong. When I take this benchmark and run it on JDK 14 I get the following results:There's never a case when
Collections.addAllis fater - on the contraryc.addAll(Arrays.asList())always wins. Pay attention especially to dramatic difference for array-based collection.So I propose to reimplement the method by simply delegating to
Arrays.asListbecause the spec declares identical behaviour and to remove perfomance notation from JavaDoc.Progress
Issue
Reviewers
Download
$ git fetch https://git.openjdk.java.net/jdk pull/1764/head:pull/1764$ git checkout pull/1764