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

Track location of InstallMethod and InstallImmediateMethod #2393

Merged
merged 5 commits into from
Apr 23, 2018

Conversation

fingolfin
Copy link
Member

@fingolfin fingolfin commented Apr 23, 2018

This PR makes the information displayed by TraceMethods and by ApplicableMethod much more useful in many case, which resolves #2239. For example, consider this method for AsList, installed in coll.gi:

InstallMethod( AsList,
    "for collections that are constant time access lists",
    [ IsCollection and IsConstantTimeAccessList ],
    Immutable );

With GAP 4.8, no location information is shown at all:

gap> TraceMethods(AsList); AsList("abc");;
#I  AsList: for collections that are constant time access lists

With GAP 4.9 / master, the location where the method (i.e., the
underlying function) was defined is shown, which already does the job in
many cases, but not here:

gap> TraceMethods(AsList); AsList("abc");;
#I  AsList: for collections that are constant time access lists at src/objects.c:fail

With this PR, we get this (note that instead of GAPROOT, the actual path
is printed)

gap> TraceMethods(AsList); AsList("abc");;
#I  AsList: for collections that are constant time access lists at GAPROOT/lib/coll.gi:302

For we immediate methods, this is what we got before this PR:

gap> TraceImmediateMethods( );
gap> g:= Group( (1,2,3), (1,2) );;
#I  immediate: Size
#I  immediate: IsCyclic
#I  immediate: IsCommutative
#I  immediate: IsTrivial
gap>
gap> M0:=Magma(FamilyObj([1]), []);;
gap> AsSSortedList(M0);
#I  immediate: RepresentativeSmallest
#I  immediate: AsList
#I  immediate: EnumeratorSorted
#I  immediate: GeneratorsOfDomain
#I  immediate: Size
#I  immediate: RepresentativeSmallest
#I  immediate: IsFinite
#I  immediate: IsTrivial
#I  immediate: IsEmpty
#I  immediate: IsNonTrivial
#I  immediate: Size
#I  immediate: RepresentativeSmallest
[  ]

After (note how e.g. the entry for EnumeratorSorted points at
coll.gi:379, and not at the location where the method function
was defined):

gap> TraceImmediateMethods( );
gap> g:= Group( (1,2,3), (1,2) );;
#I RunImmediateMethods
#I  immediate: Size at GAPROOT/lib/coll.gi:179
#I  immediate: IsCyclic at GAPROOT/lib/grp.gi:40
#I  immediate: IsCommutative at GAPROOT/lib/magma.gi:196
#I  immediate: IsTrivial at GAPROOT/lib/magma.gi:130
gap>
gap> M0:=Magma(FamilyObj([1]), []);;
#I RunImmediateMethods
#I RunImmediateMethods
gap> AsSSortedList(M0);
#I RunImmediateMethods
#I  immediate: RepresentativeSmallest at GAPROOT/lib/coll.gi:226
#I  immediate: AsList at GAPROOT/lib/domain.gi:206
#I  immediate: EnumeratorSorted at GAPROOT/lib/coll.gi:379
#I  immediate: GeneratorsOfDomain at GAPROOT/lib/domain.gi:181
#I  immediate: Size at GAPROOT/lib/coll.gi:183
#I  immediate: RepresentativeSmallest at GAPROOT/lib/coll.gi:215
#I  immediate: IsFinite at GAPROOT/lib/coll.gi:148
#I  immediate: IsTrivial at GAPROOT/lib/coll.gi:116
#I  immediate: IsEmpty at GAPROOT/lib/coll.gi:97
#I  immediate: IsNonTrivial at GAPROOT/lib/coll.gi:134
#I RunImmediateMethods
#I  immediate: Size at GAPROOT/lib/coll.gi:179
#I RunImmediateMethods
#I  immediate: RepresentativeSmallest at GAPROOT/lib/coll.gi:215
[  ]

@fingolfin fingolfin added the kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements label Apr 23, 2018
@fingolfin
Copy link
Member Author

BTW, while I found outputting RunImmediateMethods helpful for debugging, not everybody might agree, so I can remove this again. Or perhaps we should have different levels of tracing: off = level 0; level 1 = only print the invoked methods; level 2: also print the RunImmediateMethods string, and perhaps in addition, print the value of RUN_IMMEDIATE_METHODS_RUNS (and print RUN_IMMEDIATE_METHODS_CHECKS next to each executed method).
We then might also want to use an InfoLevel (and perhaps Info, though I am less clear on that; in any case, one can use an InfoLevel without Info just fine)

Copy link
Contributor

@hulpke hulpke left a comment

Choose a reason for hiding this comment

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

Very useful.

Copy link
Contributor

@ThomasBreuer ThomasBreuer left a comment

Choose a reason for hiding this comment

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

This is a nice feature.
I think it is not necessary to make the output of
RunImmediateMethods optional.

... for accessing the methods in METHODS_OPERATION(oper,n). By using
this constant, it becomes easier to add additional entries for each
method.

Note that the layout of METHODS_OPERATION is strictly speaking an
internal implementation detail, and thus packages directly accessing it
do so at their own risk. If a package accesses METHODS_OPERATION anyway
(currently only Browse) it should be updated to use METHODS_OPERATION.
(For backwards compatibility, if BASE_SIZE_METHODS_OPER_ENTRY is not
defined, it can be set to 4).
... to make it a bit easier to figure out what's going on
This makes the information displayed by TraceMethods and by
ApplicableMethod much more useful in many case, which resolves gap-system#2239.
For example, consider this method for `AsList`, installed in coll.gi:

    InstallMethod( AsList,
        "for collections that are constant time access lists",
        [ IsCollection and IsConstantTimeAccessList ],
        Immutable );

With GAP 4.8, no location information is shown at all:

    gap> TraceMethods(AsList); AsList("abc");;
    #I  AsList: for collections that are constant time access lists

With GAP 4.9 / master, the location where the method (i.e., the
underlying function) was defined is shown, which already does the job in
many cases, but not here:

    gap> TraceMethods(AsList); AsList("abc");;
    #I  AsList: for collections that are constant time access lists at src/objects.c:fail

With this PR, we get this (note that instead of GAPROOT, the actual path
is printed)

    gap> TraceMethods(AsList); AsList("abc");;
    #I  AsList: for collections that are constant time access lists at GAPROOT/lib/coll.gi:302
Before:

    gap> TraceImmediateMethods( );
    gap> g:= Group( (1,2,3), (1,2) );;
    #I  immediate: Size
    #I  immediate: IsCyclic
    #I  immediate: IsCommutative
    #I  immediate: IsTrivial
    gap>
    gap> M0:=Magma(FamilyObj([1]), []);;
    gap> AsSSortedList(M0);
    #I  immediate: RepresentativeSmallest
    #I  immediate: AsList
    #I  immediate: EnumeratorSorted
    #I  immediate: GeneratorsOfDomain
    #I  immediate: Size
    #I  immediate: RepresentativeSmallest
    #I  immediate: IsFinite
    #I  immediate: IsTrivial
    #I  immediate: IsEmpty
    #I  immediate: IsNonTrivial
    #I  immediate: Size
    #I  immediate: RepresentativeSmallest
    [  ]

After (note how e.g. the entry for EnumeratorSorted points at
coll.gi:379, and not at the location where the method function
was defined):

    gap> TraceImmediateMethods( );
    gap> g:= Group( (1,2,3), (1,2) );;
    #I RunImmediateMethods
    #I  immediate: Size at GAPROOT/lib/coll.gi:179
    #I  immediate: IsCyclic at GAPROOT/lib/grp.gi:40
    #I  immediate: IsCommutative at GAPROOT/lib/magma.gi:196
    #I  immediate: IsTrivial at GAPROOT/lib/magma.gi:130
    gap>
    gap> M0:=Magma(FamilyObj([1]), []);;
    #I RunImmediateMethods
    #I RunImmediateMethods
    gap> AsSSortedList(M0);
    #I RunImmediateMethods
    #I  immediate: RepresentativeSmallest at GAPROOT/lib/coll.gi:226
    #I  immediate: AsList at GAPROOT/lib/domain.gi:206
    #I  immediate: EnumeratorSorted at GAPROOT/lib/coll.gi:379
    #I  immediate: GeneratorsOfDomain at GAPROOT/lib/domain.gi:181
    #I  immediate: Size at GAPROOT/lib/coll.gi:183
    #I  immediate: RepresentativeSmallest at GAPROOT/lib/coll.gi:215
    #I  immediate: IsFinite at GAPROOT/lib/coll.gi:148
    #I  immediate: IsTrivial at GAPROOT/lib/coll.gi:116
    #I  immediate: IsEmpty at GAPROOT/lib/coll.gi:97
    #I  immediate: IsNonTrivial at GAPROOT/lib/coll.gi:134
    #I RunImmediateMethods
    #I  immediate: Size at GAPROOT/lib/coll.gi:179
    #I RunImmediateMethods
    #I  immediate: RepresentativeSmallest at GAPROOT/lib/coll.gi:215
    [  ]
@codecov
Copy link

codecov bot commented Apr 23, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@44a4f35). Click here to learn what that means.
The diff coverage is 72.38%.

@@            Coverage Diff            @@
##             master    #2393   +/-   ##
=========================================
  Coverage          ?   73.54%           
=========================================
  Files             ?      482           
  Lines             ?   240853           
  Branches          ?        0           
=========================================
  Hits              ?   177142           
  Misses            ?    63711           
  Partials          ?        0
Impacted Files Coverage Δ
lib/profile.g 13.8% <0%> (ø)
src/c_type1.c 84.01% <100%> (ø)
src/opers.c 92% <100%> (ø)
hpcgap/src/c_type1.c 81.82% <100%> (ø)
lib/methsel.g 43.24% <38.46%> (ø)
lib/methwhy.g 50% <57.14%> (ø)
hpcgap/src/c_oper1.c 89.03% <74.6%> (ø)
src/c_oper1.c 90.42% <75.19%> (ø)
lib/oper.g 81.18% <77.77%> (ø)

@fingolfin fingolfin merged commit 31fcade into gap-system:master Apr 23, 2018
@fingolfin fingolfin deleted the mh/track-installmethod branch April 23, 2018 18:17
@fingolfin
Copy link
Member Author

@ThomasBreuer this, however, breaks a function in Browse for browsing methods. To fix this, Browse needs to be changed to use BASE_SIZE_METHODS_OPER_ENTRY when it iterates over the methods (this will make it future proof against further additions). To keep it compatible with older GAP versions, you could simply bind it to the old value 4 if missing.

@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 Jul 31, 2018
@fingolfin fingolfin 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 Sep 20, 2018
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.

Idea: record location of InstallMethod call
3 participants