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.

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

Extend BindConstant, MakeConstantGlobal to all object types. #4142

Merged
merged 1 commit into from
Oct 30, 2020

Conversation

fingolfin
Copy link
Member

Currently we only support these on integer and boolean values. This patch
extends this to all object types. This would then simply be a "non-reversible"
version of MakeReadOnlyGlobal; i.e., unlike that, a "constant global
variables" can never be turned into a non-constant one. That is, it will
always refer to the same object.

Semantically this interpretation of constness is in line with other
languages, such as Julia, where const refers to the (re)assignability of a
variable identifier, not to the mutability of the object it refers to. (Note
that this is admittedly somewhat confusing in GAP, with
MakeReadOnlyGlobal/MakeReadOnlyGVar doing one thing and MakeReadOnlyObj
doing something orthogonal (and for now only in HPC-GAP....

Note that no further use is made of this for now; in particular, unlike for
small integer values, true and false, no attempt is made to "inline"
constant object in function definitions. However, interfaces to other
languages (such as the GAP-Julia interface) could exploit constness for
various optimizations (cc @rfourquet).

Currently we only support these on integer and boolean values. This patch
extends this to all object types. This would then simply be a "non-reversible"
version of `MakeReadOnlyGlobal`; i.e., unlike that, a "constant global
variables" can never be turned into a non-constant one. That is, it will
always refer to the same object.

Semantically this interpretation of `constness` is in line with other
languages, such as Julia, where `const` refers to the (re)assignability of a
variable identifier, *not* to the mutability of the object it refers to. (Note
that this is admittedly somewhat confusing in GAP, with
`MakeReadOnlyGlobal`/`MakeReadOnlyGVar` doing one thing and `MakeReadOnlyObj`
doing something orthogonal (and for now only in HPC-GAP....

Note that no further use is made of this for now; in particular, unlike for
small integer values, `true` and `false`, no attempt is made to "inline"
constant object in function definitions. However, interfaces to other
languages (such as the GAP-Julia interface) could exploit constness for
various optimizations.
@fingolfin fingolfin added the release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes label Oct 16, 2020
@ChrisJefferson
Copy link
Contributor

I'm happy with this -- the original restriction to bools and ints was because we can make those more efficient, and I wanted to check there were no unpleasent surpries before extending.

I haven't checked how this interacts with SwapMasterPoint -- I imagine it will be fine, but certainly people who want to use this for other stuff should be aware of that (or choose explictly to ignore it).

@fingolfin
Copy link
Member Author

Good point! SwapMasterPoint on the C / kernel level, and SWITCH_OBJ and FORCE_SWITCH_OBJ on the GAP / library level are indeed an evil that one also needs to deal with to really be able to safely exploit constness in e.g. the GAP-Julia interface. And perhaps also CLONE_OBJ...

Of course SwapMasterPoint operates at the object level, so it "knows nothing" about "global variables" or "constness" in the sense here. But for my purposes, it would already be enough if SwapMasterPoint was restricted to swapping objects of the same type; or at least would enforce that restriction if one of the two objects is "type locked", as proposed in issue #4143. That also gives a hint as to what I really want: being able to exploit this for the GAP-Julia bridge, so I can conclude "this global variable will always refer to that object, and that will always be a T_FUNCTION" and then use that to produce optimal access code on the Julia.

@coveralls
Copy link

Coverage Status

Coverage increased (+8.0e-05%) to 93.73% when pulling d75a402 on fingolfin:mh/generalize-bindconstant into dcd59d0 on gap-system:master.

@fingolfin
Copy link
Member Author

@ChrisJefferson if you are happy with this, does that mean you approve (hint, hint ;-) ) or perhaps you'd like us to get a third opinion?

@fingolfin fingolfin merged commit 57509de into gap-system:master Oct 30, 2020
@fingolfin fingolfin deleted the mh/generalize-bindconstant branch October 30, 2020 14:23
@ThomasBreuer ThomasBreuer self-assigned this Feb 16, 2021
@ThomasBreuer ThomasBreuer changed the title Extend BindConstant, MakeConstantGlobal to all object types Extend BindConstant, MakeConstantGlobal to all object types. Feb 16, 2021
@ThomasBreuer ThomasBreuer added release notes: added PRs introducing changes that have since been mentioned in the release notes and removed release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Feb 16, 2021
@ThomasBreuer ThomasBreuer removed their assignment Feb 16, 2021
@fingolfin fingolfin added the kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements label Aug 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements release notes: added PRs introducing changes that have since been mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants