From 70d4fd78eeabcf523a0bbe987533d8bf0ccba0ba Mon Sep 17 00:00:00 2001 From: Chris Jefferson Date: Thu, 14 Dec 2023 12:54:11 +0800 Subject: [PATCH 1/6] Stop using VISITED_STAT and SET_VISITED_STAT in profiling --- src/profile.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 63 insertions(+), 6 deletions(-) diff --git a/src/profile.c b/src/profile.c index e4283cc2aa..f2e4f8d6ff 100644 --- a/src/profile.c +++ b/src/profile.c @@ -163,6 +163,10 @@ static struct ProfileState // 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 @@ -532,6 +536,56 @@ static inline void printOutput(int fileid, int line, BOOL exec, BOOL visited) } } +// Mark line as visited, and return true if the line has been previously +// visited (executed) +BOOL markVisited(int fileid, UInt line) +{ + // 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); + + if (LEN_PLIST(linelist) < line || !ELM_PLIST(linelist, line)) { + AssPlist(linelist, line, True); + return FALSE; + } + return TRUE; +} + +// Return TRUE is Stat has been visited (executed) before +BOOL visitedStat(Stat stat) +{ + int fileid = getFilenameIdOfCurrentFunction(); + int line = LINE_STAT(stat); + + if (fileid == 0 || line == 0) { + return TRUE; + } + + if (LEN_PLIST(profileState.visitedStatements) < fileid || + !ELM_PLIST(profileState.visitedStatements, fileid)) { + return FALSE; + } + + Obj linelist = ELM_PLIST(profileState.visitedStatements, fileid); + + if (LEN_PLIST(linelist) < line || !ELM_PLIST(linelist, line)) { + return 0; + } + else { + return 1; + } +} + // type : the type of the statement // exec : are we executing this statement // visit: Was this statement previously visited (that is, executed) @@ -588,16 +642,14 @@ static void visitStat(Stat stat) 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); } } @@ -735,6 +787,7 @@ static Obj FuncACTIVATE_PROFILING(Obj self, OutputtedFilenameList = NEW_PLIST(T_PLIST, 0); profileState.visitedDepths = NEW_PLIST(T_PLIST, 0); + profileState.visitedStatements = NEW_PLIST(T_PLIST, 0); RequireStringRep(SELF_NAME, filename); @@ -875,6 +928,8 @@ static Int InitLibrary ( InitGVarFuncsFromTable( GVarFuncs ); profileState.visitedDepths = NEW_PLIST(T_PLIST, 0); + profileState.visitedStatements = NEW_PLIST(T_PLIST, 0); + OutputtedFilenameList = NEW_PLIST(T_PLIST, 0); return 0; } @@ -889,6 +944,8 @@ static Int InitKernel ( InitHdlrFuncsFromTable( GVarFuncs ); InitGlobalBag(&OutputtedFilenameList, "src/profile.c:OutputtedFileList"); InitGlobalBag(&profileState.visitedDepths, "src/profile.c:visitedDepths"); + InitGlobalBag(&profileState.visitedStatements, + "src/profile.c:visitedStatements"); return 0; } From fea3f62212b1abd3706a7075f9844a6d60143fb4 Mon Sep 17 00:00:00 2001 From: Chris Jefferson Date: Thu, 14 Dec 2023 12:54:11 +0800 Subject: [PATCH 2/6] Remove profiledPreviously, as no need to track this any more --- src/profile.c | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/src/profile.c b/src/profile.c index f2e4f8d6ff..afb4855422 100644 --- a/src/profile.c +++ b/src/profile.c @@ -149,12 +149,6 @@ static struct ProfileState 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. @@ -723,7 +717,6 @@ enableAtStartup(char * filename, Int repeats, TickMethod tickMethod) profileState.status = Profile_Active; RegisterThrowObserver(ProfileRegisterLongJmpOccurred); - profileState.profiledPreviously = 1; #ifdef HPCGAP profileState.profiledThread = TLS(threadID); #endif @@ -777,12 +770,6 @@ static Obj FuncACTIVATE_PROFILING(Obj self, 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); @@ -851,7 +838,6 @@ static Obj FuncACTIVATE_PROFILING(Obj self, profileState.status = Profile_Active; RegisterThrowObserver(ProfileRegisterLongJmpOccurred); - profileState.profiledPreviously = 1; #ifdef HPCGAP profileState.profiledThread = TLS(threadID); #endif From 682abd36618d03b8e1d94805cda1f2a0d6b38262 Mon Sep 17 00:00:00 2001 From: Chris Jefferson Date: Thu, 14 Dec 2023 12:54:11 +0800 Subject: [PATCH 3/6] Remove VISITED_STAT and SET_VISITED_STAT --- src/code.c | 8 -------- src/code.h | 28 ---------------------------- 2 files changed, 36 deletions(-) diff --git a/src/code.c b/src/code.c index 10c9910106..1bd3989599 100644 --- a/src/code.c +++ b/src/code.c @@ -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)) diff --git a/src/code.h b/src/code.h index a66c7f40a6..67b7902c1c 100644 --- a/src/code.h +++ b/src/code.h @@ -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; @@ -375,30 +371,6 @@ EXPORT_INLINE Int LINE_STAT(Stat stat) return CONST_STAT_HEADER(stat)->line; } - -/**************************************************************************** -** -*F VISITED_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() . . . . . . . . . . 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() . . . test if an expression is a reference to a local From 0ffedbebb6fabf4a0c36f71ddab6112f9a48ed80 Mon Sep 17 00:00:00 2001 From: Chris Jefferson Date: Thu, 14 Dec 2023 12:54:11 +0800 Subject: [PATCH 4/6] Increment minor kernel version to denote changes to profile generation --- configure.ac | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/configure.ac b/configure.ac index c71c35dec4..e74b0f2827 100644 --- a/configure.ac +++ b/configure.ac @@ -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"]) From 971d2c00846fb4c1947fa1d3bf023de2e7419d1c Mon Sep 17 00:00:00 2001 From: Max Horn Date: Thu, 14 Dec 2023 12:54:11 +0800 Subject: [PATCH 5/6] remove unused function, declare markVisited as static --- src/profile.c | 27 +-------------------------- 1 file changed, 1 insertion(+), 26 deletions(-) diff --git a/src/profile.c b/src/profile.c index afb4855422..91fbaf3a3f 100644 --- a/src/profile.c +++ b/src/profile.c @@ -532,7 +532,7 @@ static inline void printOutput(int fileid, int line, BOOL exec, BOOL visited) // Mark line as visited, and return true if the line has been previously // visited (executed) -BOOL markVisited(int fileid, UInt line) +static BOOL markVisited(int fileid, UInt line) { // Some STATs end up without a file or line -- do not output these // as they would just confuse the profile generation later. @@ -555,31 +555,6 @@ BOOL markVisited(int fileid, UInt line) return TRUE; } -// Return TRUE is Stat has been visited (executed) before -BOOL visitedStat(Stat stat) -{ - int fileid = getFilenameIdOfCurrentFunction(); - int line = LINE_STAT(stat); - - if (fileid == 0 || line == 0) { - return TRUE; - } - - if (LEN_PLIST(profileState.visitedStatements) < fileid || - !ELM_PLIST(profileState.visitedStatements, fileid)) { - return FALSE; - } - - Obj linelist = ELM_PLIST(profileState.visitedStatements, fileid); - - if (LEN_PLIST(linelist) < line || !ELM_PLIST(linelist, line)) { - return 0; - } - else { - return 1; - } -} - // type : the type of the statement // exec : are we executing this statement // visit: Was this statement previously visited (that is, executed) From aaeae1a6449ff2fe2595914e770e8d52a7d96b92 Mon Sep 17 00:00:00 2001 From: Chris Jefferson Date: Tue, 23 Jan 2024 18:37:05 +0800 Subject: [PATCH 6/6] Add some comments to profiling and interpreter hooking --- src/hookintrprtr.h | 4 ++++ src/profile.c | 14 +++++++++++++- 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/src/hookintrprtr.h b/src/hookintrprtr.h index 3f583a0de1..477d2a4ff5 100644 --- a/src/hookintrprtr.h +++ b/src/hookintrprtr.h @@ -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. ** diff --git a/src/profile.c b/src/profile.c index 91fbaf3a3f..30e1e1524d 100644 --- a/src/profile.c +++ b/src/profile.c @@ -275,7 +275,8 @@ static Obj JsonEscapeString(Obj param) 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) { @@ -343,6 +344,7 @@ static void HookedLineOutput(Obj func, char type) HashUnlock(&profileState); } +// Called whenever a function is entered static void enterFunction(Obj func) { #ifdef HPCGAP @@ -354,6 +356,7 @@ static void enterFunction(Obj func) HookedLineOutput(func, 'I'); } +// Called whenever a function exits static void leaveFunction(Obj func) { #ifdef HPCGAP @@ -377,6 +380,10 @@ static void leaveFunction(Obj func) ** 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) @@ -558,6 +565,7 @@ static BOOL markVisited(int fileid, UInt line) // 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) { @@ -585,6 +593,10 @@ outputStat(Int fileid, int line, int type, BOOL exec, BOOL visited) 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();