Heap Resize: Concurrent KO/Uninitialized Mark Map Bug fix #4716
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
RSalman
requested review from
charliegracie,
mstoodle,
rwy7 and
youngar
as code owners
RSalman
changed the title
amicic
reviewed
Jan 13, 2020
amicic
reviewed
Jan 13, 2020
RSalman
force-pushed
the
cgc-fix
branch
2 times, most recently
from
e03a819
to
a520aa2
Compare
amicic
reviewed
Jan 15, 2020
amicic
reviewed
Jan 15, 2020
amicic
reviewed
Jan 15, 2020
amicic
reviewed
Jan 15, 2020
- MarkMap init consolidated MarkMap init logic has been merged to resolve the timing hole outlined in eclipse-openj9/openj9#8020. MarkMap init is dependent on Concurrent State of the global collector, with the init logic split between `heapAddRange` and `heapReconfigured`, any changes in states between the two may leave expanded mark map uninited. Hence, the decision to init (`setMarkBitsInRange`) the mark map has been moved from `heapAddRange` to `heapReconfigured` which is where we determine if an update to the init table is required (`tuneToHeap`/`determineInitWork`). As a result, init is no longer affected by the change in Concurrent state and we eliminate the timing window. This guarantee that the MarkMap will be inited, either on the spot or afterwards by updating the init table. - Introduced HeapReconfigReason and reworked heapReconfigred API heapReconfigred now distinguishes different reasons for reconfiguration (Expand, contract, etc), this functionality is required for the Mark Map init changes, specifically for Gencon and it is used when Scavenger resizes tenure _(PhysicalSubArenaVirtualMemorySemiSpace)_ or global collecter preforms a resize _(PhysicalSubArenaVirtualMemoryFlat)_. At the time, this is new information is only consumed by Concurrent Global Collecter (ConcurrentGC.cpp), for policies making use of other collectors the HeapReconfigReason param defaults to `NONE`. The Reconfig reasons are dealt in the following ways by Concurrent Global Collector: - We should never end up in `heapReconfigured` with `RECONFIG_NONE` reason - `RECONFIG_CONTRACT` signifies that `heapRemoveRange` had taken place, in which case, we just need to update init table (`tuneToHeap`) when Concurrent is Off, otherwise just `adjustTraceTargets` - If `heapAddRange` takes place then `heapReconfigured` is called with `RECONFIG_EXPAND` and have have two different cases: 1) heapAddRange was successful (return true), heap reconfig should be provided with `lowAddress` & `highAddress` which will be used to init mark map 2) heapAddRange returns false, signifies a failed `heapAddRange`, address params are expected to be NULL, in which case mark map won't be inited but we'll still either `tuneToHeap` or `adjustTraceTarget`. Signed-off-by: Salman Rana <salman.rana@ibm.com>
|
Removed default arguments for heapReconfigured for:
The default arguments are unnecessary... they're only required for the Subspace as that's the high level caller which propagates to the Collector and memory space (which in turn propagates to heap) |
RSalman
added a commit
to RSalman/openj9
that referenced
this pull request
Jan 16, 2020
`heapReconfigured` API changes resulting from eclipse-omr/omr#4716 Signed-off-by: Salman Rana <salman.rana@ibm.com>
RSalman
commented
Jan 16, 2020
|
This looks good to me. @dmitripivkine please, have a look. |
dmitripivkine
approved these changes
Jan 17, 2020
|
I believe size of PR name in OMR still be limited to 50 characters (70 in OpenJ9). So this PR might require renaming - up to OMR reviewer |
RSalman
added a commit
to RSalman/openj9
that referenced
this pull request
Jan 17, 2020
`heapReconfigured` API changes resulting from eclipse-omr/omr#4716 Signed-off-by: Salman Rana <salman.rana@ibm.com>
amicic
approved these changes
Jan 17, 2020
RSalman
changed the title
RSalman
changed the title
|
@rwy0717 @youngar please have a look, ready to be reviewed/merged |
|
@genie-omr build all |
rwy7
approved these changes
Jan 23, 2020
RSalman
added a commit
to RSalman/omr
that referenced
this pull request
Jan 27, 2020
Follow-up changes to eclipse-omr#4716 to remove code required temporarily to not break dependency with heapReconfigure API changes. Signed-off-by: Salman Rana <salman.rana@ibm.com>
RSalman
added a commit
to RSalman/omr
that referenced
this pull request
Jan 27, 2020
Follow-up changes to eclipse-omr#4716 to remove code required temporarily to not break dependency with heapReconfigure API changes. Signed-off-by: Salman Rana <salman.rana@ibm.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes eclipse-openj9/openj9#8020
MarkMap init logic has been merged to resolve the timing hole outlined in eclipse-openj9/openj9#8020 (comment). MarkMap init is dependent on Concurrent State of the global collector, with the init logic split between
heapAddRangeandheapReconfigured, any changes in states between the two may leave expanded mark map uninited. Hence, the decision to init (setMarkBitsInRange) the mark map has been moved fromheapAddRangetoheapReconfiguredwhich is where we determine if an update to the init table is required (tuneToHeap/determineInitWork). As a result, init is no longer affected by the change in Concurrent state and we eliminate the timing window. This guarantees that the MarkMap will be inited, either on the spot or afterwards by updating the init table.HeapReconfigReasonand reworkedheapReconfigredAPIheapReconfigred now distinguishes different reasons for reconfiguration (Expand, contract, etc), this functionality is required for the Mark Map init changes, specifically for Gencon and it is used when Scavenger resizes tenure (PhysicalSubArenaVirtualMemorySemiSpace) or global collecter preforms a resize (PhysicalSubArenaVirtualMemoryFlat). At the time, this new information is only consumed by Concurrent Global Collecter (ConcurrentGC.cpp), for policies making use of other collectors, the HeapReconfigReason param defaults to
NONE.The Reconfig reasons are dealt in the following ways by Concurrent Global Collector:
heapReconfiguredwithRECONFIG_NONEreasonRECONFIG_CONTRACTsignifies thatheapRemoveRangehad taken place, in which case, we just need to update init table (tuneToHeap) when Concurrent is Off, otherwise justadjustTraceTargetsheapAddRangetakes place thenheapReconfiguredis called withRECONFIG_EXPANDand have have two different cases:1) heapAddRange was successful (returns true), heap reconfig should be provided with
lowAddress&highAddresswhich will be used to init mark map2) heapAddRange returns false, signifies a failed
heapAddRange, address params are expected to be NULL, in which case mark map won't be inited but we'll still eithertuneToHeaporadjustTraceTarget.Signed-off-by: Salman Rana salman.rana@ibm.com