8253089: Windows (MSVC 2017) build fails after JDK-8243208

[shipilev]

It seems that MSVC 2017 is getting confused about the differences in unsigned int and u2. After a few attempts at fixing this, I think we need to use u2 consistently for hash code computations. u2 is the final storage type for the hash code in JVMFlagLookup::_hashes.

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed

Issue

• JDK-8253089: Windows (MSVC 2017) build fails after JDK-8243208

Reviewers

• Martin Doerr (@TheRealMDoerr - Reviewer)
• Goetz Lindenmaier (@GoeLin - Reviewer)
• Ioi Lam (@iklam - Reviewer)

Download

$ git fetch https://git.openjdk.java.net/jdk pull/150/head:pull/150
$ git checkout pull/150

[bridgekeeper]

👋 Welcome back shade! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@shipilev The following label will be automatically applied to this pull request: hotspot-runtime.

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 (add|remove) "label" command.

[mlbridge]

Webrevs

• 00: Full (a62ebff)

[GoeLin]

Hi,
this fix works in our CI. Looks good.
Best regards,
Goetz.

[openjdk]

@shipilev This change now passes all automated pre-integration checks. In addition to the automated checks, the change must also fulfill all project specific requirements

After integration, the commit message will be:

8253089: Windows (MSVC 2017) build fails after JDK-8243208

Reviewed-by: mdoerr, goetz, iklam

• If you would like to add a summary, use the /summary command.
• To credit additional contributors, use the /contributor command.
• To add additional solved issues, use the /issue command.

Since the source branch of this PR was last updated there have been 9 commits pushed to the master branch:

• af8c678: 8247910: Improve alignment and power-of-2 utilities using C++14
• 70cc7fc: 8253098: Archived full module graph should be disabled if CDS heap cannot be mapped
• ac9d1b0: 8223187: Remove setLocale() call in jpackage native launcher
• 9c24a56: 8253029: [PPC64] Remove obsolete Power6 code
• e6a493a: 8252882: Clean up jdk.javadoc and the related parts of jdk.compiler
• 68da63d: 8240658: Code completion not working for lambdas in method invocations that require type inference
• b05290a: 8252898: remove bulk registration of JFR CompilerPhaseType names
• 779d2c3: 8253084: Zero VM is broken after JDK-8252689
• 07da3a1: 8253030: ZGC: Change ZMarkCompleteTimeout unit to microseconds

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid automatic rebasing, please merge master into your branch, and then specify the current head hash when integrating, like this: /integrate af8c678038c2a4788e287793bacdf1048634f3bf.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[GoeLin]

LGTM, Goetz.

[mlbridge]

Mailing list message from Kim Barrett on hotspot-runtime-dev:

I suspect the compiler warning is an example of
https://developercommunity.visualstudio.com/content/problem/211134/unsigned-integer-overflows-in-constexpr-functionsa.html

This would explain why using u2 would dodge the warning. The currently
overflowing arithmetic operation would be on the promoted u2 values, so
won't overflow.

I think changing to using u2 isn't the right solution though. This throws
away a lot of bits in the hash-code calculation, potentially making it less
effective. I think a better workaround is to locally suppress the warning,
which I think is from the multiplication here:

61 h = 31*h + (unsigned int) *s;

So I suggest wrapping that line with targeted warning suppression, i.e.
using PRAGMA_DIAG_PUSH/POP and PRAGMA_DISABLE_MSVC_WARNING(4307) with an
appropriate comment. I don't love cluttering code with that kind of thing,
but I think in this case that's a better solution than changing what's
being calculated.

Note that it might be that such a limited warning disable is insufficient in
the long run. This seems like a problem that might arise in other places as
we make more use of constexpr. We might instead need to globally disable
that warning for some limited range of VS versions.

[iklam]

I agree with Kim that it's better to disable the warning.

[mlbridge]

Mailing list message from Aleksey Shipilev on hotspot-runtime-dev:

On 9/14/20 6:05 PM, Kim Barrett wrote:

I think changing to using u2 isn't the right solution though. This throws
away a lot of bits in the hash-code calculation, potentially making it less
effective.

I think this accuracy concern does not apply here, because it does not actually change the computation.

This is because polynomial hash codes enjoy the modulo distributivity, computation uses unsigned
(non-negative) values, and the fact that the result is finally stored in u2, cutting out whatever
upper bits hash code had accumulated.

A bit more rigorously:

M = 2^(sizeof(u2)*8) = 16
K = 2^(sizeof(unsigned int)*8) = 32 // assume int is 32 bit

L1: note M and K are power of two, and K=2*M, so (x % K) % M = (x % M) here
(Translation: cutting out K lower bits, and then M (M < K) lower bits is equivalent to
cutting out M lower bits right away)

new_hashcode = sum_i( (31^k * s[i]) % M) % M

old_hashcode = sum_i( (31^k * s[i]) % K) % M
= sum_i(((31^k * s[i]) % K) % M) % M // modulo distributivity
= sum_i( (31^k * s[i]) % M) % M // by L1
= new_hashcode

Or try this ;)
https://cr.openjdk.java.net/~shade/8253089/mod-hashcode.cpp

I think a better workaround is to locally suppress the warning,
which I think is from the multiplication here:

61 h = 31*h + (unsigned int) *s;

So I suggest wrapping that line with targeted warning suppression, i.e.
using PRAGMA_DIAG_PUSH/POP and PRAGMA_DISABLE_MSVC_WARNING(4307) with an
appropriate comment. I don't love cluttering code with that kind of thing,
but I think in this case that's a better solution than changing what's
being calculated.

I think math thankfully saves us from cluttering the code with compiler pragmas.

--
Thanks,
-Aleksey

[mlbridge]

Mailing list message from Aleksey Shipilev on hotspot-runtime-dev:

On 9/14/20 7:27 PM, Aleksey Shipilev wrote:

On 9/14/20 6:05 PM, Kim Barrett wrote:

I think changing to using u2 isn't the right solution though. This throws
away a lot of bits in the hash-code calculation, potentially making it less
effective.

I think this accuracy concern does not apply here, because it does not actually change the computation.

[...]

Or try this ;)
https://cr.openjdk.java.net/~shade/8253089/mod-hashcode.cpp

Even more: jvmFlagLookup.o files before and after are bit-to-bit identical. (I objdumped to look at
.rodata section to see _flag_lookup_table is there).

--
Thanks,
-Aleksey

[iklam]

TL/DR; I think the proposed change is acceptable as it doesn't significantly change the bucket size distribution.

====
I added a patch to show the bucket size distribution:
http://cr.openjdk.java.net/~iklam/jdk16/8253089-jvmFlagLookup-u2-msvc/buckets_size.patch.txt

I then apply the proposed change on top. The contents of the hashtable is changed (I don't know why Aleksey sees no changes; I used output from gcc -save-temps). Even the lower 16 bit of the hash may not change, this expression will give different results depending on the upper 16 bits:

int bucket_index = (int)(hash % NUM_BUCKETS);

Anyway, the bucket size distribution is about the same as before. The max bucket size is about 12.

http://cr.openjdk.java.net/~iklam/jdk16/8253089-jvmFlagLookup-u2-msvc/jvmFlagLookup.s.old.txt
http://cr.openjdk.java.net/~iklam/jdk16/8253089-jvmFlagLookup-u2-msvc/jvmFlagLookup.s.new.txt

Scroll down to here: the first 277 entries are the bucket size.

_ZL18_flag_lookup_table:
	.value	2
	.value	8
	.value	3
	.value	5
	.value	10
	.value	1
	.value	7
	.value	8
	.value	3
	.value	3
	.value	5
	.value	2
	.value	5
	.value	7
	.value	6
	.value	2
	.value	5
	.value	2
	.value	6
	.value	3
	.value	8
	.value	1
	.value	3

[shipilev]

/integrate

[openjdk]

@shipilev Since your change was applied there have been 9 commits pushed to the master branch:

• af8c678: 8247910: Improve alignment and power-of-2 utilities using C++14
• 70cc7fc: 8253098: Archived full module graph should be disabled if CDS heap cannot be mapped
• ac9d1b0: 8223187: Remove setLocale() call in jpackage native launcher
• 9c24a56: 8253029: [PPC64] Remove obsolete Power6 code
• e6a493a: 8252882: Clean up jdk.javadoc and the related parts of jdk.compiler
• 68da63d: 8240658: Code completion not working for lambdas in method invocations that require type inference
• b05290a: 8252898: remove bulk registration of JFR CompilerPhaseType names
• 779d2c3: 8253084: Zero VM is broken after JDK-8252689
• 07da3a1: 8253030: ZGC: Change ZMarkCompleteTimeout unit to microseconds

Your commit was automatically rebased without conflicts.

Pushed as commit 3f455f0.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

[mlbridge]

Mailing list message from Kim Barrett on hotspot-runtime-dev:

On Sep 14, 2020, at 12:05 PM, Kim Barrett <kim.barrett at oracle.com> wrote:
Note that it might be that such a limited warning disable is insufficient in
the long run. This seems like a problem that might arise in other places as
we make more use of constexpr. We might instead need to globally disable
that warning for some limited range of VS versions.

Looks like I was right about that. I?ve privately received a report of a similar problem
arising as a result of yesterday?s push of 8238956. (I expect a bug report to be showing up
shortly.)

This will be a constant threat when adding uses of constexpr.

I think the only plausible solution is to disable that warning for VS versions prior to the one that fixes
that bug (mentioned in the referenced bug report). I also think the change for 8253089 should be
reverted.

I don?t know why I never encountered this in my experiments with C++14 over the past more than
a year. We (Oracle) only upgraded to VS2019 relatively recently (a few months). Though I think
the I wrote the unit test that failed to build (in that private report) fairly recently, probably after we
switched over to VS2019.

[shipilev]

I don't mind reverting this change, but it would break MSVC 2017 again, until we push the pragmas. Unfortunately, I lost the capability to build with MSVC 2017, so maybe we should ask SAP folks (who IIRC still have their pipelines with MSVC 2017) to verify this still works.

[mlbridge]

Mailing list message from Kim Barrett on hotspot-runtime-dev:

On Sep 16, 2020, at 7:24 AM, Aleksey Shipilev <shade at openjdk.java.net> wrote:

On Mon, 14 Sep 2020 22:25:06 GMT, Ioi Lam <iklam at openjdk.org> wrote:

It seems that MSVC 2017 is getting confused about the differences in `unsigned int` and `u2`. After a few attempts at
fixing this, I think we need to use `u2` consistently for hash code computations. `u2` is the final storage type for
the hash code in `JVMFlagLookup::_hashes`.

Marked as reviewed by iklam (Reviewer).

I don't mind reverting this change, but it would break MSVC 2017 again, until we push the pragmas. Unfortunately, I
lost the capability to build with MSVC 2017, so maybe we should ask SAP folks (who IIRC still have their pipelines with
MSVC 2017) to verify this still works.

I?m guessing this was intended to be in response to my followup comment.

I agree that we can?t revert that until we have something in place to deal with the VS2017 problem. It was the SAP
folks who reported the new failure to me, so I?m assuming they will be looking at whatever is done for that problem.

[mlbridge]

Mailing list message from Aleksey Shipilev on hotspot-runtime-dev:

On 9/16/20 1:42 PM, Kim Barrett wrote:

On Sep 16, 2020, at 7:24 AM, Aleksey Shipilev <shade at openjdk.java.net> wrote:

On Mon, 14 Sep 2020 22:25:06 GMT, Ioi Lam <iklam at openjdk.org> wrote:

It seems that MSVC 2017 is getting confused about the differences in `unsigned int` and `u2`. After a few attempts at
fixing this, I think we need to use `u2` consistently for hash code computations. `u2` is the final storage type for
the hash code in `JVMFlagLookup::_hashes`.

Marked as reviewed by iklam (Reviewer).

I don't mind reverting this change, but it would break MSVC 2017 again, until we push the pragmas. Unfortunately, I
lost the capability to build with MSVC 2017, so maybe we should ask SAP folks (who IIRC still have their pipelines with
MSVC 2017) to verify this still works.

I?m guessing this was intended to be in response to my followup comment.

Right. ml-bot confusion here.

I agree that we can?t revert that until we have something in place to deal with the VS2017 problem. It was the SAP
folks who reported the new failure to me, so I?m assuming they will be looking at whatever is done for that problem.

I submitted this one yesterday, btw:
https://bugs.openjdk.java.net/browse/JDK-8253154

--
Thanks,
-Aleksey