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

Jump to bottom

Remove Certain Known Inefficiencies in Call-Ins #32 (callinperf) #41

Merged
merged 1 commit into from
Oct 12, 2017
Merged

Remove Certain Known Inefficiencies in Call-Ins #32 (callinperf) #41

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
9 changes: 6 additions & 3 deletions sr_i386/g_msf.si
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
#################################################################
# #
# Copyright 2001, 2012 Fidelity Information Services, Inc #
# Copyright (c) 2001-2012 Fidelity National Information #
# Services, Inc. and/or its subsidiaries. All rights reserved. #
# #
# Copyright (c) 2017 YottaDB LLC. and/or its subsidiaries. #
# All rights reserved. #
# #
# This source code contains the intellectual property #
# of its copyright holder(s), and is made available #
Expand Down Expand Up @@ -42,8 +46,7 @@ SFT_ZINTR = 0x100
SFF_INDCE = 0x01
SFF_ZTRAP_ERR = 0x02
SFF_DEV_ACT_ERR = 0x04
SFF_CI = 0x08
SFF_ETRAP_ERR = 0x10
SFF_ETRAP_ERR = 0x08

.sbttl g_msf.si putframe
.macro putframe
Expand Down
18 changes: 5 additions & 13 deletions sr_port/alias.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
/****************************************************************
* *
* Copyright 2009, 2014 Fidelity Information Services, Inc *
* Copyright 2009, 2014 Fidelity Information Services, Inc *
* *
* Copyright (c) 2017 YottaDB LLC. and/or its subsidiaries. *
* All rights reserved. *
* *
* This source code contains the intellectual property *
* of its copyright holder(s), and is made available *
Expand All @@ -27,17 +30,6 @@

#include "zwrite.h"

/* Macro used intermittently in code to debug alias code in general. Note this macro must be specified
* as a compile option since it is used in macros that do not pull in this alias.h header file.
*/
#ifdef DEBUG_ALIAS
# define DBGALS(x) DBGFPF(x)
# define DBGALS_ONLY(x) x
#else
# define DBGALS(x)
# define DBGALS_ONLY(x)
#endif

/* Macro used intermittently to trace reference count changes */
/* #define DEBUG_REFCNT */
#ifdef DEBUG_REFCNT
Expand Down Expand Up @@ -335,6 +327,6 @@ void als_prcs_xnewref_cntnr(lvTree *lvt);
ht_ent_mname *als_lookup_base_lvval(lv_val *lvp);
zwr_alias_var *als_getzavslot(void);
int als_lvval_gc(void);
DBGALS_ONLY(void als_lvmon_output(void);)
DBGALS_ONLY(void als_lvamon_output(void);)

#endif /* !ALIAS_H_ */
15 changes: 9 additions & 6 deletions sr_port/alias_funcs.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@
* Copyright (c) 2009, 2015 Fidelity National Information *
* Services, Inc. and/or its subsidiaries. All rights reserved. *
* *
* Copyright (c) 2017 YottaDB LLC. and/or its subsidiaries. *
* All rights reserved. *
* *
* This source code contains the intellectual property *
* of its copyright holder(s), and is made available *
* under a license. If you do not know the terms of *
Expand Down Expand Up @@ -261,7 +264,7 @@ void als_lsymtab_repair(hash_table_mname *table, ht_ent_mname *table_base_orig,
last_lsym_hte = NULL;
done = FALSE;
fp = frame_pointer;
assert(frame_pointer);
assert(fp);
do
{ /* Once through for each stackframe using the same symbol table. Note this loop is similar
* to the stack frame loop in op_clralsvars.c.
Expand Down Expand Up @@ -290,10 +293,10 @@ void als_lsymtab_repair(hash_table_mname *table, ht_ent_mname *table_base_orig,
}
}
fpprev = fp;
fp = fp->old_frame_pointer;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why was this line removed? Previously it was possible for us to break out of the for loop (if done is TRUE) after setting fp to fp->old_frame_pointer whereas now fp would hold a different value than before if that same break happens. It does not seem right. A comment is definitely needed in the commit message if there is a reason for this.

Copy link
Contributor Author

@estess estess Oct 6, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

This all has to do with the elimination of the GTM_CI frame. The frame chain used to look like this:
CI-base-frame <- GTM$CI <- M routine

Now it looks like:
CI-base-frame <- M routine

In the previous scheme the GTM$CI frame had a "flag value" SFF_CI that indicated this was a call-in frame. In the new scheme the the base frame has a "type value" with SFT_CI set that indicates this is a call-in frame. The removed statement was because once we have noted the frame with the type set for a call-in frame, we have to go back one more frame to get to its base frame. We no longer have to do that with the new version as the frame with the flag IS the base frame.

I will a version of this note to the notes for alias_funcs.c.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I still don't understand the connection between SFF_CI/SFT_CI and the break that happens in line 297 in the new code. Let us say there is NO call-in frame at all (i.e. no call-ins done in this process). If the break happens because "done" is TRUE, we would come out of the loop with a value of "fp" that is one frame newer than older code. And it is not clear to me if that is correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are two cases - a call-in frame is present or a call-in frame is NOT present. First the call-in frame is
present case. Skip to "***" to see not present case.

Previous code:

1        do
2        {
3               ... <irrelevant to this discussion stuff>
4               fpprev = fp;
5               fp = fp->old_frame_pointer;
6               if (done)
7                       break;
8               if (SFF_CI & fpprev->flags)
9               {       /* Callins needs to be able to crawl past apparent end of stack to earlier stack segments.
10                       * We should be in the base frame now. See if an earlier frame exists.
11                       * Note we don't worry about trigger base frames here because triggers *always* have a
12                       * different symbol table - previous symbol tables and stack levels are not affected.
13                       */
14                      fp = *(stack_frame **)(fp + 1); /* Backups up to "prev pointer" created by base_frame() */
15                      if ((NULL == fp) || (fp >= (stack_frame *)stackbase) || (fp < (stack_frame *)stacktop))
16                              break;  /* Pointer not within the stack -- must be earliest occurence */
17              }
18      } while(fp);
19      /* Next, check the mv_stents for the stackframes we processed. Certain mv_stents also have hash
20       * table references in them that need repair.
21       */
22      for (mv_st_ent = mv_chain;
23           mv_st_ent < (mv_stent *)(fp ? fp : fpprev);        /* Last stack frame actually processed */
24           mv_st_ent = (mv_stent *)(mv_st_ent->mv_st_next + (char *)mv_st_ent))
25      {
26              ... <rest of loop>
27      }

Recall that the M stack looks like:
[1:M-base-frame (no prev frame)] : [2:callin-base-frame] : [3:GTM$CI-frame (has SFF_CI set)] : [4:callin-routine-frame]

Operations in loop:
  1. fp -> frame 4 (line 1)
  2. fpprev -> frame 4 (line 4)
  3. fp -> frame 3 (line 5)
  4. test on line 8 is FALSE
  5. line 18 loop back up
  6. fpprev = frame 3 (line 4)
  7. fp -> frame 2 (line 5)
  8. test on line 8 is TRUE
  9. fp -> frame 1 (line 14)
  10. line 18 loop back up
  11. fpprev -> frame 1 (line 4)
  12. fp -> NULL (line 5)
  13. test on line 8 is FALSE
  14. loop exit due to fp == NULL (line 18)

At loop exit, fp -> NULL and fpprev -> frame 1 (the M base frame).

The mv_stent loop, since fp is NULL, would use fpprev (the call-in base frame) in its end-point test.



Now - new code:

1        do
2        {
3               ... <irrelevant to this discussion stuff>
4               fpprev = fp;
5               if (done)
6                       break;
7               if (SFT_CI & fp->flags)
8               {       /* Callins needs to be able to crawl past apparent end of stack to earlier stack segments.
9                        * We should be in the base frame now. See if an earlier frame exists.
10                       * Note we don't worry about trigger base frames here because triggers *always* have a
11                       * different symbol table - previous symbol tables and stack levels are not affected.
12                       */
13                      fp = *(stack_frame **)(fp + 1); /* Backups up to "prev pointer" created by base_frame() */
14                      if ((NULL == fp) || (fp >= (stack_frame *)stackbase) || (fp < (stack_frame *)stacktop))
15                              break;  /* Pointer not within the stack -- must be earliest occurence */
16              } else
17                      fp = fp->old_frame_pointer;
18      } while(fp);
19      /* Next, check the mv_stents for the stackframes we processed. Certain mv_stents also have hash
20       * table references in them that need repair.
21       */
22      for (mv_st_ent = mv_chain;
23           mv_st_ent < (mv_stent *)(fp ? fp : fpprev);        /* Last stack frame actually processed */
24           mv_st_ent = (mv_stent *)(mv_st_ent->mv_st_next + (char *)mv_st_ent))
25      {
26              ... <rest of loop>
27      }

Recall that the new stack looks like:
[1:M-base-frame (no prev frame)] : [2:callin-base-frame (has SFT_CI set)] : [3:callin-routine-frame]

Operations in loop:
  1. fp -> frame 1 (line 1)
  2. fpprev -> frame 1 (line 4)
  3. test on line 7 is FALSE
  4. fp -> frame 2 (line 17)
  5. line 18 loop back up
  6. fpprev -> frame 2 (line 4)
  7. test on line 7 is TRUE
  8. fp -> frame 1 (line 13)
  9. line 18 loop back up
  10. fpprev -> frame 1 (line 4)
  11. test on line 7 is FALSE
  12. fp -> NULL (line 17)
  13. loop exit due to fp == NULL (line 18)

At loop exit, fp -> NULL and fpprev -> frame 1 (the M base frame).

The mv_stent loop, since fp is NULL would use fpprev (the call-in base frame) in its end-point test.

******************************************************************************************************

This is if a call-in frame is NOT present in the stack.

Using the previous code above and the following M stack:

[1:M-base-frame (no prev frame)] : [2:M-frameA] : [3:M-frameB]

Operations in loop:
  1. fp -> frame 3 (line 1)
  2. fpprev -> frame 3 (line 4)
  3. fp -> frame 2 (line 5)
  4. test on line 8 is FALSE
  5. line 18 loop back up
  6. fpprev = frame 2 (line 4)
  7. fp -> frame 1 (line 5)
  8. test on line 8 is FALSE
  9. fpprev -> frame 1 (line 4)
  10. fp -> NULL (line 5)
  11. test on line 8 is FALSE
  12. loop exit due to fp == NULL (line 18)

At loop exit, fp -> NULL and fpprev -> frame 1 (the M base frame).

The mv_stent loop, since fp is NULL would use fpprev (the M base frame) in its end-point test.


Using the newer code above and the same M stack:

Operations in loop:
  1. fp -> frame 3 (line 1)
  2. fpprev -> frame 3 (line 4)
  3. test on line 7 is FALSE
  4. fp -> frame 2 (line 17)
  5. line 18 loop back up
  6. fpprev -> frame 2 (line 4)
  7. test on line 7 is FALSE
  8. fp -> frame 1 (line 17)
  9. line 18 loop back up
  10. fpprev -> frame 1 (line 4)
  11. test on line 7 is FALSE
  12. fp -> NULL
  13. lop exit due to fp == NULL (line 18)

At loop exit, fp -> NULL and fpprev -> frame 1 (the M base frame).

The mv_stent loop, since fp is NULL would use fpprev (the M base frame) in its end-point test.

******************************************************************************************************

I'm not seeing the problem you are theorizing. Can you elaborate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After our phone conversation, I went back and see what you mean about when "done" is set, we could end on a different stack frame than previously. Figuring out whether that's bad or not will take some significant time so instead I have put the loop back the way it was and made adjustments for the lack of GTM$CI frame a different way that won't impact the values at loop exit. Were that actually the source of a bug (which seems likely but no idea how to trip it at this time), it would have been hard to find..

fp = fp->old_frame_pointer; /* Bump to prev frame and check if found a call-in base frame */
if (done)
break;
if (SFF_CI & fpprev->flags)
if ((NULL != fp) && (SFT_CI & fp->type))
{ /* Callins needs to be able to crawl past apparent end of stack to earlier stack segments.
* We should be in the base frame now. See if an earlier frame exists.
* Note we don't worry about trigger base frames here because triggers *always* have a
Expand Down Expand Up @@ -1199,7 +1202,7 @@ int als_lvval_gc(void)
* when necessary. Other tests check for memory leaks so if they find one, this monitoring can be used to discover the
* source so this is not needed for test coverage.
*/
void als_lvmon_output(void)
void als_lvamon_output(void)
{
symval *lvlsymtab;
lv_blk *lvbp;
Expand All @@ -1209,9 +1212,9 @@ void als_lvmon_output(void)
for (lvlsymtab = curr_symval; lvlsymtab; lvlsymtab = lvlsymtab->last_tab)
for (lvbp = curr_symval->lv_first_block; lvbp; lvbp = lvbp->next)
for (lvp = (lv_val *)LV_BLK_GET_BASE(lvbp), lvp_top = LV_BLK_GET_FREE(lvbp, lvp); lvp < lvp_top; lvp++)
if (lvp->lvmon_mark)
if (lvp->lvamon_mark)
{ /* lv_val slot not used as an sbs and is marked. Report it */
FPRINTF(stderr, "als_lvmon_output: lv_val at 0x"lvaddr" is still marked\n", lvp);
FPRINTF(stderr, "als_lvamon_output: lv_val at 0x"lvaddr" is still marked\n", lvp);
}
FFLUSH(stderr);
}
Expand Down
13 changes: 9 additions & 4 deletions sr_port/dollar_zlevel.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
/****************************************************************
* *
* Copyright 2001, 2011 Fidelity Information Services, Inc *
* Copyright (c) 2001-2011 Fidelity National Information *
* Services, Inc. and/or its subsidiaries. All rights reserved. *
* *
* Copyright (c) 2017 YottaDB LLC. and/or its subsidiaries. *
* All rights reserved. *
* *
* This source code contains the intellectual property *
* of its copyright holder(s), and is made available *
Expand All @@ -10,7 +14,8 @@
****************************************************************/

#include "mdef.h"
#include <rtnhdr.h>

#include "rtnhdr.h"
#include "stack_frame.h"
#include "dollar_zlevel.h"

Expand All @@ -28,15 +33,15 @@ int dollar_zlevel()
if (!(fp->type & SFT_COUNT))
continue;
if (NULL == fpprev)
{ /* Next frame is some sort of base frame */
{ /* If previous frame pointer is null, the current frame is some sort of base frame */
# ifdef GTM_TRIGGER
if (fp->type & SFT_TRIGR)
{ /* Have a trigger baseframe, pick up stack continuation frame_pointer stored by base_frame() */
fpprev = *(stack_frame **)(fp + 1);
continue;
} else
# endif
break; /* Some other base frame that stops us */
break; /* Some other base frame that stops us (M stack or call-in base frame) */
}
count++;
}
Expand Down
5 changes: 4 additions & 1 deletion sr_port/f_text.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@
* Copyright (c) 2001-2015 Fidelity National Information *
* Services, Inc. and/or its subsidiaries. All rights reserved. *
* *
* Copyright (c) 2017 YottaDB LLC. and/or its subsidiaries. *
* All rights reserved. *
* *
* This source code contains the intellectual property *
* of its copyright holder(s), and is made available *
* under a license. If you do not know the terms of *
Expand All @@ -27,7 +30,7 @@ GBLREF boolean_t run_time;
GBLREF command_qualifier cmd_qlf;
GBLREF mident routine_name;

STATICDEF char *suppressed_values[] = {"GTM$DMOD", "GTM$CI"};
STATICDEF char *suppressed_values[] = {"GTM$DMOD"};

error_def(ERR_RPARENMISSING);
error_def(ERR_RTNNAME);
Expand Down
7 changes: 5 additions & 2 deletions sr_port/fgncal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
/****************************************************************
* *
* Copyright 2001, 2013 Fidelity Information Services, Inc *
* Copyright (c) 2001-2013 Fidelity National Information *
* Services, Inc. and/or its subsidiaries. All rights reserved. *
* *
* Copyright (c) 2017 YottaDB LLC. and/or its subsidiaries. *
* All rights reserved. *
* *
* This source code contains the intellectual property *
* of its copyright holder(s), and is made available *
Expand All @@ -12,7 +16,6 @@
#ifndef __FGNCAL_H__
#define __FGNCAL_H__

mval *fgncal_lookup(mval *x);
void fgncal_unwind(void);
void fgncal_rundown(void);

Expand Down
45 changes: 0 additions & 45 deletions sr_port/fgncal_lookup.c
View file
Open in desktop

This file was deleted.

7 changes: 4 additions & 3 deletions sr_port/gbldefs.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@
* Copyright (c) 2001-2017 Fidelity National Information *
* Services, Inc. and/or its subsidiaries. All rights reserved. *
* *
* Copyright (c) 2017 YottaDB LLC. and/or its subsidiaries. *
* All rights reserved. *
* *
* This source code contains the intellectual property *
* of its copyright holder(s), and is made available *
* under a license. If you do not know the terms of *
Expand Down Expand Up @@ -392,8 +395,6 @@ GBLDEF fd_set mutex_wait_on_descs;
#endif
GBLDEF void (*call_on_signal)();
GBLDEF enum gtmImageTypes image_type; /* initialized at startup i.e. in dse.c, lke.c, gtm.c, mupip.c, gtmsecshr.c etc. */

GBLDEF parmblk_struct *param_list; /* call-in parameters block (defined in unix/fgncalsp.h)*/
GBLDEF unsigned int invocation_mode = MUMPS_COMPILE; /* how mumps has been invoked */
GBLDEF char cli_err_str[MAX_CLI_ERR_STR] = ""; /* Parse Error message buffer */
GBLDEF char *cli_err_str_ptr;
Expand Down Expand Up @@ -890,7 +891,7 @@ GBLDEF mval *alias_retarg; /* Points to an alias return arg created by a "QUI
* that is going to be destroyed.
*/
#ifdef DEBUG_ALIAS
GBLDEF boolean_t lvmon_enabled; /* Enable lv_val monitoring */
GBLDEF boolean_t lvamon_enabled; /* Enable lv_val/alias monitoring */
#endif
GBLDEF block_id gtm_tp_allocation_clue; /* block# hint to start allocation for created blocks in TP */
GBLDEF int4 gtm_zlib_cmp_level; /* zlib compression level specified at process startup */
Expand Down
8 changes: 4 additions & 4 deletions sr_port/lv_val.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@
(lv_ptr)->ptrs.free_ent.next_free = *savflist_ptr; \
*savflist_ptr = (lv_ptr); \
LV_SYMVAL(lv_ptr) = NULL; \
DBGALS_ONLY((lv_ptr)->lvmon_mark = FALSE); \
DBGALS_ONLY((lv_ptr)->lvamon_mark = FALSE); \
}

/* Increment the cycle for tstarts. Field is compared to same name field in lv_val to signify an lv_val has been seen
Expand Down Expand Up @@ -192,7 +192,7 @@
/* Initialize given lv_val (should be of type "lv_val *" and not "lvTreeNode *") */
#define LVVAL_INIT(lv, symvalarg) \
{ \
DBGALS_ONLY(GBLREF boolean_t lvmon_enabled;) \
DBGALS_ONLY(GBLREF boolean_t lvamon_enabled;) \
DBGALS_ONLY(GBLREF stack_frame *frame_pointer;) \
assert(MV_SYM == symvalarg->ident); /* ensure above macro is never used to initialize a "lvTreeNode *" */ \
(lv)->v.mvtype = 0; \
Expand All @@ -201,7 +201,7 @@
(lv)->stats.tstartcycle = 0; \
(lv)->stats.lvtaskcycle = 0; \
(lv)->has_aliascont = FALSE; \
DBGALS_ONLY(if (lvmon_enabled) (lv)->lvmon_mark = TRUE; else (lv)->lvmon_mark = FALSE); \
DBGALS_ONLY(if (lvamon_enabled) (lv)->lvamon_mark = TRUE; else (lv)->lvamon_mark = FALSE); \
(lv)->tp_var = NULL; \
LV_CHILD(lv) = NULL; \
LV_SYMVAL(lv) = symvalarg; \
Expand Down Expand Up @@ -294,7 +294,7 @@ typedef struct lv_val_struct
# endif
} stats;
boolean_t has_aliascont; /* This base var has or had an alias container in it */
boolean_t lvmon_mark; /* This lv_val is being monitored; Used only #ifdef DEBUG_ALIAS */
boolean_t lvamon_mark; /* This lv_val is being monitored; Used only #ifdef DEBUG_ALIAS */
struct tp_var_struct *tp_var;
} lv_val;

Expand Down
Loading