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

Speed up writing to global variables #3579

Merged
merged 2 commits into from
Jul 18, 2019

Conversation

ChrisJefferson
Copy link
Contributor

@ChrisJefferson ChrisJefferson commented Jul 16, 2019

This PR is based around the observation that when we assign a global variable, we have to check iExprsGVar, FopiesGVar or CopiesGVar for that global variable. However, for most global variables they are set to their default values. This PR adds a single flag which tells us, for each gvar, us if any of these 3 arrays have a non-default value.

To avoid introducing another array, we take WriteGVars (which stores one of three values, if a gvar is mutable, read-only or constant), and make it into FlagsGVars, which now stores this original information, and an extra bit that tells us if any of Exprs,FOpies or Copies is not default. As we have to read WriteGVars anyway, whenever we assign a global, this means there are no extra memory lookups, just the unpacking of the flags.

For simplicity, this new bit is never cleared once it is set. This means when it is true we don't know for certain one of exprsgvar, fopiesgvar or copiesgvar is set to a non-default value, but if it is false we know they are are default.

In a tight loop assigning a global, this PR speeds things up by about 40%

  • Release Notes: Speed up writing to global variables

@ChrisJefferson ChrisJefferson added kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Jul 16, 2019
@fingolfin fingolfin changed the title Speed up gvars Speed up writing to global variables Jul 16, 2019
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.

great job! Some quick remarks, I'll have a proper look after some sleep

src/gvars.c Outdated Show resolved Hide resolved
src/gvars.c Outdated Show resolved Hide resolved
src/gvars.c Outdated Show resolved Hide resolved
src/gvars.c Outdated Show resolved Hide resolved
src/gvars.c Show resolved Hide resolved
src/gvars.c Outdated Show resolved Hide resolved
src/gvars.c Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jul 17, 2019

Codecov Report

Merging #3579 into master will increase coverage by 1.57%.
The diff coverage is 98.03%.

@@            Coverage Diff             @@
##           master    #3579      +/-   ##
==========================================
+ Coverage   83.47%   85.05%   +1.57%     
==========================================
  Files         693      650      -43     
  Lines      344698   319856   -24842     
==========================================
- Hits       287749   272049   -15700     
+ Misses      56949    47807    -9142
Impacted Files Coverage Δ
src/gvars.c 89.94% <98.03%> (+3.38%) ⬆️
lib/random.g 66.66% <0%> (-30.56%) ⬇️
lib/cache.gi 70.76% <0%> (-24.62%) ⬇️
lib/read2.g 85.71% <0%> (-14.29%) ⬇️
lib/read1.g 85.36% <0%> (-13.42%) ⬇️
lib/helpview.gi 28.77% <0%> (-9.98%) ⬇️
lib/random.gd 91.3% <0%> (-8.7%) ⬇️
lib/methsel2.g 42.66% <0%> (-8%) ⬇️
lib/package.gd 93.47% <0%> (-6.53%) ⬇️
grp/perf.gd 94.44% <0%> (-5.56%) ⬇️
... and 277 more

@coveralls
Copy link

coveralls commented Jul 17, 2019

Coverage Status

Coverage decreased (-0.0001%) to 84.301% when pulling e787b0b on ChrisJefferson:speed-gvars into 0b963ef on gap-system:master.

@ChrisJefferson
Copy link
Contributor Author

I ended up merging the commits in this PR together, because both of them had been changed so that the middle one wasn't actually buildable any more, and I didn't think it was worth fixing.

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.

Fine by me (you may or may not want to fix that typo, it's not important)

src/gvars.c Outdated Show resolved Hide resolved
@fingolfin
Copy link
Member

(And by "fine by me" I obviously meant: Very nice improvement, thank you!! Obviously. Ahem. :-)

@fingolfin fingolfin merged commit 7302780 into gap-system:master Jul 18, 2019
@wucas wucas 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 Aug 20, 2019
@ChrisJefferson ChrisJefferson deleted the speed-gvars branch August 23, 2019 10:55
@olexandr-konovalov olexandr-konovalov added this to the GAP 4.11.0 milestone Aug 26, 2019
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.

5 participants