timbunce / devel-nytprof

Devel::NYTProf is a powerful feature-rich source code profiler for Perl. (Mostly in maintenance mode, so PRs are much more likely to be acted upon than Issues.)
http://blog.timbunce.org/tag/nytprof/
67 stars 50 forks source link

nytprofhtml - internal anchor links in html pages go to wrong locations #192

Open shawnlaffan opened 2 years ago

shawnlaffan commented 2 years ago

Internal links to anchors in pages generated using recent versions of nytprofhtml go to an offset position in the page. The offset varies, and can be above or below. It is, however, consistent for each link.

First occurs with 6.07. Does not occur with 6.06.

To reproduce, run nytprofhtml on the results of a profiling run. Open index.html and click on any link to a subroutine, in the flamegraph or otherwise. Then click on any of the other links in the next page, and so on.

Refreshing the page stays at the link, but selecting the URL address and hitting enter goes to the correct location on Firefox (but not on Chrome or MS Edge).

The only meaningful difference I can see in the files produced between these versions is that jquery-tablesorter-min.js has been replaced by jquery.tablesorter.min.js. However, working with js files is outside my skill set so I cannot delve further.

My operating system is Windows. Occurs across browsers (tested with Firefox, Chrome and MS Edge).

jkeenan commented 2 years ago

Rearranging OP to better respond ....

The only meaningful difference I can see in the files produced between these versions is that jquery-tablesorter-min.js has been replaced by jquery.tablesorter.min.js.

It should be noted that the switch of Jquery files took place between 6.09 and 6.10.

$ git diff -w v6.09..v6.10 -- MANIFEST
diff --git a/MANIFEST b/MANIFEST
index c4ed0fa..8ad1879 100644
--- a/MANIFEST
+++ b/MANIFEST
@@ -41,8 +41,8 @@ lib/Devel/NYTProf/js/jit/jit-yc.js
 lib/Devel/NYTProf/js/jit/jit.js
 lib/Devel/NYTProf/js/jit/Treemap.css
 lib/Devel/NYTProf/js/jquery-min.js
-lib/Devel/NYTProf/js/jquery-tablesorter-min.js
 lib/Devel/NYTProf/js/jquery.floatThead.min.js
+lib/Devel/NYTProf/js/jquery.tablesorter.min.js
[snip]

However, working with js files is outside my skill set so I cannot delve further.

Beyond my skill set as well.

Internal links to anchors in pages generated using recent versions of nytprofhtml go to an offset position in the page. The offset varies, and can be above or below. It is, however, consistent for each link.

First occurs with 6.07. Does not occur with 6.06.

Here is a possible explanation.

In 2016, the following commit was added to the master branch in the GH repository:

commit b21dd1bebd895f5016ccf7f38b703d1d47d4467a (HEAD)
Author:     Sebastian Rose, Hannover, Germany <sebastian_rose@gmx.de>
AuthorDate: Sat Oct 8 14:19:56 2016 +0200
Commit:     Sebastian Rose, Hannover, Germany <sebastian_rose@gmx.de>
CommitDate: Sat Oct 8 14:19:56 2016 +0200

    floatHeaders keep <TH>s on top.  Update jQuery to latest 1.x version.

    This is a first attempt to keep table headers visible when scrolling.  A feature
    requested by PenolopeFudd (https://github.com/PenelopeFudd).  See issue #14 on
    github:  https://github.com/timbunce/devel-nytprof/issues/14

    THIS REVISION MIGHT NOT WORK AS EXPECTED IN SOME BROWSERS!

    The author tested in a fairly recent version of Firefox, where everything works
    like charm.  Some quirks where encountered in an ancient Opera (version 12 -
    current version is 40.somthing).
...

It was at this commit that lib/Devel/NYTProf/js/jquery.floatThead.min.js was added to the repository. (Note that this was way before I myself had anything to do with Devel-NYTProf.)

However, it appears that this file was not added to the MANIFEST at that time, so it would not have been distributed via a tarball uploaded to CPAN. The file was finally added to MANIFEST as follows:

commit 63f7a8eba0b3f84caddc4dd531bc5b7750739118
Author:     Matt Lawrence <matthew.lawrence@humanstate.com>
AuthorDate: Fri Sep 25 12:24:45 2020 +0100
Commit:     James E Keenan <jkeenan@cpan.org>
CommitDate: Mon Apr 5 08:47:47 2021 -0400

    Add the floatThead js file to manifest

    this was omitted in b21dd1bebd895f5016ccf7f38b703d1d47d4467a, so the
    file was not distributed.

jquery.floatThead.min.js first appears in the MANIFEST in v6.07. The file was subsequently modified in this commit:

commit 7f1d65eac300b1af5a6fd6d27c85cf529a01f93a
Author:     Reini Urban <rurban@cpan.org>
AuthorDate: Thu Jun 6 09:07:32 2019 +0200
Commit:     James E Keenan <jkeenan@cpan.org>
CommitDate: Tue May 4 15:31:51 2021 -0400

    CVE-2019-11358 jquery-1.12.4.js update to latest

    Also update the two plugins to latest,
    and adjusted the tablesorter css.

To reproduce, run nytprofhtml on the results of a profiling run. Open index.html and click on any link to a subroutine, in the flamegraph or otherwise. Then click on any of the other links in the next page, and so on.

The test suite, being text-based, is inherently ill equipped to spot deficiencies in the generated HTML. Is there some very simple program over which you could run the 'nytprof' utility that would enable us to spot the differences in generated HTML?

Refreshing the page stays at the link, but selecting the URL address and hitting enter goes to the correct location on Firefox (but not on Chrome or MS Edge).

My operating system is Windows. Occurs across browsers (tested with Firefox, Chrome and MS Edge).

I'm a bit confused ... is there a problem with Firefox or not? (Note that if there are problems with one browser but not on another, then the problem may be out of our control.)

Thank you very much. Jim Keenan

shawnlaffan commented 2 years ago

Thanks for the detailed investigations. It looks like an issue with jquery.tablesorter.min.js.

As an experiment, I ran nytprofhtml on a profiling run. I then moved the nytprof/js/jquery.tablesorter.min.js file out of the way. On opening nytprof/index.html the html links now render correctly.

The rest of this post is to answer your queries, slightly reordered.

I'm a bit confused ... is there a problem with Firefox or not? (Note that if there are problems with one browser but not on another, then the problem may be out of our control.)

The issue impacts all three browsers under Windows 10, but Firefox has the workaround I described. As an additional point, I have reproduced the issue on Ubuntu 18 with firefox as the browser.

Is there some very simple program over which you could run the 'nytprof' utility that would enable us to spot the differences in generated HTML?

An example script is below. Call the usual sequence of perl -d:NYTProf script.pl and then nytprofhtml --open. In the browser, click on run_sub or run_sub2 in either the flamegraph or the subroutine list. Note how the selected sub is not centred on the page.

I used the tree and diff utilities in bash to see the differences between directory contents generated by different versions of Devel::NYTProf.

use 5.010;
use strict;
use warnings;

my @data = (0..100);

foreach my $i (@data) {
    run_sub ($i);
}

sub run_sub {
    my $x = shift;
    my $y = run_sub2($x+1);
    return $y;
}

#  extra subs to pad out the html 
#  in the nyt report
#  run_sub2 is in the middle

sub extra_sub1 {
    my $x = shift;
    my $y = run_sub2($x+1);

    my $padding = << 'EOPADDING'

random random random random
random random random random
random random random random
random random random random
random random random random
random mandor random random
random random random random
random random random random
random random random random
random random random random
random random random random
random random random random
random random random random
random random random random
random random random random
random random random random
random random random random
random random random random
random random random random
random random random random
random random random random
random random random random
random random random random
random random random random
random random random random
random random random random
random random random random
random random random random
random random random random
random random random random
random random random random

EOPADDING
}

sub run_sub2 {
    my $y = shift;
    #say $y;
    my $count = grep  {$_ < $y} (0..100);
    #say $count if $count;
    return $count;
}

sub extra_sub6 {
    my $x = shift;
    my $y = run_sub2($x+1);
    return $y;
}

sub extra_sub7 {
    my $x = shift;
    my $y = run_sub2($x+1);
    return $y;
}

sub extra_sub8 {
    my $x = shift;
    my $y = run_sub2($x+1);
    return $y;
}

sub extra_sub9 {
    my $x = shift;
    my $y = run_sub2($x+1);
    return $y;
}
jkeenan commented 2 years ago

This should be resolved with Devel-NYTProf version 6.11, just released to CPAN.

jkeenan commented 2 years ago

I went looking to close this ticket today and installed Devel::NYTProf v6.11 from CPAN, then ran the program which the OP provided. I was not very satisfied with the results. In both cases, the location to which I expected to be taken when clicking on a link was "below the screen" (or just at the bottom of the visible window when in full screen mode).

However, I know nothing about Javascript, jquery, etc., so I am at a loss as to how to improve the situation further. I'm going to close this ticket in 7 days unless someone suggests a better approach.

shawnlaffan commented 2 years ago

The file causing the problems is jquery.tablesorter.min.js, but it was jquery.floatThead.min.js that was removed.

Sorry for not spotting that when it was first implemented.

jkeenan commented 2 years ago

On 9/24/21 9:13 PM, shawnlaffan wrote:

The file causing the problems is |jquery.tablesorter.min.js|, but it was |jquery.floatThead.min.js| that was removed.

Sorry for not spotting that when it was first implemented.

Okay, within a few days, I'll restore |jquery.floatThead.min.js| and remove |jquery.tablesorter.min.js|. I'll do that in a branch of my github repository and ask you to test it out before I push to the main repository (timbunce) and issue a new CPAN release.

Thank you very much. Jim Keenan

shawnlaffan commented 2 years ago

That plan works for me.

jkeenan commented 2 years ago

I'm starting to think that the "bad" output I got with Shawn Laffan's test file once I installed Devel-NYTProf-6.11 from CPAN is the result of installing this version against the same perl against which I had installed earlier versions -- and that (perhaps) I'm getting "bad" results from different "generations" of .js files.

Why do I suspect this? Because when I install Devel-NYTProf-6.11 against a fresh perl and run Shawn's test file, I get good results, i.e., HTML tables properly "anchored" within the screen. Results apparently just as good as when I install Devel-NYTProf-6.06 against another fresh perl at the same commit in Perl 5 blead.

I figured that in order to have a "baseline" against which to measure the effect of changes, I would install a fresh perl, install Devel-NYTProf-6.06 against it, run Shawn's program, then take screenshots of the generated HTML output. You can find those screenshots here; you can see a shot of the index page, then shots of clicking on run_sub and run_sub2 in the subroutines table, then shots of clicking on run_sub and run_sub2 from the flamegraph.

The steps I took to generate these results were:

Devel-NYTProf-6.06

o Installed Perl 5 blead (2c205b5406). o Installed cpanm. o Installed Devel-NYTProf-6.06 from tarball. o Note: This required a `--force``, as there was one, not unexpected test failure due to the problems we got in perl-5.33. I don't believe that that affects the problem at hand. o Created tempdir and aliases.

$ mkdir -p /home/jkeenan/tmp/2c205b5406-NYTProf-6.06
$ cd /home/jkeenan/tmp/2c205b5406-NYTProf-6.06
$ alias thisperl="/home/jkeenan/testing/blead/bin/perl -I/home/jkeenan/testing/blead/lib"
$ thisperl -MDevel::NYTProf -E 'say $Devel::NYTProf::VERSION;'
6.06
fid 1 has no src saved for -e (NYTP_FIDf_HAS_SRC not set but src available!)
$ alias thisnytprofhtml="/home/jkeenan/testing/blead/bin/nytprofhtml"
$ thisperl -d:NYTProf ~/learn/perl/nytprof/shawnlaffan.pl
$ thisnytprofhtml --open
Reading nytprof.out
Processing nytprof.out data
Writing line reports to nytprof directory
 100% ... 
Extracting subroutine call data ...
Extracting subroutine links
Generating subroutine stack flame graph ...
Devel-NYTProf-6.11

o Installed Perl 5 blead (2c205b5406). o Installed cpanm. o Installed Devel-NYTProf-6.11 from tarball. o Created tempdir and aliases.

$ mkdir -p /home/jkeenan/tmp/2c205b5406-NYTProf-6.11
$ cd /home/jkeenan/tmp/2c205b5406-NYTProf-6.11
$ alias thisperl="/home/jkeenan/testing/blead/bin/perl -I/home/jkeenan/testing/blead/lib"
$ thisperl -MDevel::NYTProf -E 'say $Devel::NYTProf::VERSION;'
6.11
fid 1 has no src saved for -e (NYTP_FIDf_HAS_SRC not set but src available!)
$ alias thisnytprofhtml="/home/jkeenan/testing/blead/bin/nytprofhtml"
$ thisperl -d:NYTProf ~/learn/perl/nytprof/shawnlaffan.pl
$ thisnytprofhtml --open
Reading nytprof.out
Processing nytprof.out data
Writing line reports to nytprof directory
 100% ... 
Extracting subroutine call data ...
Extracting subroutine links
Generating subroutine stack flame graph ...

I then eyeballed the generated HTML files. I couldn't detect any formatting differences between the 6.06 and 6.11 versions.

Upon reflection, I recalled that this was the result I was getting when I began working on this issue after @shawnlaffan reported it earlier this month. It was only after I installed 6.11 from CPAN against my "everyday" perl (5.34.0, installed via perlbrew) that I started observing "badly anchored" HTML.

To add to the confusion ...

When I look at the Javascript files that are found in each installation, I observe the following:

# xblead => blead with NYTProf 6.06

[testing] 562 $ find xblead/lib/perl5/site_perl/5.35.5/x86_64-linux/Devel/NYTProf -type f -name '*.js' | xargs ls -l
-r--r--r-- 1 jkeenan jkeenan 276645 Dec 27  2012 xblead/lib/perl5/site_perl/5.35.5/x86_64-linux/Devel/NYTProf/js/jit/jit.js
-r--r--r-- 1 jkeenan jkeenan  70150 Dec 27  2012 xblead/lib/perl5/site_perl/5.35.5/x86_64-linux/Devel/NYTProf/js/jit/jit-yc.js
-r--r--r-- 1 jkeenan jkeenan  97163 Nov 24  2016 xblead/lib/perl5/site_perl/5.35.5/x86_64-linux/Devel/NYTProf/js/jquery-min.js
-r--r--r-- 1 jkeenan jkeenan  12795 Dec 27  2012 xblead/lib/perl5/site_perl/5.35.5/x86_64-linux/Devel/NYTProf/js/jquery-tablesorter-min.js

# blead => blead with NYTProf 6.11

[testing] 563 $ find blead/lib/perl5/site_perl/5.35.5/x86_64-linux/Devel/NYTProf -type f -name '*.js' | xargs ls -l
-r--r--r-- 1 jkeenan jkeenan 276645 Sep 11 11:52 blead/lib/perl5/site_perl/5.35.5/x86_64-linux/Devel/NYTProf/js/jit/jit.js
-r--r--r-- 1 jkeenan jkeenan  70150 Sep 11 11:52 blead/lib/perl5/site_perl/5.35.5/x86_64-linux/Devel/NYTProf/js/jit/jit-yc.js
-r--r--r-- 1 jkeenan jkeenan  88145 Sep 11 11:52 blead/lib/perl5/site_perl/5.35.5/x86_64-linux/Devel/NYTProf/js/jquery-min.js
-r--r--r-- 1 jkeenan jkeenan  44371 Sep 13 17:12 blead/lib/perl5/site_perl/5.35.5/x86_64-linux/Devel/NYTProf/js/jquery.tablesorter.min.js

# Installed 5.34.0 with NYTProf 6.11

[NYTProf] 535 $ pwd
/home/jkeenan/perl5/perlbrew/perls/perl-5.34.0/lib/site_perl/5.34.0/x86_64-linux/Devel/NYTProf
[NYTProf] 536 $ find ./js -type f -name '*.js' | xargs ls -l
-r--r--r-- 1 jkeenan jkeenan 276645 Mar 23  2021 ./js/jit/jit.js
-r--r--r-- 1 jkeenan jkeenan  70150 Mar 23  2021 ./js/jit/jit-yc.js
-r--r--r-- 1 jkeenan jkeenan  88145 May  5 06:55 ./js/jquery-min.js
-r--r--r-- 1 jkeenan jkeenan  44371 May  5 06:55 ./js/jquery.tablesorter.min.js

I get "good" results when I install either 6.06 or 6.11 against a freshly built perl -- even though 1 .js file has different content between 6.06 and 6.11 and another .js file has a name subtly different between 6.06 and 6.11.

But I get "bad" results when I install 6.11 against my previously built perl -- even though the .js files thus installed are the same as when I install 6.11 against a freshly built perl!

This is confusing, to say the least.

@shawnlaffan, @timbunce: Could you try reproducing the process I described here and report: o Do you get significant visual differences when installing 6.06 against a freshly built perl and installing 6.11 against another freshly built perl at the same git commit? o Do you get visual differences when installing 6.06 against a freshly built perl versus installing it against your "everyday" perl?

Thank you very much. Jim Keenan

shawnlaffan commented 2 years ago

I'll have a look and report back, but one point I've noticed is that the nytprof directory needs to be deleted between runs so the js subdir does not contain files from different nytprof releases.

If you are running the test script and then calling nytprofhtml with default arguments then it adds to the directory contents (something I've just noticed). nytprofhtml needs to be called with either the --out arg so the output goes into a new directory, or with --delete.

shawnlaffan commented 2 years ago

WRT the checks, I installed two clean copies of strawberry perl 5.31.1 portable on my machine.

I then installed Devel::NYTProf 6.11 in one of them, and 6.06 in the other. Both produced expected results.

I then installed 6.11 over the top of 6.06 and got the expected results.

Then I installed 6.10 over the top of 6.11. Calling nytprofhtml --delete gives the offset anchor issue.

Then I reinstalled 6.11 over 6.10. This still results in offset anchor issue, even when deleting the report output directory.

So it would seem that a file or files from 6.10 are having an effect on 6.11. Looking at the js directories, these are the jquery-tablesorter-min.js and jquery.floatThead.min.js files, of which the former causes the anchor problems.

Is there a simple way of cleaning up the js dir on module installation?

jkeenan commented 2 years ago

Before proceeding further I want to make a correction to the data I provided yesterday in https://github.com/timbunce/devel-nytprof/issues/192#issuecomment-927178693.

The listing of .js files I provided for the Devel-NYTProf-6.11 installed against my "everyday" perl -- perl.5.34.0 installed via perlbrew -- was misleading. I had manually renamed the extension on one of the files, ./js/jquery.floatThead.min.js, to "get it out of the way." Today I restored its correct name, so the correct listing of "installed" javascript files is as follows:

# Installed 5.34.0 with NYTProf 6.11

$ pwd
/home/jkeenan/perl5/perlbrew/perls/perl-5.34.0/lib/site_perl/5.34.0/x86_64-linux/Devel/NYTProf
$ find ./js -type f -name '*.js' | xargs ls -l
-r--r--r-- 1 jkeenan jkeenan 276645 Mar 23  2021 ./js/jit/jit.js
-r--r--r-- 1 jkeenan jkeenan  70150 Mar 23  2021 ./js/jit/jit-yc.js
-r--r--r-- 1 jkeenan jkeenan  13779 May  5 06:55 ./js/jquery.floatThead.min.js
-r--r--r-- 1 jkeenan jkeenan  88145 May  5 06:55 ./js/jquery-min.js
-r--r--r-- 1 jkeenan jkeenan  44371 May  5 06:55 ./js/jquery.tablesorter.min.js

With this set of files in position, I re-ran @shawnlaffan's test program and then took screenshots.

$ perl -d:NYTProf ~/learn/perl/nytprof/shawnlaffan.pl
$ nytprofhtml --open
Reading nytprof.out
Processing nytprof.out data
Writing line reports to nytprof directory
 100% ... 
Extracting subroutine call data ...
Extracting subroutine links
Generating subroutine stack flame graph ...

The screenshots can be found here. As you can see, the run_sub HTML pages are positioned according to expectation but the run_sub2 pages are not.

jkeenan commented 2 years ago
# Installed 5.34.0 with NYTProf 6.11

$ pwd
/home/jkeenan/perl5/perlbrew/perls/perl-5.34.0/lib/site_perl/5.34.0/x86_64-linux/Devel/NYTProf
$ find ./js -type f -name '*.js' | xargs ls -l
-r--r--r-- 1 jkeenan jkeenan 276645 Mar 23  2021 ./js/jit/jit.js
-r--r--r-- 1 jkeenan jkeenan  70150 Mar 23  2021 ./js/jit/jit-yc.js
-r--r--r-- 1 jkeenan jkeenan  13779 May  5 06:55 ./js/jquery.floatThead.min.js
-r--r--r-- 1 jkeenan jkeenan  88145 May  5 06:55 ./js/jquery-min.js
-r--r--r-- 1 jkeenan jkeenan  44371 May  5 06:55 ./js/jquery.tablesorter.min.js

With this set of files in position, I re-ran @shawnlaffan's test program and then took screenshots.

$ perl -d:NYTProf ~/learn/perl/nytprof/shawnlaffan.pl
$ nytprofhtml --open
Reading nytprof.out
Processing nytprof.out data
Writing line reports to nytprof directory
 100% ... 
Extracting subroutine call data ...
Extracting subroutine links
Generating subroutine stack flame graph ...

The screenshots can be found here. As you can see, the run_sub HTML pages are positioned according to expectation but the run_sub2 pages are not.

The implication I'm drawing from the above is that I'm having problems even when jquery-tablesorter-min.js -- let's call this tablesorter-hyphen to distinguish it from tablesorter-dot -- is no longer found on my machine. I think this is a different inference from the one @shawnlaffan is drawing.

mkoryak commented 2 years ago

Author of floatthead here, I have no idea what your thing does, but I can help you figure out why my thing isn't working.. at least that's what I understand after reading this thread

If you want my help, I would need to generated html/css/js that this tool produces when you run it so I can figure out why floatthead isn't doing what is supposed to do - that is not breaking your stuff.

I have no perl and would rather avoid it. I had a bad experience with it 15 years ago ;)

mkoryak commented 2 years ago

also, looking at https://github.com/timbunce/devel-nytprof/pull/194 - you removed the lib, but you didn't remove the code that uses it, which will now throw a javascript error and break any javascript that runs after it:

https://github.com/timbunce/devel-nytprof/blob/fe6739140f826f58cb1f9fcb37a95f01483de954/bin/nytprofhtml#L422

mkoryak commented 2 years ago

I made a PR to cleanup the issue created in #194 where you removed the script but not its usages. Merging my PR will fix the immediate problem of having broken javascript code on master. After you merge that PR, I suggest that you create a branch where it is rolled back (along with #194), run your tool and send me all the stuff it generates so i can take a look.

I know that the problem you are trying to solve here is actually caused by show_fragment_target(): https://github.com/timbunce/devel-nytprof/blob/master/bin/nytprofhtml#L1693-L1709

It probably broke because someone changed the height of something it uses to decide how to show the thing you want and not have it be under the floating headers.