[PATCH][LNT] Add ability to filter machines on daily report page + refactor LNT regression tests to enable properly testing it.

Mon Jun 8 02:41:00 PDT 2015

0001:

+                              is not None]
You don't need is not None here. .search() will evaluate to a Match object
or None, which can be treated as a boolean.

+              }}" rel="stylesheet" media="screen"/>

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.

Otherwise this looks fine to me, and thanks for the tests!

0002:
+  {{ utils.regex_filter_box('filter', '.searchable tr', "Machines...") }}

It looks like you've moved this around the page - from before the
<h3>Machines</h3> to after it. Was this deliberate?

+    (function ($) {

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.

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.

Cheers,

James

On Mon, 8 Jun 2015 at 10:21 Kristof Beyls <kristof.beyls at arm.com> wrote:

> Right, so it seems that having both server-side filtering and client-side
> filtering is useful.
>
>
>
> The attached patches implement that:
>
>
>
> 0001-Add-*: adds the server-side filtering for both the webui and the
> email daily report.
>
> 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.
>
> 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.
>
> 0004-Add-regex-*: adds the client-side regex filtering for machine names
> on the daily report page.
>
>
>
> These patches implement a consistent interface for server-side daily
> report filtering on machine names, both for the webui and the daily report.
>
> The client-side regex filtering is factored out, so having a consistent
> look-and-feel for the client-side filtering is maintained.
>
> 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.
>
>
>
> Does this look OK?
>
>
>
> Thanks,
>
>
>
> Kristof
>
>
>
> *From:* Chris Matthews [mailto:chris.matthews at apple.com]
> *Sent:* 04 June 2015 21:11
> *To:* Kristof Beyls
> *Cc:* James Molloy; llvm-commits
>
>
> *Subject:* Re: [PATCH][LNT] Add ability to filter machines on daily
> report page + refactor LNT regression tests to enable properly testing it.
>
>
>
> I added some simple regex filters to the run and machines page.  They
> would work for this purpose too.
>
>
>
> 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.
>
>
>
>
>
> On Jun 4, 2015, at 7:04 AM, Kristof Beyls <kristof.beyls at arm.com> wrote:
>
>
>
> Yeah – that’s another possibility.
>
> I do like keeping the functionality that with the URL you can specify
> which machines to see by default, e.g.
> allowing to bookmark your favourite machines.
> I’m assuming it’s easy to access URL query parameters from javascript. If
> so, the query parameter could
> control which checkboxes are enabled by default.
>
>
>
> I think the biggest disadvantage of doing it with javascript is that the
> interface becomes inconsistent
> with the “lnt send-daily-report” command, as that command obviously cannot
> run javascript regexes.
>
> I haven’t checked in detail, but there probably are differences between
> javascript regexes and python regexes.
>
>
>
> Another potential advantage of doing the filtering server-side is that the
> size of the daily report
> page could be reduced.  However, I haven’t seen examples of huge daily
> report pages so far, so
> I think this is currently just a theoretical advantage.
>
>
>
> Overall, I like your suggestion of checkboxes and enabling interactive
> filtering – but it would be nice
> to find some kind of solution to keep the interface between the daily
> report webui and the daily report
> emailing command consistent. Maybe the differences between javascript
> regexes and python regexes
> won’t be much of a problem in practice?
>
>
>
> Thanks,
>
>
>
> Kristof
>
>
>
> *From:* James Molloy [mailto:james at jamesmolloy.co.uk
> <james at jamesmolloy.co.uk>]
> *Sent:* 04 June 2015 13:51
> *To:* Kristof Beyls; Chris Matthews
> *Cc:* llvm-commits
> *Subject:* Re: [PATCH][LNT] Add ability to filter machines on daily
> report page + refactor LNT regression tests to enable properly testing it.
>
>
>
> Hi Kristof,
>
> 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.
>
>
>
> What do you think?
>
>
>
> Cheers,
>
>
>
> James
>
>
>
> On Thu, 4 Jun 2015 at 12:09 Kristof Beyls <kristof.beyls at arm.com> wrote:
>
> Thanks for the review, I've committed the test refactorings as
> r238296-r238300. You're right, it makes sense to filter machines
> based on a pattern rather than on the full name. I've considered
> just a sub-string search, matching with a glob or matching with
> a python regex. I feel that matching with a python regex is the
> best solution as it allows the most flexible filtering.
>
> I went for "?filter-machine-regex=" to enable filtering on the
> daily report webui and "--filter_machine_regex" to enable
> filtering when using the lnt send-daily-report command.
>
> See attached patch for details. Does this look OK to you?
>
> Kristof
>
> > -----Original Message-----
> > From: Chris Matthews [mailto:chris.matthews at apple.com]
> > Sent: 26 May 2015 19:28
> > To: Kristof Beyls
> > Cc: llvm-commits
> > Subject: Re: [PATCH][LNT] Add ability to filter machines on daily report
> > page + refactor LNT regression tests to enable properly testing it.
> >
> > I completely agree with this testing strategy. This is a much better way
> > to test.
> >
> > I'm fine with an explicit machine list, but I wonder if a more useful
> > way to do this is to have a machine name filter? That way queries like
> > "?filter-machine=AArch64&" could be possible along with an explicit
> > "?filter-machine=lvm-juno-lnt-perf__LNT-AArch64-A53-
> > O3__clang_DEV__aarch64&".  Just substring matching on the machine name.
> > That would make it very easy to grab classes of machines.
> >
> > Please add period to end of comment on patch 6 line 46.
> >
> > Could you update the command line daily report tool
> > (lnt/lnttool/main.py:304) to also have this functioanltiy? I think the
> > change should be really simple.
> >
> > Otherwise looks good!
> >
> > > On May 26, 2015, at 3:41 AM, Kristof Beyls <kristof.beyls at arm.com>
> > wrote:
> > >
> > > Hi Chris,
> > >
> > > I'd like to be able to filter on the daily report page, e.g.
> > > http://llvm.org/perf/db_default/v4/nts/daily_report/2015/5/21?num_days
> > > =7, so that it only shows data for a subset of the builders. The
> > > previous link shows many results coming from the penryn builders,
> > > which make it hard to just investigate the results coming from the
> > > juno board.
> > >
> > > 0006-Enable-filtering-of-machines-shown-on-LNT-daily-repo.patch adds
> > > support for this; the user visible behaviour I went for is adding
> > > machines={comma-separated list of machine names} to the query string
> > > of the url, e.g.
> > > http://llvm.org/perf/db_default/v4/nts/daily_report/2015/5/21?num_days
> > > =7&mac
> > > hines=llvm-juno-lnt-perf__LNT-AArch64-A53-O3__clang_DEV__aarch64.
> > >
> > > To be able to add a proper regression test/unit test for this, I also
> > > had to refactor the LNT regression tests a bit, which is what the
> > > other patches contain. In a number of steps, they transform the tests
> > > using tests/SharedInputs/SmallInstance/lnt.db to using a database
> > > constructed from human-readable SQL statements, creating the sqlite
> > > file on the fly during test execution. This enables adding per-test
> > > records to the database:
> > >
> > > - 0001-For-the-regression-tests-make-temporary-LNT-db-insta.patch
> > > - 0002-Replace-binary-test-lnt.db-with-human-readable-text-.patch
> > > - 0003-replace-hexadecimal-encoding-of-blobs-with-textual-r.patch
> > > - 0004-remove-most-of-the-unnecessary-insert-statement-from.patch
> > > - 0005-Enable-adding-test-specific-records-to-test-database.patch
> > >
> > > My hope is that this refactoring enables creating new functionality
> > > for the LNT ui in a more Test-Driven Development way, which should
> > > make it easier for more people to contribute to LNT & to reduce the
> > > likelihood of regressions in LNT.
> > >
> > > What do you think?
> > > Is the refactoring of the regression tests a step in the right
> > direction?
> > > Any objections to adding the machine= filter to the daily report page?
> > >
> > > Thanks,
> > >
> > > Kristof
> > > <0001-For-the-regression-tests-make-temporary-LNT-db-insta.patch><0002
> > > -Replace-binary-test-lnt.db-with-human-readable-text-.patch><0003-repl
> > > ace-hexadecimal-encoding-of-blobs-with-textual-r.patch><0004-remove-mo
> > > st-of-the-unnecessary-insert-statement-from.patch><0005-Enable-adding-
> > > test-specific-records-to-test-database.patch><0006-Enable-filtering-of
> > > -machines-shown-on-LNT-daily-repo.patch>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150608/bf28a07a/attachment.html>