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

Make ValueGlobal faster #2232

Merged
merged 1 commit into from
Mar 7, 2018
Merged

Conversation

sebasguts
Copy link
Member

@sebasguts sebasguts commented Mar 2, 2018

... by turning ValueGlobal into a synonym of VALUE_GLOBAL.

The check if the argument of ValueGlobal is a string is done by VALUE_GLOBAL, too, so this check is not necessary.

The fact if all characters in the argument string are in IdentifierLetters does not cause an error, just an info. Furthermore, variables with different characters in their identifier, e.g., \ , can still be written to and accessed.

@ChrisJefferson
Copy link
Contributor

Looks like @stevelinton may have written this file originally.

My main comment is that I don't really like just doing this to ValueGlobal, when there are many other functions in the same file (IsBoundGlobal, MakeReadOnlyGlobal,...), which all have a similar design to ValueGlobal. I would prefer we simplify all of them, or none of them, unless there is a good reason to just change one.

@stevelinton
Copy link
Contributor

That was me. A very long time ago.

I don't really like the use of DeclareSynonym here. Better to simply install VALUE_GLOBAL as the value of the global function -- performancewise it should make no difference, but it seems nicer to read that way, and (since all the declared functions now have some kind of implementation in the .gi file, makes it less urgent to adjust the other functions that @ChrisJefferson suggests.

@codecov
Copy link

codecov bot commented Mar 2, 2018

Codecov Report

Merging #2232 into master will decrease coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2232      +/-   ##
==========================================
- Coverage   69.88%   69.88%   -0.01%     
==========================================
  Files         481      481              
  Lines      252990   252986       -4     
==========================================
- Hits       176814   176805       -9     
- Misses      76176    76181       +5
Impacted Files Coverage Δ
lib/global.gi 40.14% <ø> (-1.7%) ⬇️
hpcgap/lib/hpc/queue.g 66.4% <0%> (-3.2%) ⬇️
src/hpc/threadapi.c 36.9% <0%> (-0.1%) ⬇️

lib/global.gd Outdated
@@ -61,7 +61,7 @@ DeclareGlobalFunction("IsValidIdentifier");
## </Description>
## </ManSection>
##
DeclareGlobalFunction("ValueGlobal");
DeclareSynonym("ValueGlobal", VALUE_GLOBAL);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @stevelinton : I'd leave this declaration here, and then just change the .gi file to InstallGlobalFunction( ValueGlobal, VALUE_GLOBAL ), simply for symmetry with the rest.

The difference is in the function names:

gap> DeclareSynonym("foo", VALUE_GLOBAL);
gap> DeclareGlobalFunction("bar");
gap> InstallGlobalFunction( bar, VALUE_GLOBAL );
gap> NameFunction(foo);
"VAL_GVAR"
gap> NameFunction(bar);
"bar"

Which reminds me of another thing: VALUE_GLOBAL itself is just an alias for VALUE_GVAR, and it is not even write protected: You can e.g. do VALUE_GLOBAL := 1...

So we may want to unify that, too. But of course we have to be careful because there are packages using almost any of the three names in provided for each of the functions dealing with globals sigh. (Strictly speaking, packages using the undocumented functions VALUE_GLOBAL, VALUE_GVAR etc are doing something wrong, and we are not responsible for keeping them working, but, well, let's be realistic, people will still yell at us, not the package authors, if we change this without either providing backward compatibility code, or else getting all packages using it fixed first....

and made it a synonym for VALUE_GLOBAL
@sebasguts
Copy link
Member Author

I adjusted the commit and the PR to @stevelinton 's suggestions. I also agree completely to @ChrisJefferson 's suggestions, by making that change to many of the functions in global.gd. But I think this could be done in a different PR.

@fingolfin
Copy link
Member

@sebasguts thanks, but now the commit message does not fit :-).

I think the commit message should mention that calling CheckGlobalName really is not necessary, because if the name was not valid internally, it'll just do nothing.

BTW, while I won't miss the Info call there or anywhere else, I wonder if others might? I have a hard time seeing what they would be useful for, ever, but that just shows my lack of imagination. Perhaps @stevelinton can give some insights?

Anyway, if people were to miss it, I wonder if just removing the call to CheckGlobalName resolves your problems?

Final remark: While I do not expect you to make other changes, I'd like to point out that most other CheckGlobalName calls could be renamed with the same argument; I think really only the ones in BindGlobal and BindConstant matter.

@sebasguts
Copy link
Member Author

@fingolfin Why does the message not fit? It is still a synonym, just not in the technical sense. But I can change it if you like. Then I can also point out why the call is not necessary.

I also generally agree that the call for CheckGlobalName is not necessary in almost all cases. I think even remove it for BindGlobal since the fact if the input is a string is still checked by the kernel function, and the checked letters are just documented as "the usual characters in an identifier".

Copy link
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I guess the commit message is fine after all.

@fingolfin
Copy link
Member

I have approved this PR, but I'd like @ChrisJefferson and/or @stevelinton to decide its ultimate fate for now.

@ChrisJefferson ChrisJefferson merged commit bd9d468 into gap-system:master Mar 7, 2018
@fingolfin fingolfin changed the title Removed function for ValueGlobal Turn ValueGlobal into synonym for VALUE_GLOBAL, thus avoiding slow CheckGlobalName check Mar 28, 2018
@fingolfin fingolfin added topic: performance bugs or enhancements related to performance (improvements or regressions) topic: library labels Mar 28, 2018
@fingolfin fingolfin changed the title Turn ValueGlobal into synonym for VALUE_GLOBAL, thus avoiding slow CheckGlobalName check Avoid slow and unnecessary CheckGlobalName check in ValueGlobal Mar 28, 2018
@fingolfin fingolfin changed the title Avoid slow and unnecessary CheckGlobalName check in ValueGlobal Make ValueGlobal faster Sep 28, 2018
@fingolfin fingolfin added the release notes: added PRs introducing changes that have since been mentioned in the release notes label Sep 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: added PRs introducing changes that have since been mentioned in the release notes topic: library topic: performance bugs or enhancements related to performance (improvements or regressions)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants