Refactor CTFailures._processFailuresForRangeGroup.

Refactor CTFailures._processFailuresForRangeGroup.

Avoid duplicate array traversals, and clarify what the code is trying to do.

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=181593

[Jeffrey Yasskin, 2014-09-05 00:32:00]

jyasskin@chromium.org changed reviewers:
+ ojan@chromium.org

[Jeffrey Yasskin, 2014-09-05 00:32:00]

On my machine, this cuts the time to update from the giant alerts file from
~2.3s to ~.54s. I didn't include the performance test in the patch because I'm
not sure how to write a performance test that isn't flaky.

[ojan, 2014-09-05 00:56:42]

lgtm. <3 this patch.

On 2014/09/05 at 00:32:00, jyasskin wrote:
> On my machine, this cuts the time to update from the giant alerts file from
~2.3s to ~.54s. I didn't include the performance test in the patch because I'm
not sure how to write a performance test that isn't flaky.

Even a manual perf test is useful to check in IMO.

https://codereview.chromium.org/538293002/diff/1/Tools/GardeningServer/model/...
File Tools/GardeningServer/model/ct-failures.html (right):

https://codereview.chromium.org/538293002/diff/1/Tools/GardeningServer/model/...
Tools/GardeningServer/model/ct-failures.html:84:
CTFailures.prototype._mungeAlerts = function(alerts) {
Mind writing basic unit tests for this function? I'm hoping to head in the
direction of 100% test coverage for at least the JS code in sheriff-o-matic.

https://codereview.chromium.org/538293002/diff/1/Tools/GardeningServer/model/...
Tools/GardeningServer/model/ct-failures.html:96: // The server has been fixed.
<3

https://codereview.chromium.org/538293002/diff/1/Tools/GardeningServer/model/...
Tools/GardeningServer/model/ct-failures.html:123: var alertsByKey = {}
Add a FIXME to change builder_alerts to just have the alerts be a map instead of
a list? Other than builder_alerts itself, this is the only consumer of this
field.

https://codereview.chromium.org/538293002/diff/1/Tools/GardeningServer/model/...
Tools/GardeningServer/model/ct-failures.html:154: function
groupFailuresByTreeAndReason(failures, annotations) {
Ditto. You'll need to make this a function on CTFailures to expose it to the
test, but I think that's OK.

[Jeffrey Yasskin, 2014-09-09 00:57:43]

Patchset #2 (id:20001) has been deleted

[Jeffrey Yasskin, 2014-09-09 00:59:19]

Patchset #2 (id:40001) has been deleted

[Jeffrey Yasskin, 2014-09-09 01:00:33]

This change takes model/test/manual/ct-failures-perf.html from ~3000ms to
~400ms.

https://codereview.chromium.org/538293002/diff/1/Tools/GardeningServer/model/...
File Tools/GardeningServer/model/ct-failures.html (right):

https://codereview.chromium.org/538293002/diff/1/Tools/GardeningServer/model/...
Tools/GardeningServer/model/ct-failures.html:84:
CTFailures.prototype._mungeAlerts = function(alerts) {
On 2014/09/05 00:56:42, ojan-only-code-yellow-reviews wrote:
> Mind writing basic unit tests for this function? I'm hoping to head in the
> direction of 100% test coverage for at least the JS code in sheriff-o-matic.

Done.

https://codereview.chromium.org/538293002/diff/1/Tools/GardeningServer/model/...
Tools/GardeningServer/model/ct-failures.html:123: var alertsByKey = {}
On 2014/09/05 00:56:42, ojan-only-code-yellow-reviews wrote:
> Add a FIXME to change builder_alerts to just have the alerts be a map instead
of
> a list? Other than builder_alerts itself, this is the only consumer of this
> field.

Done.

https://codereview.chromium.org/538293002/diff/1/Tools/GardeningServer/model/...
Tools/GardeningServer/model/ct-failures.html:154: function
groupFailuresByTreeAndReason(failures, annotations) {
To do the refactoring that Elliott asked for in
https://codereview.chromium.org/526633002/, I think we'd need to push the loop
over data.range_groups down through _processFailuresForRangeGroup into this
function, and have this function return a map
"tree->group->reasonKey->builder->info".

I'm not planning to pursue this further.

https://codereview.chromium.org/538293002/diff/1/Tools/GardeningServer/model/...
Tools/GardeningServer/model/ct-failures.html:154: function
groupFailuresByTreeAndReason(failures, annotations) {
On 2014/09/05 00:56:42, ojan-only-code-yellow-reviews wrote:
> Ditto. You'll need to make this a function on CTFailures to expose it to the
> test, but I think that's OK.

Tested.

[ojan, 2014-09-09 01:03:55]

lgtm

[Jeffrey Yasskin, 2014-09-09 01:05:55]

The CQ bit was checked by jyasskin@chromium.org

[commit-bot: I haz the power, 2014-09-09 01:06:13]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jyasskin@chromium.org/538293002/60001

[commit-bot: I haz the power, 2014-09-09 02:09:38]

Committed patchset #2 (id:60001) as 181593