kernel: change how ours GCs represent master pointers #4937
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
fingolfin
added
do not merge
fingolfin
force-pushed
the
mh/gc-change-mptr
branch
4 times, most recently
from
e5a12fd
to
9d58fcb
Compare
|
Just to say, I'm generally in support of this -- I don't think it will make things any worse for GAP, and I can understand how it will improve integration with other systems. Good luck! |
fingolfin
force-pushed
the
mh/gc-change-mptr
branch
from
9d58fcb
to
2389fa6
Compare
fingolfin
changed the title
fingolfin
force-pushed
the
mh/gc-change-mptr
branch
from
2389fa6
to
bb1db32
Compare
fingolfin
added
release notes: not needed
fingolfin
force-pushed
the
mh/gc-change-mptr
branch
2 times, most recently
from
9d968fd
to
77a8694
Compare
fingolfin
force-pushed
the
mh/gc-change-mptr
branch
from
77a8694
to
2e2d117
Compare
ChrisJefferson
approved these changes
Sep 23, 2022
fingolfin
force-pushed
the
mh/gc-change-mptr
branch
from
2e2d117
to
d0b757e
Compare
So far, our master pointers are pointers to a pointer to the data portion of a bag. This patch changes them to instead point at the start of the bag. Pointing inside the bag, as was done before, was am optimization for the common case where one wants to access the content of a bag. However, at least on x86 and ARM architectures, there is no difference in practice, because adding an offset of 16 (GASMAN) resp. 8 (Boehm, Julia) is essentially free for most machine code opcodes used to access the data.
fingolfin
force-pushed
the
mh/gc-change-mptr
branch
from
d0b757e
to
69dd40f
Compare
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.
So far, our master pointers are pointers to a pointer to the data portion of a bag. This patch changes them to instead point at the start of the bag.
Pointing inside the bag, as was done before, was am optimization for the common case where one wants to access the content of a bag. However, at least on x86 and ARM architectures, there is no difference in practice, because adding an offset of 16 (GASMAN) resp. 8 (Boehm, Julia) is essentially free for most machine code opcodes used to access the data.
So this should not introduce a drawback, but at the same time there is no real advantage to doing this.
BUT of course there is still a reason why i want to do this: I need this for experimental work on the Julia GC integration (in an attempt to resolve oscar-system/GAP.jl#817). But right now the result is crashing, in the Julia GC -- hence I adapted the other GCs to also use this mode, so I can test that everything is fine in GASMAN with this change. It is locally for me, but having it here as a PR gives a chance for more extensive CI tests to check it; also, the change is ABI breaking, so testing it on a "clean slate" is better than on my system.
tl;dr: this PR is not for merging but to allow me to experiment