JDK-8212707 : GlobalCounter padding is too optimistic
  • Type: Enhancement
  • Component: hotspot
  • Sub-Component: runtime
  • Affected Version: 12
  • Priority: P4
  • Status: Resolved
  • Resolution: Fixed
  • Submitted: 2018-10-19
  • Updated: 2019-05-28
  • Resolved: 2018-10-23
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
JDK 12
12 b17Fixed
Related Reports
Relates :  
JDK-8195099 - Concurrent safe-memory-reclamation mechanism
Description
Defined as follows:

class GlobalCounter : public AllStatic {
 private:
  // Since do not know what we will end up next to in BSS, we make sure the
  // counter is on a seperate cacheline.
  struct PaddedCounter {
    DEFINE_PAD_MINUS_SIZE(0, DEFAULT_CACHE_LINE_SIZE/2, 0);
    volatile uintx _counter;
    DEFINE_PAD_MINUS_SIZE(1, DEFAULT_CACHE_LINE_SIZE/2, sizeof(volatile uintx));
  };

Why "/2"? Every other use of DEFINE_PAD_MINUS_SIZE macro uses the full cache line size, and it is sensible: you can have two values *at the same cache line* separated by half of its width, unless the whole thing is cache-line-aligned.

This also sets up for awkward accidents when others copy-paste this padding block as example for padding elsewhere.
Comments
LGTM.
22-10-2018
I'll send out: diff -r bca2b63dd839 src/hotspot/share/utilities/globalCounter.hpp --- a/src/hotspot/share/utilities/globalCounter.hpp Mon Oct 22 14:08:07 2018 +0800 +++ b/src/hotspot/share/utilities/globalCounter.hpp Mon Oct 22 11:48:17 2018 +0200 @@ -48,1 +48,1 @@ - DEFINE_PAD_MINUS_SIZE(0, DEFAULT_CACHE_LINE_SIZE/2, 0); + DEFINE_PAD_MINUS_SIZE(0, DEFAULT_CACHE_LINE_SIZE, 0); @@ -50,1 +50,1 @@ - DEFINE_PAD_MINUS_SIZE(1, DEFAULT_CACHE_LINE_SIZE/2, sizeof(volatile uintx)); + DEFINE_PAD_MINUS_SIZE(1, DEFAULT_CACHE_LINE_SIZE, sizeof(volatile uintx));
22-10-2018
Would you do this fix? Or want me to RFR it?
22-10-2018
Sorry If I was unclear. Yes this should be fixed. And someone should fix the actual define to makes sense in a separate issue.
22-10-2018
Yes, I remember making the same concerned noises when DEFAULT_CACHE_LINE_SIZE guessing based on compiler flags was first introduced, and that's a separate story. Still, I don't think this allows actual usages to second-guess whatever method is used to produce that value, and should instead take that setting at face value, without further refinements. Makes sense?
22-10-2018
[~shade] You are correct, except for x64. Which was what I was looking at.... #ifdef _LP64 // tiered, 64-bit, large machine #define DEFAULT_CACHE_LINE_SIZE 128 #else As far as I understand it, Intel (>=core2 ?) will only fetch 2 cachelines if you using 'streaming' instructions. I think it really confusing to set CPU cache line size depending on which compilers we have enabled in the build. If someone build the vm with only C1 the padding size is wrong for x64 also.
22-10-2018
Paging [~rehn] :)
19-10-2018