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.

Sign up for GitHub

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

Jump to bottom

Clean up CopyForwardScheme class #21072

Merged
merged 1 commit into from
Feb 11, 2025
Merged

Clean up CopyForwardScheme class #21072

merged 1 commit into from
Feb 11, 2025

Conversation

LinHu2016
Copy link
Contributor

Use consistent data types and code formatting

@LinHu2016 LinHu2016 mentioned this pull request Feb 5, 2025
Merged
@LinHu2016
Copy link
Contributor Author

@amicic @dmitripivkine please review the changes, Thanks

keithc-ca
keithc-ca requested changes Feb 5, 2025
View reviewed changes
runtime/gc_vlhgc/CopyForwardScheme.cpp
@@ -705,14 +705,14 @@ MM_CopyForwardScheme::reinitCache(MM_EnvironmentVLHGC *env, MM_CopyScanCacheVLHG

MM_HeapRegionDescriptorVLHGC * region = (MM_HeapRegionDescriptorVLHGC *)_regionManager->tableDescriptorForAddress(cache->cacheBase);
Trc_MM_CopyForwardScheme_reinitCache(env->getLanguageVMThread(), _regionManager->mapDescriptorToRegionTableIndex(region), cache,
region->getAllocationAgeSizeProduct() / (1024 * 1024) / (1024 * 1024), (double)((UDATA)cache->cacheAlloc - (UDATA)region->getLowAddress()) / (1024 * 1024));
region->getAllocationAgeSizeProduct() / (1024 * 1024) / (1024 * 1024), (double)((uintptr_t)cache->cacheAlloc - (uintptr_t)region->getLowAddress()) / (1024 * 1024));
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this really want to work with TB (dividing by 1MB twice)?

runtime/gc_vlhgc/CopyForwardScheme.cpp Outdated
@@ -1284,7 +1284,7 @@ MM_CopyForwardScheme::copyAndForward(MM_EnvironmentVLHGC *env, MM_AllocationCont
/* Object has been copied - update the forwarding information and return */
*objectPtrIndirect = objectPtr;
} else {
Assert_GC_true_with_message(env, (UDATA)0x99669966 == _extensions->objectModel.getPreservedClass(&forwardHeader)->eyecatcher, "Invalid class in objectPtr=%p\n", originalObjectPtr);
Assert_GC_true_with_message(env, (uintptr_t)0x9966uintptr_t== _extensions->objectModel.getPreservedClass(&forwardHeader)->eyecatcher, "Invalid class in objectPtr=%p\n", originalObjectPtr);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't expect this will compile. I don't think the second uintptr_t was intended here.

runtime/gc_vlhgc/CopyForwardScheme.cpp Outdated
UDATA *leafPtr = (UDATA *)J9GC_J9OBJECT_CLAZZ(objectPtr, env)->instanceLeafDescription;
UDATA leafBits;
uintptr_t *leafPtr = (uintptr_t *)J9GC_J9OBJECT_CLAZZ(objectPtr, env)->instanceLeafDescription;
UDATuintptr_tfBits;
Copy link
Contributor

Choose a reason for hiding this comment

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

Another compile error?

runtime/gc_vlhgc/CopyForwardScheme.cpp Outdated
@@ -2799,7 +2799,7 @@ MM_CopyForwardScheme::getNextWorkUnit(MM_EnvironmentVLHGC *env, UDATA preferredN
while (!isAnyScanWorkAvailable(env) && (doneIndex == _doneIndex)) {
#if defined(J9MODRON_TGC_PARALLEL_STATISTICS)
PORT_ACCESS_FROM_ENVIRONMENT(env);
U_64 waitEndTime, waitStartTime;
uint8_t waitEndTime, waitStartTime;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you meant uint64_t, and each local should be declared (and initialized separately).

@LinHu2016 LinHu2016 force-pushed the coding-std-update11 branch from dc9f2a6 to 7d96658 Compare February 6, 2025 15:58
@LinHu2016 LinHu2016 requested a review from keithc-ca February 6, 2025 16:17
@LinHu2016 LinHu2016 force-pushed the coding-std-update11 branch 2 times, most recently from 53473c9 to eddf6c8 Compare February 7, 2025 15:05
dmitripivkine
dmitripivkine reviewed Feb 7, 2025
View reviewed changes
runtime/gc_vlhgc/CopyForwardScheme.cpp Outdated
@@ -3600,7 +3602,7 @@ void
MM_CopyForwardScheme::cleanCardTableForPartialCollect(MM_EnvironmentVLHGC *env, MM_CardCleaner *cardCleaner)
{
PORT_ACCESS_FROM_ENVIRONMENT(env);
U_64 cleanStartTime = j9time_hires_clock();
uint8_t cleanStartTime = j9time_hires_clock();
Copy link
Contributor

Choose a reason for hiding this comment

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

space at wrong side of star line 3608

Copy link
Contributor

Choose a reason for hiding this comment

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

mistake: type should be uint64_t

@LinHu2016 LinHu2016 force-pushed the coding-std-update11 branch from eddf6c8 to 194c4bf Compare February 7, 2025 17:32
dmitripivkine
dmitripivkine reviewed Feb 7, 2025
View reviewed changes
runtime/gc_vlhgc/CopyForwardScheme.cpp Outdated
Assert_MM_false(UDATA_MAX == compactGroup->_markMapPGCSlotIndex); /* Safety check from flushing to see if somehow the cache is being resurrected */
Assert_MM_false(UDATA_MAX == compactGroup->_markMapGMPSlotIndex); /* Safety check from flushing to see if somehow the cache is being resurrected */
Assert_MM_false(uintptr_t_MAX == compactGroup->_markMapPGCSlotIndex); /* Safety check from flushing to see if somehow the cache is being resurrected */
Assert_MM_false(uintptr_t_MAX == compactGroup->_markMapGMPSlotIndex); /* Safety check from flushing to see if somehow the cache is being resurrected */
Copy link
Contributor

Choose a reason for hiding this comment

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

I think these two lines have been updated by mistake

dmitripivkine
dmitripivkine reviewed Feb 7, 2025
View reviewed changes
runtime/gc_vlhgc/CopyForwardScheme.cpp Outdated
@@ -2873,7 +2874,8 @@ MM_CopyForwardScheme::getNextWorkUnitNoWait(MM_EnvironmentVLHGC *env, UDATA pref
}
if (SCAN_REASON_NONE == ret && (0 != _regionCountCannotBeEvacuated) && !abortFlagRaised()) {
if (env->_workStack.retrieveInputPacket(env)) {
ret = SCAN_REASON_PACKET;
ret = SCAN_REASON_PACKET;1188
Copy link
Contributor

Choose a reason for hiding this comment

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

please remove garbage and empty line

dmitripivkine
dmitripivkine reviewed Feb 7, 2025
View reviewed changes
runtime/gc_vlhgc/CopyForwardScheme.cpp Outdated
{
if (cache == NULL) {
return SCAN_TO_COPY_CACHE_MAX_DISTANCE;
}
IDATA distance = ((UDATA) cache->cacheAlloc) - ((UDATA) cache->scanCurrent);
UDATA udistance;
IDATA distance = ((uintptr_t) cache->cacheAlloc) - ((uintptr_t) cache->scanCurrent);
Copy link
Contributor

Choose a reason for hiding this comment

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

IDATA should be replaced to intptr_t

dmitripivkine
dmitripivkine reviewed Feb 7, 2025
View reviewed changes
runtime/gc_vlhgc/CopyForwardScheme.cpp Outdated
@@ -3659,7 +3661,7 @@ MM_CopyForwardScheme::cleanCardTableForPartialCollect(MM_EnvironmentVLHGC *env,
}
}

U_64 cleanEndTime = j9time_hires_clock();
uint8_t cleanEndTime = j9time_hires_clock();
Copy link
Contributor

Choose a reason for hiding this comment

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

mistake: type should be uint64_t

dmitripivkine
dmitripivkine reviewed Feb 7, 2025
View reviewed changes
runtime/gc_vlhgc/CopyForwardScheme.cpp Outdated

Assert_MM_true(0 != usedBytes);

/* convert allocation age product (usedBytes * age) back to pure age */
U_64 newAllocationAge = (U_64)(region->getAllocationAgeSizeProduct() / (double)usedBytes);
uint8_t newAllocationAge = (uint8_t)(region->getAllocationAgeSizeProduct() / (double)usedBytes);
Copy link
Contributor

Choose a reason for hiding this comment

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

mistake: type should be uint64_t

dmitripivkine
dmitripivkine reviewed Feb 7, 2025
View reviewed changes
runtime/gc_vlhgc/CopyForwardScheme.hpp Outdated
omrthread_monitor_t _scanCacheMonitor; /**< Used when waiting on work on any of the _cacheScanLists */

volatile UDATA* _workQueueWaitCountPtr; /**< The number of threads currently sleeping on *_workQueueMonitorPtr, awaiting scan cache work or work from packets*/
omrthread_monitor_t* _workQueueMonitorPtr; /**< Used when waiting on work on any of the _cacheScanLists or workPackets*/
volatile uintptr_t* _workQueueWaitCountPtr; /**< The number of threads currently sleeping on *_workQueueMonitorPtr, awaiting scan cache work or work from packets*/
Copy link
Contributor

Choose a reason for hiding this comment

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

space at the wrong side of the star

@LinHu2016
Clean up CopyForwardScheme class
19a56fe 
Use consistent data types and code formatting

Signed-off-by: lhu <[email protected]>
@dmitripivkine
Copy link
Contributor

Jenkins test sanity xLinux,aix jdk21

@dmitripivkine
Copy link
Contributor

AIX build failed due infra reason, merging

@dmitripivkine dmitripivkine merged commit 1c4701c into eclipse-openj9:master Feb 11, 2025
5 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dmitripivkine dmitripivkine dmitripivkine approved these changes

@keithc-ca keithc-ca Awaiting requested review from keithc-ca

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@LinHu2016 @dmitripivkine @keithc-ca