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.

Sign up for GitHub

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

Remove VISITED_STAT #5261

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from
Draft

Remove VISITED_STAT #5261

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
70d4fd7
Stop using VISITED_STAT and SET_VISITED_STAT in profiling
ChrisJefferson Dec 14, 2023
fea3f62
Remove profiledPreviously, as no need to track this any more
ChrisJefferson Dec 14, 2023
682abd3
Remove VISITED_STAT and SET_VISITED_STAT
ChrisJefferson Dec 14, 2023
0ffedbe
Increment minor kernel version to denote changes to profile generation
ChrisJefferson Dec 14, 2023
971d2c0
remove unused function, declare markVisited as static
fingolfin Dec 14, 2023
aaeae1a
Add some comments to profiling and interpreter hooking
ChrisJefferson Jan 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion configure.ac
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ dnl if any interfaces have been added since the last public release, then
dnl increment minor.
dnl
m4_define([kernel_major_version], [9])
m4_define([kernel_minor_version], [0])
m4_define([kernel_minor_version], [1])

m4_define([GAP_DEFINE], [GAP_DEFINES="$GAP_DEFINES -D$1"])

Expand Down
8 changes: 0 additions & 8 deletions src/code.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,14 +121,6 @@ static StatHeader * STAT_HEADER(CodeState * cs, Stat stat)
return (StatHeader *)ADDR_STAT(cs, stat) - 1;
}

void SET_VISITED_STAT(Stat stat)
{
Stat * addr = (Stat *)STATE(PtrBody) + stat / sizeof(Stat);
StatHeader * header = (StatHeader *)addr - 1;
header->visited = 1;
}


static Int TNUM_STAT_OR_EXPR(CodeState * cs, Expr expr)
{
if (IS_REF_LVAR(expr))
Expand Down
28 changes: 0 additions & 28 deletions src/code.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,6 @@
** Header for any statement or expression encoded in a function body.
*/
typedef struct {
// `visited` starts out as 0 and is set to 1 if the statement or
// expression has ever been executed while profiling is turned on
unsigned int visited : 1;

// `line` records the line number in the source file in which the
// statement or expression started
unsigned int line : 31;
Copy link
Member

Choose a reason for hiding this comment

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

Might as well put that bit to use again (and possibly allow generation of slightly nicer code?)

Suggested change
unsigned int line : 31;
unsigned int line : 32;

Expand Down Expand Up @@ -375,30 +371,6 @@ EXPORT_INLINE Int LINE_STAT(Stat stat)
return CONST_STAT_HEADER(stat)->line;
}


/****************************************************************************
**
*F VISITED_STAT(<stat>) . . . . . . . . . . . if statement has even been run
**
** 'VISITED_STAT' returns true if the statement has ever been executed
** while profiling is turned on.
*/
EXPORT_INLINE Int VISITED_STAT(Stat stat)
{
return CONST_STAT_HEADER(stat)->visited;
}


/****************************************************************************
**
*F SET_VISITED_STAT(<stat>) . . . . . . . . . . mark statement as having run
**
** 'SET_VISITED_STAT' marks the statement as having been executed while
** profiling was turned on.
*/
void SET_VISITED_STAT(Stat stat);


/****************************************************************************
**
*F IS_REF_LVAR(<expr>) . . . test if an expression is a reference to a local
Expand Down
4 changes: 4 additions & 0 deletions src/hookintrprtr.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,12 +50,16 @@ extern EvalBoolFunc OriginalEvalBoolFuncsForHook[256];
**
** * 'visitStat' is called for every visited Stat (and Expr) from the
** GAP bytecode.
** * 'visitInterpretedStat' is called when code is executed directly,
** and will not be turned into bytecode. Only the file and line are given.
** * 'enterFunction' and 'leaveFunction' are called whenever a function
** is entered, or left. This is passed the function which is being
** entered (or left)
** * 'registerStat' is called whenever a statement is read from a text
** file. Note that you will only see files which are read while your
** hooks are running.
** * 'registerInterpretedStat' is called when code is read which will
** not be turned into bytecode. Only the file and line are given.
** * 'hookName' is a string is used in debugging messages to describe
** the currently active hooks.
**
Expand Down
72 changes: 51 additions & 21 deletions src/profile.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,12 +149,6 @@
int profiledThread;
#endif

// Have we previously profiled this execution of GAP? We need this because
// code coverage doesn't work more than once, as we use a bit in each Stat
// to mark if we previously executed this statement, which we can't
// clear
UInt profiledPreviously;

Int LongJmpOccurred;

// We store the value of RecursionDepth each time we enter a function.
Expand All @@ -163,6 +157,10 @@
// We need to store the actual values, as RecursionDepth can increase
// by more than one when a GAP function is called
Obj visitedDepths;

// Mark which statements in which files have been visited.
// This is a plist (index by file id) of plists (indexed by line)
Obj visitedStatements;
} profileState;

// Some GAP functionality (such as syntaxtree) evaluates expressions, which makes
Expand Down Expand Up @@ -277,7 +275,8 @@
return copy;
}


// Check if the filename of the file 'id' has even been outputted.
// We only output this once per file to reduce file size.
static inline void outputFilenameIdIfRequired(UInt id)
{
if (id == 0) {
Expand Down Expand Up @@ -345,6 +344,7 @@
HashUnlock(&profileState);
}

// Called whenever a function is entered
static void enterFunction(Obj func)
{
#ifdef HPCGAP
Expand All @@ -356,6 +356,7 @@
HookedLineOutput(func, 'I');
}

// Called whenever a function exits
static void leaveFunction(Obj func)
{
#ifdef HPCGAP
Expand All @@ -379,6 +380,10 @@
** If we could rely on the existence of the IO package, we would use that here.
** however, we want to be able to start compressing files right at the start
** of GAP's execution, before anything else is done.
**
** This has not switched to using GAP's internal gzip support because by
** using an external gzip we get free parallelisation, and GAP code is not
** 'charged' for the time taken to compress the profile output.
*/

static BOOL endsWithgz(const char * s)
Expand Down Expand Up @@ -532,9 +537,35 @@
}
}

// Mark line as visited, and return true if the line has been previously
// visited (executed)
static BOOL markVisited(int fileid, UInt line)
Copy link
Member

Choose a reason for hiding this comment

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

This function is not thread safe, yet is not called from a critical section either. Can't that cause problems in HPC-GAP?

So perhaps it needs a lock? Or perhaps we can make profileState.visitedStatements thread local -- it suffices to merge this into a single plist at the time we write out the profiles, no? (Though I guess that would break if a thread was killed before we do that...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(old reply!)

We only call this from one thread -- we lock profiling to a single thread (you can see this from the caller).

There could be issues to do with ownership of the datastructures here in HPC-GAP, if they are created in one thread and used in another. I'm not opposed to a fix for that, but it wouldn't shock me if various things don't work in HPC-GAP with profiling.

{
// Some STATs end up without a file or line -- do not output these
// as they would just confuse the profile generation later.
if (fileid == 0 || line == 0) {
return TRUE;
}

if (LEN_PLIST(profileState.visitedStatements) < fileid ||
!ELM_PLIST(profileState.visitedStatements, fileid)) {
AssPlist(profileState.visitedStatements, fileid,
NEW_PLIST(T_PLIST, 0));
}

Obj linelist = ELM_PLIST(profileState.visitedStatements, fileid);
Copy link
Member

Choose a reason for hiding this comment

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

I guess executing all this for every single file is quite excessive. A possible optimization: we do something like for PtrBody and PtrLVars, by tracking a STATE(ProfileLineList) which essential contains linelist from above and is updated by SWITCH_TO_OLD_LVARS if profiling is active (well and also by the code which activate/deactivates profiling). The all of the above can be replaced by something like this:

    Obj linelist = STATE(ProfileLineList);
    if (!linelist)
        return visited;

Or one could go one step further and store ADDR_OBJ(linelist) (and then update that pointer in VarsAfterCollectBags, too). Then of course LEN_PLIST, ELM_PLIST etc. below would need to be substituted.

Copy link
Member

@fingolfin fingolfin Jan 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

(I have no idea if either of these would be worthwhile, but then again, this is executed for every single expression or statement, so we should measure and make it as fast as possible?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

markedVisited is only about 2% of the time taken in profiling (that's less than I expected, but it's what cachegrind reports). Almost all the time is spent in printing.


if (LEN_PLIST(linelist) < line || !ELM_PLIST(linelist, line)) {
AssPlist(linelist, line, True);
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we can use a blist instead of a plist to reduce memory usage by a factor of 64 or so? I think the main missing ingredient would be a function which takes a blist and efficiently increases its size by adding zero blocks.

Copy link
Member

Choose a reason for hiding this comment

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

Oh and perhaps another helper which takes a blist and an index and returns TRUE or FALSE (not True or False) ... this helper then could also just return 0 for anything beyond the bounds. That would simplify the code here a bit, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As you say, I started trying to use blists then discovered there is no way to extend a blist while having it remain a blist.

It probably wouldn't be a bad idea to add such extensions to blists (maybe even accessible from the GAP level), but that was a rabbit-hole I didn't want to fall down, as it could be done later / more generally (should we just add a generic "extend list, and use this member to fill the new values, and special case that for blists?)

Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't try to make something clever or generic. Thinking again about it, I would indeed just aim for a purely kernel internal API which does exactly what we need. These could even be private inside profile.c, no need to have them in blister.h:

int SAFE_TEST_BIT_BLIST(Obj list, UInt pos)
{
    if (pos > LEN_BLIST(list)
        return 0;
    return TEST_BIT_BLIST(list, pos);
}

void SAFE_SET_BIT_BLIST(Obj list, UInt pos)
{
    if (SIZE_OBJ(list) < SIZE_PLEN_BLIST(pos))
        ResizeBag(list, SIZE_PLEN_BLIST(pos));
    SET_BIT_BLIST(list, pos);
    //CLEAR_FILTS_LIST(list); // not really necessary for our application
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll have a quick look. I worry about not testing such functions (you didn't do SET_LEN_BLIST in SAFE_SET_BIT_BLIST, which I think is required), I will take a look at doing this with BLists, however.

Copy link
Member

Choose a reason for hiding this comment

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

Oh sure. I'm not even sure that code compiles, I just copy&pasted it quickly. Yeah, it should have something like if (pos > LEN_BLIST(list)) SET_LEN_BLIST(list, pos); added

return FALSE;
}
return TRUE;
}

// type : the type of the statement
// exec : are we executing this statement
// visit: Was this statement previously visited (that is, executed)
// This deals with code which has been compiled into bytecode.
static inline void
outputStat(Int fileid, int line, int type, BOOL exec, BOOL visited)
{
Expand Down Expand Up @@ -562,6 +593,10 @@
printOutput(fileid, line, exec, visited);
}

// This deals with code which is interpreted without being turned into
// bytecode. This is only code which is outside of functions and loops.
// GAP reports the file, line, and if the code is executed or skipped (exec).
// Code is skipped when an 'if' statement is false, for example.
static inline void outputInterpretedStat(int fileid, int line, BOOL exec)
{
CheckLeaveFunctionsAfterLongjmp();
Expand All @@ -588,16 +623,14 @@
return;
#endif

BOOL visited = VISITED_STAT(stat);
int fileid = getFilenameIdOfCurrentFunction();
int line = LINE_STAT(stat);

if (!visited) {
SET_VISITED_STAT(stat);
}
BOOL visited = markVisited(fileid, line);

if (profileState.OutputRepeats || !visited) {
HashLock(&profileState);
outputStat(getFilenameIdOfCurrentFunction(), LINE_STAT(stat),
TNUM_STAT(stat), TRUE, visited);
outputStat(fileid, line, TNUM_STAT(stat), TRUE, visited);
HashUnlock(&profileState);
}
}
Expand Down Expand Up @@ -671,7 +704,6 @@

profileState.status = Profile_Active;
RegisterThrowObserver(ProfileRegisterLongJmpOccurred);
profileState.profiledPreviously = 1;
#ifdef HPCGAP
profileState.profiledThread = TLS(threadID);
#endif
Expand Down Expand Up @@ -725,16 +757,11 @@
return Fail;
}

if(profileState.profiledPreviously &&
coverage == True) {
ErrorMayQuit("Code coverage can only be started once per"
" GAP session. Please exit GAP and restart. Sorry.",0,0);
}

memset(&profileState, 0, sizeof(profileState));

OutputtedFilenameList = NEW_PLIST(T_PLIST, 0);
profileState.visitedDepths = NEW_PLIST(T_PLIST, 0);
profileState.visitedStatements = NEW_PLIST(T_PLIST, 0);

Check warning on line 764 in src/profile.c

View check run for this annotation

Codecov / codecov/patch

src/profile.c#L764 

Added line #L764 was not covered by tests

RequireStringRep(SELF_NAME, filename);

Expand Down Expand Up @@ -798,7 +825,6 @@

profileState.status = Profile_Active;
RegisterThrowObserver(ProfileRegisterLongJmpOccurred);
profileState.profiledPreviously = 1;
#ifdef HPCGAP
profileState.profiledThread = TLS(threadID);
#endif
Expand Down Expand Up @@ -875,6 +901,8 @@
InitGVarFuncsFromTable( GVarFuncs );

profileState.visitedDepths = NEW_PLIST(T_PLIST, 0);
profileState.visitedStatements = NEW_PLIST(T_PLIST, 0);

OutputtedFilenameList = NEW_PLIST(T_PLIST, 0);
return 0;
}
Expand All @@ -889,6 +917,8 @@
InitHdlrFuncsFromTable( GVarFuncs );
InitGlobalBag(&OutputtedFilenameList, "src/profile.c:OutputtedFileList");
InitGlobalBag(&profileState.visitedDepths, "src/profile.c:visitedDepths");
InitGlobalBag(&profileState.visitedStatements,
"src/profile.c:visitedStatements");
return 0;
}

Expand Down