Skip to content

Changes for new/delete used by IProfiler #19973

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

Merged
merged 1 commit into from
Aug 12, 2024

Conversation

mpirvu
Copy link
Contributor

@mpirvu mpirvu commented Aug 7, 2024

This commit implements the following changes:

  • operator new was defined in every class derived from TR_IPBytecodeHashTableEntry After this commit there will be only one definition of operator new provided by the base class TR_IPBytecodeHashTableEntry and all derived classes will use the same definition. The same applies for operator delete
  • operator delete used to do nothing. After this commit operator delete will use the IProfiler persistent memory deallocator to free memory. Note that freed memory goes into a pool, to be reused later.
  • For 64-bit environments, operator new used to ensure aligned allocations by adding four bytes the size needed and then rounding up on a 8 byte boundary. This is not needed, because the persistent allocator already makes sure that allocations are 8 byte aligned, so the operator was just wasting memory.
  • TR_IProfiler::findOrCreateEntry() contains a race condition which may lead to a small (and bounded) memory leak. This commit frees the entry that is allocated but later found that is not needed.

@mpirvu mpirvu added the comp:jit label Aug 7, 2024
@mpirvu mpirvu requested a review from dsouzai as a code owner August 7, 2024 17:16
@mpirvu
Copy link
Contributor Author

mpirvu commented Aug 7, 2024

jenkins test sanity all jdk21

2 similar comments
@mpirvu
Copy link
Contributor Author

mpirvu commented Aug 7, 2024

jenkins test sanity all jdk21

@mpirvu
Copy link
Contributor Author

mpirvu commented Aug 8, 2024

jenkins test sanity all jdk21

@mpirvu
Copy link
Contributor Author

mpirvu commented Aug 8, 2024

Testing continues to fail with:

2  stderr: fatal: unable to access 'https://github.com/eclipse-openj9/openj9.git/': gnutls_handshake() failed: The TLS connection was non-properly terminated.

@mpirvu
Copy link
Contributor Author

mpirvu commented Aug 8, 2024

jenkins test sanity all jdk17

@mpirvu
Copy link
Contributor Author

mpirvu commented Aug 9, 2024

@dsouzai Could you please review/merge? Thanks

@dsouzai dsouzai self-assigned this Aug 9, 2024
@dsouzai
Copy link
Contributor

dsouzai commented Aug 9, 2024

@mpirvu build failure due to

12:30:41  /Users/jenkins/workspace/Build_JDK17_aarch64_mac_Personal/openj9/runtime/compiler/runtime/IProfiler.cpp:1130:7: error: delete called on 'TR_IPBytecodeHashTableEntry' that is abstract but has non-virtual destructor [-Werror,-Wdelete-abstract-non-virtual-dtor]
12:30:41        delete entry; // Newly allocated entry is not needed
12:30:41        ^

This commit implements the following changes:
- `operator new` was defined in every class derived from TR_IPBytecodeHashTableEntry
   After this commit there will be only one definition of `operator new`
   provided by the base class TR_IPBytecodeHashTableEntry and all derived
   classes will use the same definition. The same applies for
   `operator delete`
- `operator delete` used to do nothing. After this commit `operator delete`
   will use the IProfiler persistent memory deallocator to free memory.
   Note that freed memory goes into a pool, to be reused later.
- For 64-bit environments, `operator new` used to ensure aligned
   allocations by adding four bytes the size needed and then rounding up
   on a 8 byte boundary. This is not needed, because the persistent allocator
   already makes sure that allocations are 8 byte aligned, so the operator
   was just wasting memory.
- TR_IProfiler::findOrCreateEntry() contains a race condition which may
   lead to a small (and bounded) memory leak. This commit frees the entry
   that is allocated but later found that is not needed.

Signed-off-by: Marius Pirvu <[email protected]>
@mpirvu
Copy link
Contributor Author

mpirvu commented Aug 9, 2024

jenkins test sanity amac jdk17

@mpirvu
Copy link
Contributor Author

mpirvu commented Aug 9, 2024

jenkins test sanity all jdk17

@mpirvu
Copy link
Contributor Author

mpirvu commented Aug 10, 2024

windows build failed with

All nodes of label ‘[ci.role.build&&hw.arch.x86&&sw.os.windows](https://openj9-jenkins.osuosl.org/label/ci.role.build&&hw.arch.x86&&sw.os.windows/)’ are offline

The error on zlinux is [testJitserverArguments_0] and has nothing to do with this PR

Testing: Test SSL Failure Case with connection to Non-SSL Server
...
20:13:56  >> Required condition was not found: [Output match: JITServer::StreamFailure: Failed to SSL_connect]

@dsouzai dsouzai merged commit d57675e into eclipse-openj9:master Aug 12, 2024
13 of 16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants