[RefC] Object Immortalization and Pre-Generation of Constants #3242
+396
−203
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
* Shrink Value_Header to improve utilization of memory.
* When refCounter reaches its maximum, the object is immortalized to
avoid overflow. This immotalization is also used to represent
statically allocated stock objects.
* Prepare some commonly seen values and share it to improve memory usage.
* Added debug code to dump memory stats.
* Commonly seen values such as integers less than 100 are predefined and shared. * Constant String, Int64, Bits64 and Double values are allocated statically as indestructible and shared.
Refactor constantName function to return Core String type and handle unsupported types by throwing InternalError exception.
mattpolzin
reviewed
Jun 5, 2024
| Ch x => pure "idris2_mkChar(\{escapeChar x})" | ||
| Str _ => orStagen | ||
| PrT t => pure $ cPrimType t | ||
| _ => pure "NULL" |
There was a problem hiding this comment.
this catch-all feels a bit brittle to me at first look at least
| B64 x => pure "((Value*)&idris2_constant_Bits64_\{show x})" | ||
| Db x => pure "((Value*)&idris2_constant_Double_\{cCleanString $ show x})" | ||
| Str x => pure "((Value*)&idris2_constant_String_\{n})" | ||
| _ => throw $ InternalError "[refc] Unsupported type of constant." |
There was a problem hiding this comment.
this compile-time error feels like it could be fairly easily remediated by a new smaller data-type than Constant so that we aren't relying on the invariant that no new type of constant is stored in ConstDef without a new case being added here.
Comment on lines
+210
to
+214
| } else if (elem->header.refCounter != 1) { | ||
| --elem->header.refCounter; | ||
| return; | ||
| } else { | ||
| IDRIS2_INC_MEMSTAT(n_freed); |
There was a problem hiding this comment.
I notice that whereas elem->header.refCounter used to be decremented prior to freeing, now it is not; that seems theoretically ok to me, but I wanted to make sure it was intentional because I have not verified that nothing downstream of this code would double check the ref count.
Co-authored-by: Mathew Polzin <[email protected]>
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.
Description
This PR immortalizes objects whose reference counter has reached its maximum value and makes them excluded from GC. This immortalization is used to make the following improvements.
Statically generate values that often appear at runtime, such as empty strings and integers less than 100, and share them at runtime to reduce allocation costs. This heuristic may attract many opinions, but it is a good first-try choice.
On the other hand, there is no way to release immortal objects, so they leak. Dynamic immortalization rarely occurs, but it is a problem for programs that run for long periods of time, such as servers. A secondary GC is needed to solve this, but is not implemented in this PR.
Should this change go in the CHANGELOG?
implementation, I have updated
CHANGELOG_NEXT.md(and potentially alsoCONTRIBUTORS.md).