585876 - fast-path json2

Status: RESOLVED FIXED

(Webtools Graveyard :: Tinderbox, defect)

Description

[Robert Helmer [:rhelmer]]

Based on findings from profiling Tinderbox in bug 585814 we could make JSON output faster by not loading data it does not need.

This also simplifies the JSON output and makes it smaller.

Comment 1

[Robert Helmer [:rhelmer]]

Attachment: custom tb_load function for json2

Try out ideas in 585814 comment 6. It's less than half the size of the usual JSON log (both uncompressed and no whitespace).

This is running on stage, you can get a pretty-print locally like this:
curl -H 'Accept-encoding: gzip' 'http://tinderbox-stage.mozilla.org/showbuilds.cgi?tree=Firefox&json2=1' | gzip -dc | python -mjson.tool | less

This patch includes the patch in bug 399190 (that one should be landed first.)

Comment 2

[Robert Helmer [:rhelmer]]

Attachment: rebuild build_table to remove recursive references

This removes the need to use Data::Dumper at all; the problem is that each entry in build_table has a reference to $td. I am not sure why that is, so instead of removing it Tinderbox-wide I built a new hashtable.

The only other thing I would like to see here is perhaps to have tb_load_json_data() iterate over the scrape, warnings and ignore data and fold that into the build_table instead of having separate hashes (it creates lots of redundant data in the feed). 

I think we could go forward without this though, according to profiler data removing Data::Dumper should be a big win (and safer too, the encoder should not have e.g. escaping issues.)

Comment 3

[Mike Taylor [:bear]]

Comment on attachment 464483 [details] [diff] [review]
rebuild build_table to remove recursive references

I'm loving this patch more and more :)

I agree that the next step, after a new round of profiling, would be to look into avoiding having to create the remaining hashes and try to get them into the json in a DRY manner

Comment 4

[Robert Helmer [:rhelmer]]

(In reply to comment #3)
> Comment on attachment 464483 [details] [diff] [review]
> rebuild build_table to remove recursive references
> 
> I'm loving this patch more and more :)
> 
> I agree that the next step, after a new round of profiling, would be to look
> into avoiding having to create the remaining hashes and try to get them into
> the json in a DRY manner

Per IRC, let's get this landed close to as-is and refactor further later if warranted.

One benefit to leaving it this way is that I *think* it should be compatible with the previous JSON, to allow TBPL to easily switch.

Comment 5

[Robert Helmer [:rhelmer]]

Comment on attachment 464483 [details] [diff] [review]
rebuild build_table to remove recursive references

r+ from bear (from IRC) assuming that I land bug 399190 first and merge this one.

Comment 6

[Robert Helmer [:rhelmer]]

Attachment: fast-path json loading [as-landed] [checked-in]

Checking in showbuilds.pl;
/cvsroot/mozilla/webtools/tinderbox/showbuilds.pl,v  <--  showbuilds.pl
new revision: 1.40; previous revision: 1.39
done
Checking in tbglobals.pl;
/cvsroot/mozilla/webtools/tinderbox/tbglobals.pl,v  <--  tbglobals.pl
new revision: 1.75; previous revision: 1.74
done

Comment 7

[Robert Helmer [:rhelmer]]

Testing this in TBPL over in bug 586123 I see that we're missing note_array... looks like this gets called inside make_build_table (which we're trying to avoid).

TBPL also currently uses build_names index, which it looks like is what note_array is really after. It might be simplest to reinstate both of these, I'll see what I can do.

Comment 8

[Robert Helmer [:rhelmer]]

Attachment: make note array and build indexes work

For some reason load_notes() gets called from make_build_table and requires this  "build_table" structure. Like a few other functions, instead of returning the data (or a reference) it just goes ahead and modified the $td ref that it got. Blah.

I patched this into the load_tb_json_data() function so as to avoid modifying Tinderbox itself. Fixing load_notes() so it doesn't need all this would be ideal but this is the bare minimum that it takes to get it going Right Now without affecting anything else, AFAICT.

Comment 9

[Mike Taylor [:bear]]

Comment on attachment 464700 [details] [diff] [review]
make note array and build indexes work

so ugly! but that's life in tbox - nice work-around for a gnarly global var issue

Comment 10

[Robert Helmer [:rhelmer]]

(In reply to comment #9)
> Comment on attachment 464700 [details] [diff] [review]
> make note array and build indexes work
> 
> so ugly! but that's life in tbox - nice work-around for a gnarly global var
> issue

Thanks, checked in (tbglobals.pl r1.76)

Comment 11

[Robert Helmer [:rhelmer]]

(In reply to comment #3)
> Comment on attachment 464483 [details] [diff] [review]
> rebuild build_table to remove recursive references
> 
> I'm loving this patch more and more :)
> 
> I agree that the next step, after a new round of profiling, would be to look
> into avoiding having to create the remaining hashes and try to get them into
> the json in a DRY manner

After looking at this in the context of TBPL (bug 5861230) I think it is worth doing this work now (folding the note_array and scrape structures into the individual hashes in the build_table.)

I think we could just get away with letting Tinderbox do it's thing (comment 8) and then manipulating the data structure before returning it from tb_load_json_data(). 

I'd rather avoid providing alternative or refactoring the underlying data load methods, if possible.

Comment 12

[Robert Helmer [:rhelmer]]

Attachment: a nice DRY data structure to make TBPL happier

Strip down the data structure, so we end up with something more like discussed in bug 586123.

It turns out that the build_table does a lot of useful correlation for us, so I removed the FIXME around notes and took advantage of this in the for-each that builds the JSON output.

Required:

{
    "builds": [
        {
            "buildname": "Linux mozilla-central debug test 2", 
            "buildstatus": "success", 
            "buildtime": "1281563615", 
            "endtime": "1281563660", 
            "errorparser": "unittest", 
            "ignored": 0, 
            "logfile": "1281563615.1281234504.1281239003.gz", 
            "scrape_enabled": 0, 
            "warnings_enabled": 0
        }, 
    ],
}

You may optionally see these one or more of these inside the builds hashes:

scrape
notes
warnings
binaryurl

Comment 13

[Mike Taylor [:bear]]

Comment on attachment 464994 [details] [diff] [review]
a nice DRY data structure to make TBPL happier

so glad we got feedback from the tbpl folks on this - nice work

Comment 14

[Robert Helmer [:rhelmer]]

Calling it done:

Checking in tbglobals.pl;
/cvsroot/mozilla/webtools/tinderbox/tbglobals.pl,v  <--  tbglobals.pl
new revision: 1.77; previous revision: 1.76
done

Comment 15

[Robert Helmer [:rhelmer]]

Attachment: scrape does not fiddle with $td, use local variable

Found this testing with a local TBPL. Everything else looks good so far!

Comment 16

[Robert Helmer [:rhelmer]]

Comment on attachment 465145 [details] [diff] [review]
scrape does not fiddle with $td, use local variable

Landed, thanks:

Checking in tbglobals.pl;
/cvsroot/mozilla/webtools/tinderbox/tbglobals.pl,v  <--  tbglobals.pl
new revision: 1.78; previous revision: 1.77
done

Comment 17

[Robert Helmer [:rhelmer]]

Attachment: perf enhancement - use native JSON not JSON:PP

I just realized that the latest round of changes makes using JSON:PP unnecessary (it supports "loose" and "single-quote", but we don't need that now that we are using a nice clean datastructure.)

This shows a very nice improvement on NYTProf; JSON is now below tbglobals and showbuilds.pl in the report. Overall time is about 1/3 (30ms->10ms) in my unscientific tests.

Comment 18

[Robert Helmer [:rhelmer]]

woot:

Checking in showbuilds.pl;
/cvsroot/mozilla/webtools/tinderbox/showbuilds.pl,v  <--  showbuilds.pl
new revision: 1.42; previous revision: 1.41
done