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

Simplify caching logic in GeneralMappingsFamily #1888

Merged
merged 1 commit into from
Nov 13, 2017

Conversation

fingolfin
Copy link
Member

Previously, the cache consisted of a weak list with entries alternating
between "keys" (= range domain) and "values" (= the corresponding mapping
family).

But the "values" contain the keys (via FamilyRange(fam). By exploiting that,
we can simplify the caching logic. The resulting code is very similar to the
code in DirectProductElementsFamily, suggesting that we can eventually replace
both with a common abstraction.

Previously, the cache consisted of a weak list with entries alternating
between "keys" (= range domain) and "values" (= the corresponding mapping
family).

But the "values" contain the keys (via FamilyRange(fam). By exploiting that,
we can simplify the caching logic. The resulting code is very similar to the
code in DirectProductElementsFamily, suggesting that we can eventually replace
both with a common abstraction.
@fingolfin fingolfin added the release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes label Nov 10, 2017
@codecov
Copy link

codecov bot commented Nov 10, 2017

Codecov Report

Merging #1888 into master will increase coverage by 12.75%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           master    #1888       +/-   ##
===========================================
+ Coverage   50.92%   63.67%   +12.75%     
===========================================
  Files         438      946      +508     
  Lines      232003   284493    +52490     
  Branches    10442    12746     +2304     
===========================================
+ Hits       118138   181164    +63026     
+ Misses     111082   100528    -10554     
- Partials     2783     2801       +18
Impacted Files Coverage Δ
lib/mapping.gi 79.68% <100%> (+0.75%) ⬆️
hpcgap/lib/mapping.gi 78.34% <100%> (ø)
src/funcs.c 77.27% <0%> (-13.06%) ⬇️
src/intobj.h 81.48% <0%> (-7.71%) ⬇️
src/dteval.c 2.79% <0%> (-0.29%) ⬇️
src/dt.c 1.61% <0%> (-0.26%) ⬇️
hpcgap/pkg/gapdoc/lib/GAPDoc2LaTeX.gi 1.49% <0%> (-0.14%) ⬇️
hpcgap/pkg/gapdoc/lib/GAPDoc2Text.gi 0.53% <0%> (-0.05%) ⬇️
src/range.h 100% <0%> (ø) ⬆️
src/compiled.h 0% <0%> (ø) ⬆️
... and 824 more

Copy link
Contributor

@stevelinton stevelinton left a comment

Choose a reason for hiding this comment

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

Clearly correct, simpler, and probably little slower, if slower at all.

@fingolfin fingolfin merged commit 168fceb into gap-system:master Nov 13, 2017
@fingolfin fingolfin deleted the mh/cache branch November 13, 2017 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants