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.

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

8271569: Clean up the use of CDS constants and field offsets #5450

Closed
wants to merge 8 commits into from

Conversation

yminqi
Copy link
Contributor

@yminqi yminqi commented Sep 9, 2021

Changed cdsOffsets.cpp to cdsConstants.cpp, now the offsets and constants are initialized static and searched separately.
The offsets array could not use 'constexpr' since g++ on MacOs and VSC++ on Windows complained reinterpret_cast in 'offset_of' should not be used in constexpr initialization. Changed some field access for forming global list first.

Note with 'git mv' to rename cdsoffset.cpp to cdsConstants.cpp, same for cdsoffsets.hpp to cdsConstants.hpp, due to the contents changed more than 50% so git will not think cdsConstants.cpp is renamed from cdsoffsets.cpp. Instead, it is regarded as a new file.

Tests: ter1-4

Thanks
Yumin


Progress

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

Issue

  • JDK-8271569: Clean up the use of CDS constants and field offsets

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/5450/head:pull/5450
$ git checkout pull/5450

Update a local copy of the PR:
$ git checkout pull/5450
$ git pull https://git.openjdk.java.net/jdk pull/5450/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5450

View PR using the GUI difftool:
$ git pr show -t 5450

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/5450.diff

@yminqi
Copy link
Contributor Author

yminqi commented Sep 9, 2021

/label add hotspot-runtime

@bridgekeeper
Copy link

bridgekeeper bot commented Sep 9, 2021

👋 Welcome back minqi! 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.

@yminqi yminqi marked this pull request as draft Sep 9, 2021
@openjdk openjdk bot added rfr hotspot-runtime labels Sep 9, 2021
@openjdk
Copy link

openjdk bot commented Sep 9, 2021

@yminqi
The hotspot-runtime label was successfully added.

@yminqi yminqi marked this pull request as ready for review Sep 9, 2021
@openjdk openjdk bot added rfr and removed rfr labels Sep 9, 2021
@mlbridge
Copy link

mlbridge bot commented Sep 9, 2021

Copy link
Member

@iklam iklam left a comment

Looks good. Just some small nits.

src/hotspot/share/cds/cdsConstants.hpp Show resolved Hide resolved
Copy link
Member

@dholmes-ora dholmes-ora left a comment

Hi Yumin,

There's obviously a lot more going on here than just a simple rename so I think the JBS issue should be changed accordingly.

A few minor comments below.

Thanks,
David

};

size_t get_cds_offset(const char* name) {
for (int i = 0; i < (int)(sizeof(cds_offsets)/sizeof(cds_offsets[0])); i++) {
Copy link
Member

@dholmes-ora dholmes-ora Sep 10, 2021

Choose a reason for hiding this comment

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

Can't we just define a size for the array and use that? It isn't really a dynamic/unknown quantity.

Copy link
Contributor Author

@yminqi yminqi Sep 10, 2021

Choose a reason for hiding this comment

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

The array size is known at compile time --- it is not dynamic but we do not need to manually count the size of the array this way. If define a size for the array, we need to count the number of items in the array to define the array size. We may add more items to the array --- just add items and don't need change anything else. Since total array memory size and array component size are known at compile time, so compiler will not generate code to calculate the size. How about this?
const int size = (int)(sizeof(cds_offsets)/sizeof(cds_offsets[0]));
for (int i = 0; i < size; i++) {
...
}

Copy link
Member

@dholmes-ora dholmes-ora Sep 13, 2021

Choose a reason for hiding this comment

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

I always thought that was a hack way of determining an array size, not an approved method. :(

Don't worry just leave it as-is.

Copy link
Member

@stefank stefank Sep 13, 2021

Choose a reason for hiding this comment

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

We have functionality for this in globalDefinitions.hpp:

#define ARRAY_SIZE(array) sizeof(array_size_impl(array))

Copy link
Contributor Author

@yminqi yminqi Sep 13, 2021

Choose a reason for hiding this comment

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

@stefank Thanks --- it saves code size.

src/hotspot/share/cds/dynamicArchive.hpp Show resolved Hide resolved
@@ -208,10 +208,11 @@ class FileMapHeader: private CDSFileMapHeaderBase {
// The following fields are all sanity checks for whether this archive
// will function correctly with this JVM and the bootclasspath it's
// invoked with.
public:
Copy link
Member

@dholmes-ora dholmes-ora Sep 10, 2021

Choose a reason for hiding this comment

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

Again this looks like it should be private with either a public accessor or else a friend declaration for the client code.

Copy link
Contributor Author

@yminqi yminqi Sep 10, 2021

Choose a reason for hiding this comment

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

same reason as above.

Copy link
Member

@stefank stefank left a comment

I saw the array size comments ...

*/

#include "precompiled.hpp"
#include <cstddef>
Copy link
Member

@stefank stefank Sep 13, 2021

Choose a reason for hiding this comment

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

It is not stated in the style guide, but I think most files include the system headers after the HotSpot headers, separated by a blank line.

Copy link
Member

@dholmes-ora dholmes-ora Sep 14, 2021

Choose a reason for hiding this comment

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

Is this even needed now? I don't see it being used.

Copy link
Member

@stefank stefank Sep 14, 2021

Choose a reason for hiding this comment

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

To be clear, the layout that I'd like to see is:

#include "precompiled.hpp"
#include "cds.hpp"
...
#include "cds/filemap.hpp"

#include <cstddef>

However, I think the more appropriate change is to include globalDefinitions.hpp instead of explicitly including cstddef.

Copy link
Contributor Author

@yminqi yminqi Sep 14, 2021

Choose a reason for hiding this comment

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

The pre-submit tests failed to build hotspot for linux on arm/ppc/s390 etc after removed. needed for defining size_t. So will add it back at last section of 'include'.

Copy link
Member

@iklam iklam Sep 14, 2021

Choose a reason for hiding this comment

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

The problem is here in cdsConstants.hpp

#include "memory/allStatic.hpp"
typedef struct {
  const char* _name;
  size_t _value;
} CDSConst;

globalDefinitions.hpp should be included here for the declaration of size_t.

Copy link
Contributor Author

@yminqi yminqi Sep 14, 2021

Choose a reason for hiding this comment

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

Yes, but it still failed for non-pch since cds.h is before cdsConstants.hpp. size_t in cds.h is not defined yet. Should I add it to cds.h?

Copy link
Member

@iklam iklam Sep 14, 2021

Choose a reason for hiding this comment

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

This is a bit complicated. The file is src/hotspot/share/include/cds.h, and the HotSpot convention for including files in this directory usually drops the "include/" part. E.g.,

#include "jvm.h"

But this will put #include "cds.h" at the very beginning of all includes, so it doesn't take up size_t which is indirectly declared later via globalDefinitions.hpp.

However, header files in src/hotspot/share/ may be included by C/C++ files outside of HotSpot. As a result, these headers cannot include other HotSpot headers. I.e., you cannot put #include "utilities/globalDefinitions.hpp" in cds.h, because cds.h is included by src/jdk.hotspot.agent/share/native/libsaproc/ps_core_common.c.

I think one compromise is to add #include <stddefs.h> in cds.h. Since cds.h can be included by C source code, we cannot use #include <cstddef> which is a C++ thing. <stddefs.h> is part of ANSI C standard and we already include it inside share/utilities/debug.hpp, so we can safely assume that it exists for all compilers that can build HotSpot.

Copy link
Member

@dholmes-ora dholmes-ora Sep 15, 2021

Choose a reason for hiding this comment

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

I agree with Ioi - use stddefs.h in cds.h

};

size_t get_cds_offset(const char* name) {
for (int i = 0; i < (int)(sizeof(cds_offsets)/sizeof(cds_offsets[0])); i++) {
Copy link
Member

@stefank stefank Sep 13, 2021

Choose a reason for hiding this comment

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

We have functionality for this in globalDefinitions.hpp:

#define ARRAY_SIZE(array) sizeof(array_size_impl(array))

…made CDSConstants a friend class of accessed classes
src/hotspot/share/cds/dynamicArchive.hpp Show resolved Hide resolved
{ "static_magic", (int)CDS_ARCHIVE_MAGIC },
{ "dynamic_magic", (int)CDS_DYNAMIC_ARCHIVE_MAGIC },
{ "int_size", sizeof(int) },
{ "CDSFileMapRegion_size", sizeof(CDSFileMapRegion) },
Copy link
Member

@iklam iklam Sep 13, 2021

Choose a reason for hiding this comment

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

Should be (int)sizeof(int) because the type of sizeof(...) is implementation-defined. See https://en.cppreference.com/w/cpp/types/size_t

Copy link
Contributor Author

@yminqi yminqi Sep 13, 2021

Choose a reason for hiding this comment

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

Now that all almost all (except for 'magic') with size_t, we should use name vs size_t in the struct and get functions, cast when return from vm in whiteBox.cpp.

@yminqi yminqi changed the title 8271569: Rename cdsoffsets.cpp to cdsConstants.cpp 8271569: Clean up the use of CDS constants and field offsets Sep 13, 2021
Copy link
Member

@dholmes-ora dholmes-ora left a comment

Hi Yumin,

Updates seem okay to me. One query below.

Thanks,
David

*/

#include "precompiled.hpp"
#include <cstddef>
Copy link
Member

@dholmes-ora dholmes-ora Sep 14, 2021

Choose a reason for hiding this comment

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

Is this even needed now? I don't see it being used.

@openjdk
Copy link

openjdk bot commented Sep 14, 2021

@yminqi This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8271569: Clean up the use of CDS constants and field offsets

Reviewed-by: iklam, dholmes

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 66 new commits pushed to the master branch:

  • febcc72: 8273366: [testbug] javax/swing/UIDefaults/6302464/bug6302464.java fails on macOS12
  • 1017a2c: 8273135: java/awt/color/ICC_ColorSpace/MTTransformReplacedProfile.java crashes in liblcms.dylib with NULLSeek+0x7
  • 6cf70f5: 8273638: javax/swing/JTable/4235420/bug4235420.java fails in GTK L&F
  • e66bf47: 8273414: ResourceObj::operator delete should handle nullptr in debug builds
  • 16c3ad1: 8272574: C2: assert(false) failed: Bad graph detected in build_loop_late
  • e7ab372: 8273641: (bf) Buffer subclasses documentation contains template strings
  • 22a7191: 8273040: Turning off JpAllowDowngrades (or Upgrades)
  • 394ebc8: 8270553: Tests should not use (real, in-use, routable) 1.1.1.1 as dummy IP value
  • 0f31d0f: 8273373: Zero: Cannot invoke JVM in primordial threads on Zero
  • fe89dd3: 8271254: javac generates unreachable code when using empty semicolon statement
  • ... and 56 more: https://git.openjdk.java.net/jdk/compare/c81690d7166c32caff6ef3a55fe9b157049e2b56...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

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

@openjdk openjdk bot added the ready label Sep 14, 2021
*/

#include "precompiled.hpp"
#include <cstddef>
Copy link
Member

@stefank stefank Sep 14, 2021

Choose a reason for hiding this comment

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

To be clear, the layout that I'd like to see is:

#include "precompiled.hpp"
#include "cds.hpp"
...
#include "cds/filemap.hpp"

#include <cstddef>

However, I think the more appropriate change is to include globalDefinitions.hpp instead of explicitly including cstddef.

iklam
iklam approved these changes Sep 14, 2021
Copy link
Member

@iklam iklam left a comment

LGTM. Small nit on friend class declaration.

friend class VMStructs;
friend class CDSConstants;
Copy link
Member

@iklam iklam Sep 14, 2021

Choose a reason for hiding this comment

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

It's better to sort the classes alphabetically. CDSConstants should come before VMStructs.

iklam
iklam approved these changes Sep 15, 2021
Copy link
Member

@iklam iklam left a comment

The new version looks good to me.

@yminqi
Copy link
Contributor Author

yminqi commented Sep 16, 2021

/integrate

@openjdk
Copy link

openjdk bot commented Sep 16, 2021

Going to push as commit 9c5441c.
Since your change was applied there have been 91 commits pushed to the master branch:

  • 12fa707: 8261941: Use ClassLoader for unregistered classes during -Xshare:dump
  • 7e92abe: 8273710: Remove redundant stream() call before forEach in jdk.jdeps
  • 59b2478: 8273659: Replay compilation crashes with SIGSEGV since 8271911
  • 5e4d09c: 8273300: Check Mutex ranking during a safepoint
  • c86e24d: 8271954: C2: assert(false) failed: Bad graph detected in build_loop_late
  • 14dc517: 8273372: Remove scavenge trace message in psPromotionManager
  • 241ac89: 8273606: Zero: SPARC64 build fails with si_band type mismatch
  • 181292d: 8273801: Handle VMTYPE for "core" VM variant
  • 09ecb11: 8273806: compiler/cpuflags/TestSSE4Disabled.java should test for CPU feature explicitly
  • 99cfc16: 8273805: gc/g1/TestGCLogMessages.java test should handle non-JFR configs
  • ... and 81 more: https://git.openjdk.java.net/jdk/compare/c81690d7166c32caff6ef3a55fe9b157049e2b56...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Sep 16, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Sep 16, 2021
@openjdk
Copy link

openjdk bot commented Sep 16, 2021

@yminqi Pushed as commit 9c5441c.

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

@yminqi yminqi deleted the jdk-8271569 branch Sep 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-runtime integrated
4 participants