Add tracing and counting of built-in operations #3713

ChrisJefferson · 2019-10-23T14:37:09Z

This is some code I've had a branch of a long time, cleaned up.

Sometimes, for papers and general research it is interesting to know things like how many times + is applied to integers, or * or ^ is applied to permutations.

This can't be done by any of GAP's normal tracing functionality at present. Also, printing a message every time one of these functions is called would be far too much.

This adds TraceInternalMethods, which produces a record which contains how many times each operation is applied to each tnum (or each pair of tnums for 2-argument methods).

This doesn't try to catch when kernel functions add integers, only when these operations are performed at the GAP level.

This still needs tests, but before I put more work into polishing I wanted to see if other people found it useful.

coveralls · 2019-10-23T16:30:05Z

Coverage remained the same at 84.691% when pulling e1d8612 on ChrisJefferson:tracking_ops into e6d5121 on gap-system:master.

DominikBernhardt

I think this is a helpful function, I've missed it in the past, so I would be happy for it to go into GAP. I have a few suggestions, see above. Also, it would be nice to add tests before merging this.

lib/oper.g

src/tracing.c

src/tracing.h

ChrisJefferson · 2019-11-25T20:27:10Z

Thanks for the corrections. I've just put an explicit list of the "internal methods" which are traced, along with the name GAP gives them (which is the name of the internal variable, but might not be obvious so is worth explictly stating I think)

DominikBernhardt

Thanks for implementing the changes, I am much happier with this now.
I've left 3 more comments. Also, I would leave it at changes requested until there are tests so this doesn't get merged accidentally?

lib/oper.g

tst/testinstall/trace.tst

DominikBernhardt · 2019-11-28T02:50:09Z

lib/oper.g

+## gap> 3^(1,2,3);;
+## gap> GetTracedInternalMethodsCounts();
+## rec( Pow := rec( integer := rec( ("permutation (small)") := 1 ) ),
+##      Sum := rec( integer := rec( integer := 4 ), macfloat := rec( macfloat := 1 ) ) )


In the compiled documentation, the last ) is outside of the rectangular box in which Examples are printed. Thus, it looks like a overfull hbox. Maybe you could insert another line break?

If this is tested by Travis etc, then Chris doesn't have any choice about how the output should look; if he adds arbitrary line breaks, then the tests will fail when the manual examples are tested. However I'm not sure where this is ever tested by Travis.

Oh ok, I see. I guess I won't take the risk of a manual formatting now because it might be tested in the future and thing could break?

THis formatting was actually done by me, so I'll shorten/neaten it

DominikBernhardt · 2019-11-28T02:51:48Z

Thanks @ChrisJefferson . Overall, I am happy with this now. If you could address the small points above I am happy to approve this and to get in merged.

DominikBernhardt · 2019-12-03T02:46:15Z

@ChrisJefferson Tests are failing now. Maybe you could have a look at that? Also, while you're at it, could you change the documentation and change the is to are as suggested above?

lib/oper.g

DominikBernhardt

Thanks for all the changes @ChrisJefferson

wilfwilson

Looks sensible to me 🙂

fingolfin

I didn't see this PR before, and just glancing at it, I wonder about several things, and thus would greatly prefer if this was not merged for now, until I have a chance to properly look at it (i.e., not while sitting in a train with a crappy internet connection).

fingolfin · 2019-12-19T09:00:09Z

src/objects.c

@@ -875,6 +875,12 @@ static Obj FuncMakeImmutable(Obj self, Obj obj)
  return obj;
 }

+static Obj FuncGET_TNAM_FROM_TNUM(Obj self, Obj obj)
+{
+    UInt         tnum = GetBoundedInt("GET_TNAM_FROM_TNUM", obj, 0, 255);


255 should have been NUM_TYPES - 1

fingolfin · 2019-12-19T13:14:52Z

lib/oper.g

+##  <Mark>LQuo</Mark><Item>The left-quotient operator</Item>
+##  <Mark>Pow</Mark><Item>The operator <Ref Oper="\^"/></Item>
+##  <Mark>Comm</Mark><Item>The operator <Ref Oper="Comm"/></Item>
+##  <Mark>Mod</Mark><Item>The operator <Ref Oper="\mod"/></Item>


I find the above discussion rather confusing. It mixes up methods and operations: This text sounds as if Zero, ZeroMut, etc. are the "internal methods". But really, these are operations which have a special implementation such that before all regular methods, a special internal method is considered; and only if that is not applicable or fails or whatever, are other (regular) methods tried.

fingolfin · 2019-12-19T13:15:45Z

lib/oper.g

+##  As these methods can be called hundreds of thousands of times in simple GAP
+##  code, there isn't a statement printed each time one is called. Instead, the
+##  method <Ref Func="GetTraceInternalMethodsCounts"/> returns how many times
+##  each operation has been applied to each type of variable (the type of a


Variables don't have a type in GAP, so "variable" should be changed to "object" here and in the next line (I can't make change suggestions on closed PRs, I am afraid).

fingolfin · 2019-12-19T13:16:30Z

lib/oper.g

+##  <Mark>Inv, InvMut</Mark><Item>Mutable and Immutable <Ref Attr="Inverse"/></Item>
+##  <Mark>Sum</Mark><Item>The operator <Ref Oper="\+"/></Item>
+##  <Mark>Diff</Mark><Item>The operator <C>-</C> operator</Item>
+##  <Mark>Prod</Mark><Item>The operator <Ref Oper="\*"/></Item>
+##  <Mark>Quo</Mark><Item>The operator <Ref Oper="\/"/></Item>
+##  <Mark>LQuo</Mark><Item>The left-quotient operator</Item>
+##  <Mark>Pow</Mark><Item>The operator <Ref Oper="\^"/></Item>
+##  <Mark>Comm</Mark><Item>The operator <Ref Oper="Comm"/></Item>
+##  <Mark>Mod</Mark><Item>The operator <Ref Oper="\mod"/></Item>
+##  </List>
+##  <P/>
+##  <Ref Func="UntraceInternalMethods"/> turns tracing off.
+##  As these methods can be called hundreds of thousands of times in simple GAP
+##  code, there isn't a statement printed each time one is called. Instead, the
+##  method <Ref Func="GetTraceInternalMethodsCounts"/> returns how many times
+##  each operation has been applied to each type of variable (the type of a
+##  variable can be found with the <C>TNAM_OBJ</C> method).
+##  The return value for two argument operators is a record of records <C>r</C>, where


"The return value of GetTraceInternalMethodsCounts for ..."

fingolfin · 2019-12-19T13:17:29Z

lib/oper.g

+##  <Func Name="TraceInternalMethods" Arg=''/>
+##  <Func Name="UntraceInternalMethods" Arg=''/>
+##  <Func Name="GetTraceInternalMethodsCounts" Arg=''/>
+##  <Func Name="ClearTraceInternalMethodsCounts" Arg=''/>


The documentation says nothing about ClearTraceInternalMethodsCounts

fingolfin · 2019-12-19T14:41:10Z

src/tracing.c

+    tracking_active = 0;
+    return 0;
+}
+static Obj FuncGET_TRACED_INTERNAL_METHODS_COUNTS(Obj self)


Add empty line above

fingolfin · 2019-12-19T14:41:44Z

src/tracing.c

+
+// Store the list of operators which can have tracing enabled and disabled
+static voidfuncs controllers[64];
+static int       tracking_active;


we use CamelCase and not snake_case (nor lowerCamelCase) for global variables (almost) everywhere else...

fingolfin · 2019-12-19T14:43:14Z

src/tracing.c

+void InstallOpWrapper(voidfunc activate, voidfunc deactivate)
+{
+    int pos = 0;
+    while (pos < 64 && controllers[pos].activate != 0) {


Instead of hardcoding 64 here and elsewhere, it would be better to use ARRAYSIZE(controllers). Or use a global enum for the size. Or don't use a size at all, and instead use a PLIST to store this data.

I started switching to PLIST, but it doesn't really work as we have to store C function pointers

fingolfin · 2019-12-19T14:44:18Z

src/tracing.h

+void InstallOpWrapper(voidfunc, voidfunc);
+
+
+// These are macros which make it simple to wrap


This comment looks as if it is incomplete? In any case, it is useless in the current form. Would be nice to have at least some comments explaining what those macros are and how to use them.

fingolfin · 2019-12-19T14:44:32Z

src/tracing.h

+
+void ReportWrappedOperation1(const char *, Obj op);
+void ReportWrappedOperation2(const char *, Obj op1, Obj op2);
+void InstallOpWrapper(voidfunc, voidfunc);


Some comments explaining these functions and how/when to use them would be nice

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 Oct 23, 2019

DominikBernhardt requested changes Nov 24, 2019

View reviewed changes

ChrisJefferson force-pushed the tracking_ops branch from 1669403 to b7cd46d Compare November 25, 2019 20:26

DominikBernhardt requested changes Nov 26, 2019

View reviewed changes

lib/oper.g Outdated Show resolved Hide resolved

lib/oper.g Outdated Show resolved Hide resolved

lib/oper.g Outdated Show resolved Hide resolved

ChrisJefferson force-pushed the tracking_ops branch 3 times, most recently from f28c182 to c174c7a Compare November 27, 2019 11:32

DominikBernhardt reviewed Nov 28, 2019

View reviewed changes

tst/testinstall/trace.tst Show resolved Hide resolved

DominikBernhardt reviewed Nov 28, 2019

View reviewed changes

ChrisJefferson force-pushed the tracking_ops branch from c174c7a to 4ef72b4 Compare December 2, 2019 11:06

ChrisJefferson force-pushed the tracking_ops branch 2 times, most recently from 2f20af0 to 560d652 Compare December 3, 2019 08:33

DominikBernhardt requested changes Dec 4, 2019

View reviewed changes

lib/oper.g Outdated Show resolved Hide resolved

ChrisJefferson force-pushed the tracking_ops branch from 560d652 to dc30014 Compare December 5, 2019 09:25

ChrisJefferson added 2 commits December 5, 2019 14:42

Add function to get the name of a TNAM from the TNUM

a23ad61

Add tracing of built-in operations

e1d8612

ChrisJefferson force-pushed the tracking_ops branch from dc30014 to e1d8612 Compare December 5, 2019 13:42

DominikBernhardt approved these changes Dec 9, 2019

View reviewed changes

wilfwilson approved these changes Dec 18, 2019

View reviewed changes

wilfwilson merged commit 534dc89 into gap-system:master Dec 18, 2019

fingolfin requested changes Dec 18, 2019

View reviewed changes

fingolfin reviewed Dec 19, 2019

View reviewed changes

ChrisJefferson mentioned this pull request Dec 19, 2019

Improve tracing code #3791

Merged

ChrisJefferson deleted the tracking_ops branch April 1, 2020 10:06

PaulaHaehndel self-assigned this Feb 17, 2021

PaulaHaehndel 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 17, 2021

PaulaHaehndel removed their assignment Feb 17, 2021

olexandr-konovalov mentioned this pull request Feb 19, 2021

Simplify the creation of release notes #4257

Closed

olexandr-konovalov mentioned this pull request Mar 23, 2021

Python script to get an overview of changes with PyGithub #4277

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tracing and counting of built-in operations #3713

Add tracing and counting of built-in operations #3713

ChrisJefferson commented Oct 23, 2019

coveralls commented Oct 23, 2019 •

edited

Loading

DominikBernhardt left a comment

ChrisJefferson commented Nov 25, 2019 •

edited

Loading

DominikBernhardt left a comment

DominikBernhardt Nov 28, 2019

wilfwilson Nov 29, 2019

DominikBernhardt Dec 2, 2019

ChrisJefferson Dec 2, 2019

DominikBernhardt commented Nov 28, 2019

DominikBernhardt commented Dec 3, 2019

DominikBernhardt left a comment

wilfwilson left a comment

fingolfin left a comment

fingolfin Dec 19, 2019

fingolfin Dec 19, 2019

fingolfin Dec 19, 2019

fingolfin Dec 19, 2019

fingolfin Dec 19, 2019

fingolfin Dec 19, 2019

fingolfin Dec 19, 2019

fingolfin Dec 19, 2019

ChrisJefferson Dec 19, 2019

fingolfin Dec 19, 2019

fingolfin Dec 19, 2019

		void InstallOpWrapper(voidfunc, voidfunc);


		// These are macros which make it simple to wrap

Add tracing and counting of built-in operations #3713

Add tracing and counting of built-in operations #3713

Conversation

ChrisJefferson commented Oct 23, 2019

coveralls commented Oct 23, 2019 • edited Loading

DominikBernhardt left a comment

Choose a reason for hiding this comment

ChrisJefferson commented Nov 25, 2019 • edited Loading

DominikBernhardt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DominikBernhardt commented Nov 28, 2019

DominikBernhardt commented Dec 3, 2019

DominikBernhardt left a comment

Choose a reason for hiding this comment

wilfwilson left a comment

Choose a reason for hiding this comment

fingolfin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Oct 23, 2019 •

edited

Loading

ChrisJefferson commented Nov 25, 2019 •

edited

Loading