[PATCH] perf jvmti: remove redundant jitdump line table entries

[PATCH] perf jvmti: remove redundant jitdump line table entries

[Nick Gasson]

For each PC/BCI pair in the JVMTI compiler inlining record table, the
jitdump plugin emits debug line table entries for every source line in
the method preceding that BCI. Instead only emit one source line per
PC/BCI pair. Reported by Ian Rogers. This reduces the .dump size for
SPECjbb from ~230MB to ~40MB.

Also fix an error in the DWARF line table state machine where addresses
are incorrectly offset by -0x40 (GEN_ELF_TEXT_OFFSET). This can be seen
with `objdump -S` on the ELF files after perf inject.

Signed-off-by: Nick Gasson <nick.gasson@arm.com>
---
 tools/perf/jvmti/libjvmti.c    | 73 +++++++++++++---------------------
 tools/perf/util/genelf_debug.c |  4 +-
 2 files changed, 30 insertions(+), 47 deletions(-)

diff --git a/tools/perf/jvmti/libjvmti.c b/tools/perf/jvmti/libjvmti.c
index a9a056d68416..398e4ba6498d 100644
--- a/tools/perf/jvmti/libjvmti.c
+++ b/tools/perf/jvmti/libjvmti.c
@@ -32,38 +32,41 @@ static void print_error(jvmtiEnv *jvmti, const char *msg, jvmtiError ret)
 
 #ifdef HAVE_JVMTI_CMLR
 static jvmtiError
-do_get_line_numbers(jvmtiEnv *jvmti, void *pc, jmethodID m, jint bci,
-		    jvmti_line_info_t *tab, jint *nr)
+do_get_line_number(jvmtiEnv *jvmti, void *pc, jmethodID m, jint bci,
+		   jvmti_line_info_t *tab)
 {
-	jint i, lines = 0;
-	jint nr_lines = 0;
+	jint i, nr_lines = 0;
 	jvmtiLineNumberEntry *loc_tab = NULL;
 	jvmtiError ret;
+	jint src_line = -1;
 
 	ret = (*jvmti)->GetLineNumberTable(jvmti, m, &nr_lines, &loc_tab);
 	if (ret == JVMTI_ERROR_ABSENT_INFORMATION || ret == JVMTI_ERROR_NATIVE_METHOD) {
 		/* No debug information for this method */
-		*nr = 0;
-		return JVMTI_ERROR_NONE;
+		return ret;
 	} else if (ret != JVMTI_ERROR_NONE) {
 		print_error(jvmti, "GetLineNumberTable", ret);
 		return ret;
 	}
 
-	for (i = 0; i < nr_lines; i++) {
-		if (loc_tab[i].start_location < bci) {
-			tab[lines].pc = (unsigned long)pc;
-			tab[lines].line_number = loc_tab[i].line_number;
-			tab[lines].discrim = 0; /* not yet used */
-			tab[lines].methodID = m;
-			lines++;
-		} else {
-			break;
-		}
+	for (i = 0; i < nr_lines && loc_tab[i].start_location <= bci; i++) {
+		src_line = i;
+	}
+
+	if (src_line != -1) {
+		tab->pc = (unsigned long)pc;
+		tab->line_number = loc_tab[src_line].line_number;
+		tab->discrim = 0; /* not yet used */
+		tab->methodID = m;
+
+		ret = JVMTI_ERROR_NONE;
+	} else {
+		ret = JVMTI_ERROR_ABSENT_INFORMATION;
 	}
+
 	(*jvmti)->Deallocate(jvmti, (unsigned char *)loc_tab);
-	*nr = lines;
-	return JVMTI_ERROR_NONE;
+
+	return ret;
 }
 
 static jvmtiError
@@ -71,9 +74,8 @@ get_line_numbers(jvmtiEnv *jvmti, const void *compile_info, jvmti_line_info_t **
 {
 	const jvmtiCompiledMethodLoadRecordHeader *hdr;
 	jvmtiCompiledMethodLoadInlineRecord *rec;
-	jvmtiLineNumberEntry *lne = NULL;
 	PCStackInfo *c;
-	jint nr, ret;
+	jint ret;
 	int nr_total = 0;
 	int i, lines_total = 0;
 
@@ -86,24 +88,7 @@ get_line_numbers(jvmtiEnv *jvmti, const void *compile_info, jvmti_line_info_t **
 	for (hdr = compile_info; hdr != NULL; hdr = hdr->next) {
 		if (hdr->kind == JVMTI_CMLR_INLINE_INFO) {
 			rec = (jvmtiCompiledMethodLoadInlineRecord *)hdr;
-			for (i = 0; i < rec->numpcs; i++) {
-				c = rec->pcinfo + i;
-				nr = 0;
-				/*
-				 * unfortunately, need a tab to get the number of lines!
-				 */
-				ret = (*jvmti)->GetLineNumberTable(jvmti, c->methods[0], &nr, &lne);
-				if (ret == JVMTI_ERROR_NONE) {
-					/* free what was allocated for nothing */
-					(*jvmti)->Deallocate(jvmti, (unsigned char *)lne);
-					nr_total += (int)nr;
-				} else if (ret == JVMTI_ERROR_ABSENT_INFORMATION
-					   || ret == JVMTI_ERROR_NATIVE_METHOD) {
-					/* No debug information for this method */
-				} else {
-					print_error(jvmti, "GetLineNumberTable", ret);
-				}
-			}
+			nr_total += rec->numpcs;
 		}
 	}
 
@@ -122,14 +107,12 @@ get_line_numbers(jvmtiEnv *jvmti, const void *compile_info, jvmti_line_info_t **
 			rec = (jvmtiCompiledMethodLoadInlineRecord *)hdr;
 			for (i = 0; i < rec->numpcs; i++) {
 				c = rec->pcinfo + i;
-				nr = 0;
-				ret = do_get_line_numbers(jvmti, c->pc,
-							  c->methods[0],
-							  c->bcis[0],
-							  *tab + lines_total,
-							  &nr);
+				ret = do_get_line_number(jvmti, c->pc,
+							 c->methods[0],
+							 c->bcis[0],
+							 *tab + lines_total);
 				if (ret == JVMTI_ERROR_NONE)
-					lines_total += nr;
+					lines_total++;
 			}
 		}
 	}
diff --git a/tools/perf/util/genelf_debug.c b/tools/perf/util/genelf_debug.c
index 30e9f618f6cd..dd40683bd4c0 100644
--- a/tools/perf/util/genelf_debug.c
+++ b/tools/perf/util/genelf_debug.c
@@ -342,7 +342,7 @@ static void emit_lineno_info(struct buffer_ext *be,
 	 */
 
 	/* start state of the state machine we take care of */
-	unsigned long last_vma = code_addr;
+	unsigned long last_vma = 0;
 	char const  *cur_filename = NULL;
 	unsigned long cur_file_idx = 0;
 	int last_line = 1;
@@ -473,7 +473,7 @@ jit_process_debug_info(uint64_t code_addr,
 		ent = debug_entry_next(ent);
 	}
 	add_compilation_unit(di, buffer_ext_size(dl));
-	add_debug_line(dl, debug, nr_debug_entries, 0);
+	add_debug_line(dl, debug, nr_debug_entries, GEN_ELF_TEXT_OFFSET);
 	add_debug_abbrev(da);
 	if (0) buffer_ext_dump(da, "abbrev");
 
-- 
2.26.2

Re: [PATCH] perf jvmti: remove redundant jitdump line table entries

[Jiri Olsa]

On Fri, May 22, 2020 at 02:53:30PM +0800, Nick Gasson wrote:
> For each PC/BCI pair in the JVMTI compiler inlining record table, the
> jitdump plugin emits debug line table entries for every source line in
> the method preceding that BCI. Instead only emit one source line per
> PC/BCI pair. Reported by Ian Rogers. This reduces the .dump size for
> SPECjbb from ~230MB to ~40MB.
> 
> Also fix an error in the DWARF line table state machine where addresses
> are incorrectly offset by -0x40 (GEN_ELF_TEXT_OFFSET). This can be seen
> with `objdump -S` on the ELF files after perf inject.

hi,
I can't apply this on latest Arnaldo's perf/core:

patching file jvmti/libjvmti.c
Hunk #1 FAILED at 32.
Hunk #2 succeeded at 67 (offset -4 lines).
Hunk #3 FAILED at 85.
Hunk #4 succeeded at 114 (offset -7 lines).

jirka

> 
> Signed-off-by: Nick Gasson <nick.gasson@arm.com>
> ---
>  tools/perf/jvmti/libjvmti.c    | 73 +++++++++++++---------------------
>  tools/perf/util/genelf_debug.c |  4 +-
>  2 files changed, 30 insertions(+), 47 deletions(-)
> 
> diff --git a/tools/perf/jvmti/libjvmti.c b/tools/perf/jvmti/libjvmti.c
> index a9a056d68416..398e4ba6498d 100644
> --- a/tools/perf/jvmti/libjvmti.c
> +++ b/tools/perf/jvmti/libjvmti.c
> @@ -32,38 +32,41 @@ static void print_error(jvmtiEnv *jvmti, const char *msg, jvmtiError ret)
>  
>  #ifdef HAVE_JVMTI_CMLR
>  static jvmtiError
> -do_get_line_numbers(jvmtiEnv *jvmti, void *pc, jmethodID m, jint bci,
> -		    jvmti_line_info_t *tab, jint *nr)
> +do_get_line_number(jvmtiEnv *jvmti, void *pc, jmethodID m, jint bci,
> +		   jvmti_line_info_t *tab)
>  {
> -	jint i, lines = 0;
> -	jint nr_lines = 0;
> +	jint i, nr_lines = 0;
>  	jvmtiLineNumberEntry *loc_tab = NULL;
>  	jvmtiError ret;
> +	jint src_line = -1;
>  
>  	ret = (*jvmti)->GetLineNumberTable(jvmti, m, &nr_lines, &loc_tab);
>  	if (ret == JVMTI_ERROR_ABSENT_INFORMATION || ret == JVMTI_ERROR_NATIVE_METHOD) {
>  		/* No debug information for this method */
> -		*nr = 0;
> -		return JVMTI_ERROR_NONE;
> +		return ret;
>  	} else if (ret != JVMTI_ERROR_NONE) {
>  		print_error(jvmti, "GetLineNumberTable", ret);
>  		return ret;
>  	}
>  
> -	for (i = 0; i < nr_lines; i++) {
> -		if (loc_tab[i].start_location < bci) {
> -			tab[lines].pc = (unsigned long)pc;
> -			tab[lines].line_number = loc_tab[i].line_number;
> -			tab[lines].discrim = 0; /* not yet used */
> -			tab[lines].methodID = m;
> -			lines++;
> -		} else {
> -			break;
> -		}
> +	for (i = 0; i < nr_lines && loc_tab[i].start_location <= bci; i++) {
> +		src_line = i;
> +	}
> +
> +	if (src_line != -1) {
> +		tab->pc = (unsigned long)pc;
> +		tab->line_number = loc_tab[src_line].line_number;
> +		tab->discrim = 0; /* not yet used */
> +		tab->methodID = m;
> +
> +		ret = JVMTI_ERROR_NONE;
> +	} else {
> +		ret = JVMTI_ERROR_ABSENT_INFORMATION;
>  	}
> +
>  	(*jvmti)->Deallocate(jvmti, (unsigned char *)loc_tab);
> -	*nr = lines;
> -	return JVMTI_ERROR_NONE;
> +
> +	return ret;
>  }
>  
>  static jvmtiError
> @@ -71,9 +74,8 @@ get_line_numbers(jvmtiEnv *jvmti, const void *compile_info, jvmti_line_info_t **
>  {
>  	const jvmtiCompiledMethodLoadRecordHeader *hdr;
>  	jvmtiCompiledMethodLoadInlineRecord *rec;
> -	jvmtiLineNumberEntry *lne = NULL;
>  	PCStackInfo *c;
> -	jint nr, ret;
> +	jint ret;
>  	int nr_total = 0;
>  	int i, lines_total = 0;
>  
> @@ -86,24 +88,7 @@ get_line_numbers(jvmtiEnv *jvmti, const void *compile_info, jvmti_line_info_t **
>  	for (hdr = compile_info; hdr != NULL; hdr = hdr->next) {
>  		if (hdr->kind == JVMTI_CMLR_INLINE_INFO) {
>  			rec = (jvmtiCompiledMethodLoadInlineRecord *)hdr;
> -			for (i = 0; i < rec->numpcs; i++) {
> -				c = rec->pcinfo + i;
> -				nr = 0;
> -				/*
> -				 * unfortunately, need a tab to get the number of lines!
> -				 */
> -				ret = (*jvmti)->GetLineNumberTable(jvmti, c->methods[0], &nr, &lne);
> -				if (ret == JVMTI_ERROR_NONE) {
> -					/* free what was allocated for nothing */
> -					(*jvmti)->Deallocate(jvmti, (unsigned char *)lne);
> -					nr_total += (int)nr;
> -				} else if (ret == JVMTI_ERROR_ABSENT_INFORMATION
> -					   || ret == JVMTI_ERROR_NATIVE_METHOD) {
> -					/* No debug information for this method */
> -				} else {
> -					print_error(jvmti, "GetLineNumberTable", ret);
> -				}
> -			}
> +			nr_total += rec->numpcs;
>  		}
>  	}
>  
> @@ -122,14 +107,12 @@ get_line_numbers(jvmtiEnv *jvmti, const void *compile_info, jvmti_line_info_t **
>  			rec = (jvmtiCompiledMethodLoadInlineRecord *)hdr;
>  			for (i = 0; i < rec->numpcs; i++) {
>  				c = rec->pcinfo + i;
> -				nr = 0;
> -				ret = do_get_line_numbers(jvmti, c->pc,
> -							  c->methods[0],
> -							  c->bcis[0],
> -							  *tab + lines_total,
> -							  &nr);
> +				ret = do_get_line_number(jvmti, c->pc,
> +							 c->methods[0],
> +							 c->bcis[0],
> +							 *tab + lines_total);
>  				if (ret == JVMTI_ERROR_NONE)
> -					lines_total += nr;
> +					lines_total++;
>  			}
>  		}
>  	}
> diff --git a/tools/perf/util/genelf_debug.c b/tools/perf/util/genelf_debug.c
> index 30e9f618f6cd..dd40683bd4c0 100644
> --- a/tools/perf/util/genelf_debug.c
> +++ b/tools/perf/util/genelf_debug.c
> @@ -342,7 +342,7 @@ static void emit_lineno_info(struct buffer_ext *be,
>  	 */
>  
>  	/* start state of the state machine we take care of */
> -	unsigned long last_vma = code_addr;
> +	unsigned long last_vma = 0;
>  	char const  *cur_filename = NULL;
>  	unsigned long cur_file_idx = 0;
>  	int last_line = 1;
> @@ -473,7 +473,7 @@ jit_process_debug_info(uint64_t code_addr,
>  		ent = debug_entry_next(ent);
>  	}
>  	add_compilation_unit(di, buffer_ext_size(dl));
> -	add_debug_line(dl, debug, nr_debug_entries, 0);
> +	add_debug_line(dl, debug, nr_debug_entries, GEN_ELF_TEXT_OFFSET);
>  	add_debug_abbrev(da);
>  	if (0) buffer_ext_dump(da, "abbrev");
>  
> -- 
> 2.26.2
> 

Re: [PATCH] perf jvmti: remove redundant jitdump line table entries

[Ian Rogers]

On Thu, May 21, 2020 at 11:54 PM Nick Gasson <nick.gasson@arm.com> wrote:
>
> For each PC/BCI pair in the JVMTI compiler inlining record table, the
> jitdump plugin emits debug line table entries for every source line in
> the method preceding that BCI. Instead only emit one source line per
> PC/BCI pair. Reported by Ian Rogers. This reduces the .dump size for
> SPECjbb from ~230MB to ~40MB.

Great result, thanks! I note there is a lack of symbolization when
benchmarking a few Java applications. I'll try to see if there's a
sensible resolution for those.

> Also fix an error in the DWARF line table state machine where addresses
> are incorrectly offset by -0x40 (GEN_ELF_TEXT_OFFSET). This can be seen
> with `objdump -S` on the ELF files after perf inject.

It'd be better to make this into two patches. Also on acme's perf/core
branch if possible:
https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/log/?h=perf/core

> Signed-off-by: Nick Gasson <nick.gasson@arm.com>
> ---
>  tools/perf/jvmti/libjvmti.c    | 73 +++++++++++++---------------------
>  tools/perf/util/genelf_debug.c |  4 +-
>  2 files changed, 30 insertions(+), 47 deletions(-)
>
> diff --git a/tools/perf/jvmti/libjvmti.c b/tools/perf/jvmti/libjvmti.c
> index a9a056d68416..398e4ba6498d 100644
> --- a/tools/perf/jvmti/libjvmti.c
> +++ b/tools/perf/jvmti/libjvmti.c
> @@ -32,38 +32,41 @@ static void print_error(jvmtiEnv *jvmti, const char *msg, jvmtiError ret)
>
>  #ifdef HAVE_JVMTI_CMLR
>  static jvmtiError
> -do_get_line_numbers(jvmtiEnv *jvmti, void *pc, jmethodID m, jint bci,
> -                   jvmti_line_info_t *tab, jint *nr)
> +do_get_line_number(jvmtiEnv *jvmti, void *pc, jmethodID m, jint bci,
> +                  jvmti_line_info_t *tab)
>  {
> -       jint i, lines = 0;
> -       jint nr_lines = 0;
> +       jint i, nr_lines = 0;
>         jvmtiLineNumberEntry *loc_tab = NULL;
>         jvmtiError ret;
> +       jint src_line = -1;
>
>         ret = (*jvmti)->GetLineNumberTable(jvmti, m, &nr_lines, &loc_tab);
>         if (ret == JVMTI_ERROR_ABSENT_INFORMATION || ret == JVMTI_ERROR_NATIVE_METHOD) {
>                 /* No debug information for this method */
> -               *nr = 0;
> -               return JVMTI_ERROR_NONE;
> +               return ret;
>         } else if (ret != JVMTI_ERROR_NONE) {
>                 print_error(jvmti, "GetLineNumberTable", ret);
>                 return ret;
>         }
>
> -       for (i = 0; i < nr_lines; i++) {
> -               if (loc_tab[i].start_location < bci) {
> -                       tab[lines].pc = (unsigned long)pc;
> -                       tab[lines].line_number = loc_tab[i].line_number;
> -                       tab[lines].discrim = 0; /* not yet used */
> -                       tab[lines].methodID = m;
> -                       lines++;
> -               } else {
> -                       break;
> -               }
> +       for (i = 0; i < nr_lines && loc_tab[i].start_location <= bci; i++) {
> +               src_line = i;
> +       }
> +
> +       if (src_line != -1) {
> +               tab->pc = (unsigned long)pc;
> +               tab->line_number = loc_tab[src_line].line_number;
> +               tab->discrim = 0; /* not yet used */
> +               tab->methodID = m;
> +
> +               ret = JVMTI_ERROR_NONE;
> +       } else {
> +               ret = JVMTI_ERROR_ABSENT_INFORMATION;
>         }
> +
>         (*jvmti)->Deallocate(jvmti, (unsigned char *)loc_tab);
> -       *nr = lines;
> -       return JVMTI_ERROR_NONE;
> +
> +       return ret;
>  }
>
>  static jvmtiError
> @@ -71,9 +74,8 @@ get_line_numbers(jvmtiEnv *jvmti, const void *compile_info, jvmti_line_info_t **
>  {
>         const jvmtiCompiledMethodLoadRecordHeader *hdr;
>         jvmtiCompiledMethodLoadInlineRecord *rec;
> -       jvmtiLineNumberEntry *lne = NULL;
>         PCStackInfo *c;
> -       jint nr, ret;
> +       jint ret;
>         int nr_total = 0;
>         int i, lines_total = 0;
>
> @@ -86,24 +88,7 @@ get_line_numbers(jvmtiEnv *jvmti, const void *compile_info, jvmti_line_info_t **
>         for (hdr = compile_info; hdr != NULL; hdr = hdr->next) {
>                 if (hdr->kind == JVMTI_CMLR_INLINE_INFO) {
>                         rec = (jvmtiCompiledMethodLoadInlineRecord *)hdr;
> -                       for (i = 0; i < rec->numpcs; i++) {
> -                               c = rec->pcinfo + i;
> -                               nr = 0;
> -                               /*
> -                                * unfortunately, need a tab to get the number of lines!
> -                                */
> -                               ret = (*jvmti)->GetLineNumberTable(jvmti, c->methods[0], &nr, &lne);
> -                               if (ret == JVMTI_ERROR_NONE) {
> -                                       /* free what was allocated for nothing */
> -                                       (*jvmti)->Deallocate(jvmti, (unsigned char *)lne);
> -                                       nr_total += (int)nr;
> -                               } else if (ret == JVMTI_ERROR_ABSENT_INFORMATION
> -                                          || ret == JVMTI_ERROR_NATIVE_METHOD) {
> -                                       /* No debug information for this method */
> -                               } else {
> -                                       print_error(jvmti, "GetLineNumberTable", ret);
> -                               }
> -                       }
> +                       nr_total += rec->numpcs;
>                 }
>         }
>
> @@ -122,14 +107,12 @@ get_line_numbers(jvmtiEnv *jvmti, const void *compile_info, jvmti_line_info_t **
>                         rec = (jvmtiCompiledMethodLoadInlineRecord *)hdr;
>                         for (i = 0; i < rec->numpcs; i++) {
>                                 c = rec->pcinfo + i;
> -                               nr = 0;
> -                               ret = do_get_line_numbers(jvmti, c->pc,
> -                                                         c->methods[0],
> -                                                         c->bcis[0],
> -                                                         *tab + lines_total,
> -                                                         &nr);
> +                               ret = do_get_line_number(jvmti, c->pc,
> +                                                        c->methods[0],
> +                                                        c->bcis[0],
> +                                                        *tab + lines_total);

Nit: It'd be nice to comment here that [0] is the leaf method/bci. I
don't believe it is supported but c->numstackframes could be used to
give full inlining information.

Thanks,
Ian


>                                 if (ret == JVMTI_ERROR_NONE)
> -                                       lines_total += nr;
> +                                       lines_total++;
>                         }
>                 }
>         }
> diff --git a/tools/perf/util/genelf_debug.c b/tools/perf/util/genelf_debug.c
> index 30e9f618f6cd..dd40683bd4c0 100644
> --- a/tools/perf/util/genelf_debug.c
> +++ b/tools/perf/util/genelf_debug.c
> @@ -342,7 +342,7 @@ static void emit_lineno_info(struct buffer_ext *be,
>          */
>
>         /* start state of the state machine we take care of */
> -       unsigned long last_vma = code_addr;
> +       unsigned long last_vma = 0;
>         char const  *cur_filename = NULL;
>         unsigned long cur_file_idx = 0;
>         int last_line = 1;
> @@ -473,7 +473,7 @@ jit_process_debug_info(uint64_t code_addr,
>                 ent = debug_entry_next(ent);
>         }
>         add_compilation_unit(di, buffer_ext_size(dl));
> -       add_debug_line(dl, debug, nr_debug_entries, 0);
> +       add_debug_line(dl, debug, nr_debug_entries, GEN_ELF_TEXT_OFFSET);
>         add_debug_abbrev(da);
>         if (0) buffer_ext_dump(da, "abbrev");
>
> --
> 2.26.2
>

Re: [PATCH] perf jvmti: remove redundant jitdump line table entries

[Nick Gasson]

On 05/26/20 19:55 PM, Jiri Olsa wrote:
> On Fri, May 22, 2020 at 02:53:30PM +0800, Nick Gasson wrote:
>> For each PC/BCI pair in the JVMTI compiler inlining record table, the
>> jitdump plugin emits debug line table entries for every source line in
>> the method preceding that BCI. Instead only emit one source line per
>> PC/BCI pair. Reported by Ian Rogers. This reduces the .dump size for
>> SPECjbb from ~230MB to ~40MB.
>> 
>> Also fix an error in the DWARF line table state machine where addresses
>> are incorrectly offset by -0x40 (GEN_ELF_TEXT_OFFSET). This can be seen
>> with `objdump -S` on the ELF files after perf inject.
>
> hi,
> I can't apply this on latest Arnaldo's perf/core:
>
> patching file jvmti/libjvmti.c
> Hunk #1 FAILED at 32.
> Hunk #2 succeeded at 67 (offset -4 lines).
> Hunk #3 FAILED at 85.
> Hunk #4 succeeded at 114 (offset -7 lines).
>

Sorry I based this on my earlier patch series below but I didn't realise
that wasn't merged to perf/core yet. Could those patches be applied
first? I believe Ian added a Reviewed-by for all three.

https://lore.kernel.org/lkml/20200427061520.24905-3-nick.gasson@arm.com/T/


>
>> 
>> Signed-off-by: Nick Gasson <nick.gasson@arm.com>
>> ---
>>  tools/perf/jvmti/libjvmti.c    | 73 +++++++++++++---------------------
>>  tools/perf/util/genelf_debug.c |  4 +-
>>  2 files changed, 30 insertions(+), 47 deletions(-)
>> 
>> diff --git a/tools/perf/jvmti/libjvmti.c b/tools/perf/jvmti/libjvmti.c
>> index a9a056d68416..398e4ba6498d 100644
>> --- a/tools/perf/jvmti/libjvmti.c
>> +++ b/tools/perf/jvmti/libjvmti.c
>> @@ -32,38 +32,41 @@ static void print_error(jvmtiEnv *jvmti, const char *msg, jvmtiError ret)
>>  
>>  #ifdef HAVE_JVMTI_CMLR
>>  static jvmtiError
>> -do_get_line_numbers(jvmtiEnv *jvmti, void *pc, jmethodID m, jint bci,
>> -		    jvmti_line_info_t *tab, jint *nr)
>> +do_get_line_number(jvmtiEnv *jvmti, void *pc, jmethodID m, jint bci,
>> +		   jvmti_line_info_t *tab)
>>  {
>> -	jint i, lines = 0;
>> -	jint nr_lines = 0;
>> +	jint i, nr_lines = 0;
>>  	jvmtiLineNumberEntry *loc_tab = NULL;
>>  	jvmtiError ret;
>> +	jint src_line = -1;
>>  
>>  	ret = (*jvmti)->GetLineNumberTable(jvmti, m, &nr_lines, &loc_tab);
>>  	if (ret == JVMTI_ERROR_ABSENT_INFORMATION || ret == JVMTI_ERROR_NATIVE_METHOD) {
>>  		/* No debug information for this method */
>> -		*nr = 0;
>> -		return JVMTI_ERROR_NONE;
>> +		return ret;
>>  	} else if (ret != JVMTI_ERROR_NONE) {
>>  		print_error(jvmti, "GetLineNumberTable", ret);
>>  		return ret;
>>  	}
>>  
>> -	for (i = 0; i < nr_lines; i++) {
>> -		if (loc_tab[i].start_location < bci) {
>> -			tab[lines].pc = (unsigned long)pc;
>> -			tab[lines].line_number = loc_tab[i].line_number;
>> -			tab[lines].discrim = 0; /* not yet used */
>> -			tab[lines].methodID = m;
>> -			lines++;
>> -		} else {
>> -			break;
>> -		}
>> +	for (i = 0; i < nr_lines && loc_tab[i].start_location <= bci; i++) {
>> +		src_line = i;
>> +	}
>> +
>> +	if (src_line != -1) {
>> +		tab->pc = (unsigned long)pc;
>> +		tab->line_number = loc_tab[src_line].line_number;
>> +		tab->discrim = 0; /* not yet used */
>> +		tab->methodID = m;
>> +
>> +		ret = JVMTI_ERROR_NONE;
>> +	} else {
>> +		ret = JVMTI_ERROR_ABSENT_INFORMATION;
>>  	}
>> +
>>  	(*jvmti)->Deallocate(jvmti, (unsigned char *)loc_tab);
>> -	*nr = lines;
>> -	return JVMTI_ERROR_NONE;
>> +
>> +	return ret;
>>  }
>>  
>>  static jvmtiError
>> @@ -71,9 +74,8 @@ get_line_numbers(jvmtiEnv *jvmti, const void *compile_info, jvmti_line_info_t **
>>  {
>>  	const jvmtiCompiledMethodLoadRecordHeader *hdr;
>>  	jvmtiCompiledMethodLoadInlineRecord *rec;
>> -	jvmtiLineNumberEntry *lne = NULL;
>>  	PCStackInfo *c;
>> -	jint nr, ret;
>> +	jint ret;
>>  	int nr_total = 0;
>>  	int i, lines_total = 0;
>>  
>> @@ -86,24 +88,7 @@ get_line_numbers(jvmtiEnv *jvmti, const void *compile_info, jvmti_line_info_t **
>>  	for (hdr = compile_info; hdr != NULL; hdr = hdr->next) {
>>  		if (hdr->kind == JVMTI_CMLR_INLINE_INFO) {
>>  			rec = (jvmtiCompiledMethodLoadInlineRecord *)hdr;
>> -			for (i = 0; i < rec->numpcs; i++) {
>> -				c = rec->pcinfo + i;
>> -				nr = 0;
>> -				/*
>> -				 * unfortunately, need a tab to get the number of lines!
>> -				 */
>> -				ret = (*jvmti)->GetLineNumberTable(jvmti, c->methods[0], &nr, &lne);
>> -				if (ret == JVMTI_ERROR_NONE) {
>> -					/* free what was allocated for nothing */
>> -					(*jvmti)->Deallocate(jvmti, (unsigned char *)lne);
>> -					nr_total += (int)nr;
>> -				} else if (ret == JVMTI_ERROR_ABSENT_INFORMATION
>> -					   || ret == JVMTI_ERROR_NATIVE_METHOD) {
>> -					/* No debug information for this method */
>> -				} else {
>> -					print_error(jvmti, "GetLineNumberTable", ret);
>> -				}
>> -			}
>> +			nr_total += rec->numpcs;
>>  		}
>>  	}
>>  
>> @@ -122,14 +107,12 @@ get_line_numbers(jvmtiEnv *jvmti, const void *compile_info, jvmti_line_info_t **
>>  			rec = (jvmtiCompiledMethodLoadInlineRecord *)hdr;
>>  			for (i = 0; i < rec->numpcs; i++) {
>>  				c = rec->pcinfo + i;
>> -				nr = 0;
>> -				ret = do_get_line_numbers(jvmti, c->pc,
>> -							  c->methods[0],
>> -							  c->bcis[0],
>> -							  *tab + lines_total,
>> -							  &nr);
>> +				ret = do_get_line_number(jvmti, c->pc,
>> +							 c->methods[0],
>> +							 c->bcis[0],
>> +							 *tab + lines_total);
>>  				if (ret == JVMTI_ERROR_NONE)
>> -					lines_total += nr;
>> +					lines_total++;
>>  			}
>>  		}
>>  	}
>> diff --git a/tools/perf/util/genelf_debug.c b/tools/perf/util/genelf_debug.c
>> index 30e9f618f6cd..dd40683bd4c0 100644
>> --- a/tools/perf/util/genelf_debug.c
>> +++ b/tools/perf/util/genelf_debug.c
>> @@ -342,7 +342,7 @@ static void emit_lineno_info(struct buffer_ext *be,
>>  	 */
>>  
>>  	/* start state of the state machine we take care of */
>> -	unsigned long last_vma = code_addr;
>> +	unsigned long last_vma = 0;
>>  	char const  *cur_filename = NULL;
>>  	unsigned long cur_file_idx = 0;
>>  	int last_line = 1;
>> @@ -473,7 +473,7 @@ jit_process_debug_info(uint64_t code_addr,
>>  		ent = debug_entry_next(ent);
>>  	}
>>  	add_compilation_unit(di, buffer_ext_size(dl));
>> -	add_debug_line(dl, debug, nr_debug_entries, 0);
>> +	add_debug_line(dl, debug, nr_debug_entries, GEN_ELF_TEXT_OFFSET);
>>  	add_debug_abbrev(da);
>>  	if (0) buffer_ext_dump(da, "abbrev");
>>  
>> -- 
>> 2.26.2
>> 

Re: [PATCH] perf jvmti: remove redundant jitdump line table entries

[Nick Gasson]

On 05/27/20 13:03 PM, Ian Rogers wrote:
>
> Great result, thanks! I note there is a lack of symbolization when
> benchmarking a few Java applications. I'll try to see if there's a
> sensible resolution for those.
>

I noticed it loses information when the Hotspot code cache is
resized. I've been working around that by setting
-XX:InitialCodeCacheSize and -XX:ReservedCodeCacheSize to large
values. Does this help in your case?

>
> It'd be better to make this into two patches. Also on acme's perf/core
> branch if possible:
> https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/log/?h=perf/core

OK sure, I'll do that.

--
Nick

Re: [PATCH] perf jvmti: remove redundant jitdump line table entries

[Arnaldo Carvalho de Melo]

On May 27, 2020 2:29:05 AM GMT-03:00, Nick Gasson <nick.gasson@arm.com> wrote:
>On 05/26/20 19:55 PM, Jiri Olsa wrote:
>> On Fri, May 22, 2020 at 02:53:30PM +0800, Nick Gasson wrote:
>>> For each PC/BCI pair in the JVMTI compiler inlining record table,
>the
>>> jitdump plugin emits debug line table entries for every source line
>in
>>> the method preceding that BCI. Instead only emit one source line per
>>> PC/BCI pair. Reported by Ian Rogers. This reduces the .dump size for
>>> SPECjbb from ~230MB to ~40MB.
>>> 
>>> Also fix an error in the DWARF line table state machine where
>addresses
>>> are incorrectly offset by -0x40 (GEN_ELF_TEXT_OFFSET). This can be
>seen
>>> with `objdump -S` on the ELF files after perf inject.
>>
>> hi,
>> I can't apply this on latest Arnaldo's perf/core:
>>
>> patching file jvmti/libjvmti.c
>> Hunk #1 FAILED at 32.
>> Hunk #2 succeeded at 67 (offset -4 lines).
>> Hunk #3 FAILED at 85.
>> Hunk #4 succeeded at 114 (offset -7 lines).
>>
>
>Sorry I based this on my earlier patch series below but I didn't
>realise
>that wasn't merged to perf/core yet. Could those patches be applied
>first? I believe Ian added a Reviewed-by for all three.
>
>https://lore.kernel.org/lkml/20200427061520.24905-3-nick.gasson@arm.com/T/

I'll check, thanks
- Arnaldo

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH] perf jvmti: remove redundant jitdump line table entries

[Ian Rogers]

On Tue, May 26, 2020 at 10:40 PM Nick Gasson <nick.gasson@arm.com> wrote:
>
> On 05/27/20 13:03 PM, Ian Rogers wrote:
> >
> > Great result, thanks! I note there is a lack of symbolization when
> > benchmarking a few Java applications. I'll try to see if there's a
> > sensible resolution for those.
> >
>
> I noticed it loses information when the Hotspot code cache is
> resized. I've been working around that by setting
> -XX:InitialCodeCacheSize and -XX:ReservedCodeCacheSize to large
> values. Does this help in your case?

Thanks, I tried and also with Steve's patch:
https://lore.kernel.org/lkml/1590544271-125795-1-git-send-email-steve.maclean@linux.microsoft.com/

Trying something very basic like just the -version command with compile only:
/tmp/perf/perf record -k 1 -e cycles:u -F 6500 -o /tmp/perf.data java
-agentpath:/tmp/perf/libperf-jvmti.so -XX:+PreserveFramePointer
-XX:InitialCodeCacheSize=2G -XX:ReservedCodeCacheSize=2G
-XX:CompileOnly=1 -version
/tmp/perf/perf inject -i /tmp/perf.data -o /tmp/perf-jit.data -j
/tmp/perf/perf report -i /tmp/perf-jit.data

I don't see any of the JDK classes but 35 unknown symbols out of 272.
The JDK classes are stripped to some degree iirc, but we should be
able to give a symbol name as we don't care about local variables and
like.

This isn't a blocker for this patch and perhaps I'm special in seeing
this problem. Thanks,
Ian

> >
> > It'd be better to make this into two patches. Also on acme's perf/core
> > branch if possible:
> > https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/log/?h=perf/core
>
> OK sure, I'll do that.
>
> --
> Nick

Re: [PATCH] perf jvmti: remove redundant jitdump line table entries

[Nick Gasson]

On 05/28/20 02:08 AM, Ian Rogers wrote:
>>
>> I noticed it loses information when the Hotspot code cache is
>> resized. I've been working around that by setting
>> -XX:InitialCodeCacheSize and -XX:ReservedCodeCacheSize to large
>> values. Does this help in your case?
>
> Thanks, I tried and also with Steve's patch:
> https://lore.kernel.org/lkml/1590544271-125795-1-git-send-email-steve.maclean@linux.microsoft.com/

Thanks for the reference! That patch fixes the problem I had with code
cache resizing so the workaround above is no longer necessary.

>
> Trying something very basic like just the -version command with compile only:
> /tmp/perf/perf record -k 1 -e cycles:u -F 6500 -o /tmp/perf.data java
> -agentpath:/tmp/perf/libperf-jvmti.so -XX:+PreserveFramePointer
> -XX:InitialCodeCacheSize=2G -XX:ReservedCodeCacheSize=2G
> -XX:CompileOnly=1 -version
> /tmp/perf/perf inject -i /tmp/perf.data -o /tmp/perf-jit.data -j
> /tmp/perf/perf report -i /tmp/perf-jit.data
>
> I don't see any of the JDK classes but 35 unknown symbols out of 272.
> The JDK classes are stripped to some degree iirc, but we should be
> able to give a symbol name as we don't care about local variables and
> like.
>

I tried this with latest perf/core and JDK 11 but I don't see any
[unknown] from jitted-*.so. All the events are in "Interpreter": I think
the options you want are -Xcomp -Xbatch rather than -XX:CompileOnly=1?
The latter restricts compilation to the named method/package.

There was a bug where no jitdump debug info was written for classes
compiled without line tables. That was fixed by d3ea46da3 ("perf jvmti:
Fix jitdump for methods without debug info").

--
Nick

Re: [PATCH] perf jvmti: remove redundant jitdump line table entries

[Ian Rogers]

On Wed, May 27, 2020 at 9:39 PM Nick Gasson <nick.gasson@arm.com> wrote:
>
> On 05/28/20 02:08 AM, Ian Rogers wrote:
> >>
> >> I noticed it loses information when the Hotspot code cache is
> >> resized. I've been working around that by setting
> >> -XX:InitialCodeCacheSize and -XX:ReservedCodeCacheSize to large
> >> values. Does this help in your case?
> >
> > Thanks, I tried and also with Steve's patch:
> > https://lore.kernel.org/lkml/1590544271-125795-1-git-send-email-steve.maclean@linux.microsoft.com/
>
> Thanks for the reference! That patch fixes the problem I had with code
> cache resizing so the workaround above is no longer necessary.
>
> >
> > Trying something very basic like just the -version command with compile only:
> > /tmp/perf/perf record -k 1 -e cycles:u -F 6500 -o /tmp/perf.data java
> > -agentpath:/tmp/perf/libperf-jvmti.so -XX:+PreserveFramePointer
> > -XX:InitialCodeCacheSize=2G -XX:ReservedCodeCacheSize=2G
> > -XX:CompileOnly=1 -version
> > /tmp/perf/perf inject -i /tmp/perf.data -o /tmp/perf-jit.data -j
> > /tmp/perf/perf report -i /tmp/perf-jit.data
> >
> > I don't see any of the JDK classes but 35 unknown symbols out of 272.
> > The JDK classes are stripped to some degree iirc, but we should be
> > able to give a symbol name as we don't care about local variables and
> > like.
> >
>
> I tried this with latest perf/core and JDK 11 but I don't see any
> [unknown] from jitted-*.so. All the events are in "Interpreter": I think
> the options you want are -Xcomp -Xbatch rather than -XX:CompileOnly=1?
> The latter restricts compilation to the named method/package.
>
> There was a bug where no jitdump debug info was written for classes
> compiled without line tables. That was fixed by d3ea46da3 ("perf jvmti:
> Fix jitdump for methods without debug info").

Thanks, that fixed it! There are still two unknowns running -version
with Xcomp for me, I suspect they are both C2 stub routines as there
is some work necessary on these. Fwiw, I tried -client and fighting
the tiered compilation flags but to no avail.

Thanks,
Ian

> --
> Nick