<div dir="ltr">0001:<div><br></div><div>+                              is not None]<br>You don't need is not None here. .search() will evaluate to a Match object or None, which can be treated as a boolean.</div><div><br></div><div><div>+              }}" rel="stylesheet" media="screen"/></div></div><div><br></div><div>Is there any particular reason you've XHTML'd up these tags? It looks like there's a lot of HTML cleanup in this patch that probably shouldn't be there. Stuff like adding </tr> I don't think anyone can argue with (just commit that?) but adding /> to the end of tags isn't HTML, it's XHTML and is not normally done, especially for img,link and meta tags.</div><div><br></div><div>Otherwise this looks fine to me, and thanks for the tests!</div><div><br></div><div>0002: </div><div><div>+  {{ utils.regex_filter_box('filter', '.searchable tr', "Machines...") }}</div></div><div><br></div><div>It looks like you've moved this around the page - from before the <h3>Machines</h3> to after it. Was this deliberate?</div><div><br></div><div><div>+    (function ($) {</div></div><div><br></div><div>I'm not sure why this nested function is necessary, but it was just copy-paste so I won't mention any more about it.</div><div><br></div><div>0003 and 0004: Instead of trying to work out the column to search on, I think it would be easier if you would just give all cells that are searchable some extra class, such as "td-searchable". Then, you wouldn't need to parameterize the build_regex macro; it could just always do something like "$(this).find('td.td-searchable').text()". How does that sound? Especially in 0004, this might simplify things.</div><div><br></div><div>Cheers,</div><div><br></div><div>James</div></div><br><div class="gmail_quote"><div dir="ltr">On Mon, 8 Jun 2015 at 10:21 Kristof Beyls <<a href="mailto:kristof.beyls@arm.com">kristof.beyls@arm.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div lang="EN-GB" link="blue" vlink="purple"><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">Right, so it seems that having both server-side filtering and client-side filtering is useful.<u></u><u></u></span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u> <u></u></span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">The attached patches implement that:<u></u><u></u></span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u> <u></u></span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">0001-Add-*: adds the server-side filtering for both the webui and the email daily report.<u></u><u></u></span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">0002-Factor-out-*: refactors the regex functionality that Chris added for the run and machines pages, so the code isn’t repeated and easier to reuse. NFC.<u></u><u></u></span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">0003-Run-page-*: makes sure that on the run page, only machine names are tested against the regex, not the data in the other columns in the table.<u></u><u></u></span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">0004-Add-regex-*: adds the client-side regex filtering for machine names on the daily report page.<u></u><u></u></span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u> <u></u></span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">These patches implement a consistent interface for server-side daily report filtering on machine names, both for the webui and the daily report.<u></u><u></u></span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">The client-side regex filtering is factored out, so having a consistent look-and-feel for the client-side filtering is maintained.<u></u><u></u></span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">If useful, on the daily report page, a search box for filtering based on benchmark name could also be added easily after the above patches have been committed.<u></u><u></u></span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u> <u></u></span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">Does this look OK?<u></u><u></u></span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u> <u></u></span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">Thanks,<u></u><u></u></span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u> <u></u></span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">Kristof<u></u><u></u></span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u> <u></u></span></p><div style="border:none;border-left:solid blue 1.5pt;padding:0cm 0cm 0cm 4.0pt"><div><div style="border:none;border-top:solid #b5c4df 1.0pt;padding:3.0pt 0cm 0cm 0cm"><p class="MsoNormal"><b><span lang="EN-US" style="font-size:10.0pt;font-family:"Tahoma","sans-serif"">From:</span></b><span lang="EN-US" style="font-size:10.0pt;font-family:"Tahoma","sans-serif""> Chris Matthews [mailto:<a href="mailto:chris.matthews@apple.com" target="_blank">chris.matthews@apple.com</a>] <br><b>Sent:</b> 04 June 2015 21:11<br><b>To:</b> Kristof Beyls<br><b>Cc:</b> James Molloy; llvm-commits</span></p></div></div></div></div></div><div lang="EN-GB" link="blue" vlink="purple"><div><div style="border:none;border-left:solid blue 1.5pt;padding:0cm 0cm 0cm 4.0pt"><div><div style="border:none;border-top:solid #b5c4df 1.0pt;padding:3.0pt 0cm 0cm 0cm"><p class="MsoNormal"><span lang="EN-US" style="font-size:10.0pt;font-family:"Tahoma","sans-serif""><br><b>Subject:</b> Re: [PATCH][LNT] Add ability to filter machines on daily report page + refactor LNT regression tests to enable properly testing it.<u></u><u></u></span></p></div></div></div></div></div><div lang="EN-GB" link="blue" vlink="purple"><div><div style="border:none;border-left:solid blue 1.5pt;padding:0cm 0cm 0cm 4.0pt"><p class="MsoNormal"><u></u> <u></u></p><div><p class="MsoNormal">I added some simple regex filters to the run and machines page.  They would work for this purpose too.<u></u><u></u></p></div><div><p class="MsoNormal"><u></u> <u></u></p></div><div><p class="MsoNormal">Server side filtering has the advantage of limiting the query size, which is super useful for me. Our 7 day reports runs on enough machines that it is slow (~100k samples returned by the query).  Being able to limit it would be handy.<u></u><u></u></p></div><div><p class="MsoNormal"><u></u> <u></u></p></div><p class="MsoNormal"><u></u> <u></u></p><div><blockquote style="margin-top:5.0pt;margin-bottom:5.0pt"><div><p class="MsoNormal">On Jun 4, 2015, at 7:04 AM, Kristof Beyls <<a href="mailto:kristof.beyls@arm.com" target="_blank">kristof.beyls@arm.com</a>> wrote:<u></u><u></u></p></div><p class="MsoNormal"><u></u> <u></u></p><div><div><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">Yeah – that’s another possibility.</span><u></u><u></u></p></div><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">I do like keeping the functionality that with the URL you can specify which machines to see by default, e.g.<br>allowing to bookmark your favourite machines.<br>I’m assuming it’s easy to access URL query parameters from javascript. If so, the query parameter could<br>control which checkboxes are enabled by default.</span><u></u><u></u></p></div><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"> </span><u></u><u></u></p></div><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">I think the biggest disadvantage of doing it with javascript is that the interface becomes inconsistent<br>with the “lnt send-daily-report” command, as that command obviously cannot run javascript regexes.</span><u></u><u></u></p></div><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">I haven’t checked in detail, but there probably are differences between javascript regexes and python regexes.</span><u></u><u></u></p></div><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"> </span><u></u><u></u></p></div><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">Another potential advantage of doing the filtering server-side is that the size of the daily report<br>page could be reduced.  However, I haven’t seen examples of huge daily report pages so far, so<br>I think this is currently just a theoretical advantage.</span><u></u><u></u></p></div><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"> </span><u></u><u></u></p></div><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">Overall, I like your suggestion of checkboxes and enabling interactive filtering – but it would be nice<br>to find some kind of solution to keep the interface between the daily report webui and the daily report<br>emailing command consistent. Maybe the differences between javascript regexes and python regexes<br>won’t be much of a problem in practice?</span><u></u><u></u></p></div><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"> </span><u></u><u></u></p></div><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">Thanks,</span><u></u><u></u></p></div><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"> </span><u></u><u></u></p></div><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">Kristof</span><u></u><u></u></p></div><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"> </span><u></u><u></u></p></div><div style="border:none;border-left:solid blue 1.5pt;padding:0cm 0cm 0cm 4.0pt"><div><div style="border:none;border-top:solid #b5c4df 1.0pt;padding:3.0pt 0cm 0cm 0cm"><div><p class="MsoNormal"><b><span lang="EN-US" style="font-size:10.0pt;font-family:"Tahoma","sans-serif"">From:</span></b><span lang="EN-US" style="font-size:10.0pt;font-family:"Tahoma","sans-serif""> James Molloy [<a href="mailto:james@jamesmolloy.co.uk" target="_blank">mailto:james@jamesmolloy.co.uk</a>] <br><b>Sent:</b> 04 June 2015 13:51<br><b>To:</b> Kristof Beyls; Chris Matthews<br><b>Cc:</b> llvm-commits<br><b>Subject:</b> Re: [PATCH][LNT] Add ability to filter machines on daily report page + refactor LNT regression tests to enable properly testing it.</span><u></u><u></u></p></div></div></div><div><p class="MsoNormal"> <u></u><u></u></p></div><div><p class="MsoNormal" style="margin-bottom:12.0pt">Hi Kristof,<u></u><u></u></p><div><div><p class="MsoNormal">I was thinking about this today, and I think the easiest way to work this from a UI perspective would be to just provide checkboxes next to the machine in the overview table - toggling these boxes would toggle the visibility of results from that run on-the-fly (via javascript), similar to how the new test regexp filter box works in the v4_runs page.<u></u><u></u></p></div></div><div><div><p class="MsoNormal"> <u></u><u></u></p></div></div><div><div><p class="MsoNormal">What do you think?<u></u><u></u></p></div></div><div><div><p class="MsoNormal"> <u></u><u></u></p></div></div><div><div><p class="MsoNormal">Cheers,<u></u><u></u></p></div></div><div><div><p class="MsoNormal"> <u></u><u></u></p></div></div><div><div><p class="MsoNormal">James<u></u><u></u></p></div></div></div><div><p class="MsoNormal"> <u></u><u></u></p></div><div><div><div><p class="MsoNormal">On Thu, 4 Jun 2015 at 12:09 Kristof Beyls <<a href="mailto:kristof.beyls@arm.com" target="_blank">kristof.beyls@arm.com</a>> wrote:<u></u><u></u></p></div></div><blockquote style="border:none;border-left:solid #cccccc 1.0pt;padding:0cm 0cm 0cm 6.0pt;margin-left:4.8pt;margin-top:5.0pt;margin-right:0cm;margin-bottom:5.0pt"><div><p class="MsoNormal">Thanks for the review, I've committed the test refactorings as<br>r238296-r238300. You're right, it makes sense to filter machines<br>based on a pattern rather than on the full name. I've considered<br>just a sub-string search, matching with a glob or matching with<br>a python regex. I feel that matching with a python regex is the<br>best solution as it allows the most flexible filtering.<br><br>I went for "?filter-machine-regex=" to enable filtering on the<br>daily report webui and "--filter_machine_regex" to enable<br>filtering when using the lnt send-daily-report command.<br><br>See attached patch for details. Does this look OK to you?<br><br>Kristof<br><br>> -----Original Message-----<br>> From: Chris Matthews [mailto:<a href="mailto:chris.matthews@apple.com" target="_blank">chris.matthews@apple.com</a>]<br>> Sent: 26 May 2015 19:28<br>> To: Kristof Beyls<br>> Cc: llvm-commits<br>> Subject: Re: [PATCH][LNT] Add ability to filter machines on daily report<br>> page + refactor LNT regression tests to enable properly testing it.<br>><br>> I completely agree with this testing strategy. This is a much better way<br>> to test.<br>><br>> I'm fine with an explicit machine list, but I wonder if a more useful<br>> way to do this is to have a machine name filter? That way queries like<br>> "?filter-machine=AArch64&" could be possible along with an explicit<br>> "?filter-machine=lvm-juno-lnt-perf__LNT-AArch64-A53-<br>> O3__clang_DEV__aarch64&".  Just substring matching on the machine name.<br>> That would make it very easy to grab classes of machines.<br>><br>> Please add period to end of comment on patch 6 line 46.<br>><br>> Could you update the command line daily report tool<br>> (lnt/lnttool/main.py:304) to also have this functioanltiy? I think the<br>> change should be really simple.<br>><br>> Otherwise looks good!<br>><br>> > On May 26, 2015, at 3:41 AM, Kristof Beyls <<a href="mailto:kristof.beyls@arm.com" target="_blank">kristof.beyls@arm.com</a>><br>> wrote:<br>> ><br>> > Hi Chris,<br>> ><br>> > I'd like to be able to filter on the daily report page, e.g.<br>> > <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_perf_db-5Fdefault_v4_nts_daily-5Freport_2015_5_21-3Fnum-5Fdays&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=1Am1BhjpBBecTZAEwS5xmBkgMBDe04gUMHqibEKfn4E&s=vfxCTOTu45WeIDSbO6q4Hv50Z1PaOw87bSS4C3QahRk&e=" target="_blank">http://llvm.org/perf/db_default/v4/nts/daily_report/2015/5/21?num_days</a><br>> > =7, so that it only shows data for a subset of the builders. The<br>> > previous link shows many results coming from the penryn builders,<br>> > which make it hard to just investigate the results coming from the<br>> > juno board.<br>> ><br>> > 0006-Enable-filtering-of-machines-shown-on-LNT-daily-repo.patch adds<br>> > support for this; the user visible behaviour I went for is adding<br>> > machines={comma-separated list of machine names} to the query string<br>> > of the url, e.g.<br>> > <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_perf_db-5Fdefault_v4_nts_daily-5Freport_2015_5_21-3Fnum-5Fdays&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=1Am1BhjpBBecTZAEwS5xmBkgMBDe04gUMHqibEKfn4E&s=vfxCTOTu45WeIDSbO6q4Hv50Z1PaOw87bSS4C3QahRk&e=" target="_blank">http://llvm.org/perf/db_default/v4/nts/daily_report/2015/5/21?num_days</a><br>> > =7&mac<br>> > hines=llvm-juno-lnt-perf__LNT-AArch64-A53-O3__clang_DEV__aarch64.<br>> ><br>> > To be able to add a proper regression test/unit test for this, I also<br>> > had to refactor the LNT regression tests a bit, which is what the<br>> > other patches contain. In a number of steps, they transform the tests<br>> > using tests/SharedInputs/SmallInstance/lnt.db to using a database<br>> > constructed from human-readable SQL statements, creating the sqlite<br>> > file on the fly during test execution. This enables adding per-test<br>> > records to the database:<br>> ><br>> > - 0001-For-the-regression-tests-make-temporary-LNT-db-insta.patch<br>> > - 0002-Replace-binary-test-lnt.db-with-human-readable-text-.patch<br>> > - 0003-replace-hexadecimal-encoding-of-blobs-with-textual-r.patch<br>> > - 0004-remove-most-of-the-unnecessary-insert-statement-from.patch<br>> > - 0005-Enable-adding-test-specific-records-to-test-database.patch<br>> ><br>> > My hope is that this refactoring enables creating new functionality<br>> > for the LNT ui in a more Test-Driven Development way, which should<br>> > make it easier for more people to contribute to LNT & to reduce the<br>> > likelihood of regressions in LNT.<br>> ><br>> > What do you think?<br>> > Is the refactoring of the regression tests a step in the right<br>> direction?<br>> > Any objections to adding the machine= filter to the daily report page?<br>> ><br>> > Thanks,<br>> ><br>> > Kristof<br>> > <0001-For-the-regression-tests-make-temporary-LNT-db-insta.patch><0002<br>> > -Replace-binary-test-lnt.db-with-human-readable-text-.patch><0003-repl<br>> > ace-hexadecimal-encoding-of-blobs-with-textual-r.patch><0004-remove-mo<br>> > st-of-the-unnecessary-insert-statement-from.patch><0005-Enable-adding-<br>> > test-specific-records-to-test-database.patch><0006-Enable-filtering-of<br>> > -machines-shown-on-LNT-daily-repo.patch><br><br>_______________________________________________<br>llvm-commits mailing list<br><a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br><a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><u></u><u></u></p></div></blockquote></div></div></div></div></blockquote></div><p class="MsoNormal"><u></u> <u></u></p></div></div></div></blockquote></div>