internal: data-oriented storage for TLib
#797
Merged
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
Summary ======= Replace storing `TLib` objects as `ref`s in `TSym` with storing all `TLib` instances belonging to a module in a sequence that a `TSym` then stores an index into. This slightly reduces the size of `TSym` and, more importantly, makes serializing the symbols and associated `TLib` instances easier: instead of storing a `PackedLib` for *each* packed symbol, the packed symbols now only store an index into the attached-to module's packed lib list. Details ======= For simplicity, the list with the libs for a module is stored in a separate `seq` in `ModuleGraph`. The list cannot be stored in `PContext`, as it needs to be accessible after semantic analysis, at which point `PContext` is already gone. Storing the `TLib` objects for a module happens when the module is closed. While not strictly necessary at the moment, this is a preparation for storing a `PSym` in `TLib.name` that is only set up once the module is closed. Aside from reducing the size of rodfiles, not storing a `PackedLib` for each `PackedSym` also fixes another inefficiency, namely that each deserialized got a unique, possibly duplicated, `TLib` instance.
A symbol can now reference the `TLib` belonging to a module that is not the one the symbol is attached to. This works around an issue with aliased structural types. Also, use the ID provided by the module's ID generator instead of the `skModule`s position. While both represent the same thing (a ``FileIndex``), querying the ID generator is more consistent with how it works elsewhere.
A module ID isn't necessarily the same before and after loading/storing a rodfile. Loading and storing of `LibId`s now considers this.
zerbina
added
refactor
saem
approved these changes
Jul 15, 2023
| @@ -1615,6 +1615,20 @@ type | |||
| name*: Rope | |||
| path*: PNode ## can be a string literal! | |||
|
|
|||
| LibId* = object | |||
| ## Identifies a ``TLib`` instance. The default value means 'none'. | |||
There was a problem hiding this comment.
Might be worth adding a unknownLibId or the like as a constant below to further 'reserve' the concept of the default value LibId(module: 0, index: 0) as none.
Update: reading more of the PR, there is a similar tuple type for IC, so we wouldn't end up using that const/this object type all that much. Seems less worth it practically speaking, mostly there to signal intent.
| # the index is 1-based, so we can directly use the length | ||
| result = LibId(module: module.int32, index: g.libs[module].len.uint32) | ||
|
|
||
| proc storeLibs*(g: ModuleGraph, module: int) = |
There was a problem hiding this comment.
Suggested change
| proc storeLibs*(g: ModuleGraph, module: int) = | |
| proc ownTheLibs*(g: ModuleGraph, module: int) = |
jk jk 😆
|
/merge |
|
Merge requested by: @saem Contents after the first section break of the PR description has been removed and preserved below:
|
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.
Summary
Replace storing
TLibobjects asrefs inTSymwith storing allTLibinstances belonging to a module in a sequence that aTSymthenstores an index-like reference into.
This makes serializing the symbols and associated
TLibinstanceseasier: instead of storing a
PackedLibfor each packed symbol, thepacked symbols now only store a module+index tuple.
Details
For simplicity, the list with the libs for a module is stored in a
separate
seqinModuleGraph. The list cannot be stored inPContext, as it needs to be accessible after semantic analysis, atwhich point
PContextis already gone.While the handle to the
TLibinstance stored withTSymwouldideally be a single index into the
TLiblist of the module asymbol is attached to, the way symbols of aliased structural types
currently work (the aliased type's
TSymcontent is partiallycopied) makes this not possible, meaning that a symbol also has to
store the ID of the module the referenced
TLibinstance is partof.
Storing the
TLibobjects for a module happens when the module isclosed. While not strictly necessary at the moment, this is a
preparation for storing a
PSyminTLib.namethat is only setupwhen the module is closed.
Aside from reducing the size of rodfiles, not storing a
PackedLibfor each
PackedSymalso fixes another inefficiency, namely thateach deserialized
TSymgot a unique, possibly duplicated,TLibinstance.