Spec: allow reordering contributions before embedding in a report

[alexmturner]

This should not affect functionality at all, but provides implementations with slightly more flexiblity.

---

Preview | Diff

[dmcardle]

Would it be functionally different to allow for reordering at the beginning of this procedure?

[alexmturner]

Yeah, it would be -- that would mean the decision about which contributions to truncate would become implementation-defined as well.

I'm also realizing that this breaks some of our WPTs technically. We could spec that the contributions are sorted, but that feels a bit unnecessary.

[dmcardle]

Gotcha.

As discussed offline, I'm not certain any callers actually rely on this behavior, e.g. by making more important contributions first. But I can also understand the desire to avoid breaking changes when not strictly necessary!

I'd seriously consider requiring sorting by bucket, if only because it simplifies testing. The cost of sorting maximum report contributions elements is O(1), after all. Looking towards a possible future where there are thousands of contributions, a clever implementation of contribution merging probably can't beat O(n log(n)) time, so sorting by bucket in O(n log(n)) wouldn't change the time complexity.

[alexmturner]

Yeah, agreed that we need a good way to test these.

I thought about it a little more and I was wondering if we might be able to do something a bit more flexible, but still short of implementing a full CBOR decoder. In particular, we could hardcode the expected starting and ending byte sequences (for the portions that aren't the contributions themselves). And then test that the rest is any ordering of the the correct contributions' byte sequences (which we also hardcode).

Maybe, I'll have a go and see if I can do this in a nice enough way before we decide on an approach for the spec.

[dmcardle]

It occurs to me that we could avoid the CBOR tricks if we require that the implementation-defined reordering is a pure function of the input. In other words, that replaying the same calls to contributeToHistogram() from another isolated context is guaranteed to produce an identical payload (modulo budgeting).

Were you aware that std::map iterates in ascending order of keys? (I was not.) I guess that means that the draft implementation of client-side contribution merging already emits a payload with a consistent, defined order. I would be inclined to require in the spec that contributions are sorted in increasing lexicographic order of <bucket, filtering_id>, since it seems it's neither an algorithmic burden nor does it add complexity to our implementation. WDYT and is there anything I'm misunderstanding here?

[alexmturner]

Yeah agreed that our current implementation does provide a consistent order; I'm just not sure I see the benefit to requiring that other implementations produce the same ordering. For example, if someone re-implemented this using a different map type (or language) they might need to add a sorting step, which seems unnecessary. (E.g. our previous implementation before this feature didn't sort them like this.)

So my instinct is to allow any ordering to be counted as conformant, especially if we still have a relatively simple way to test without restricting order.

[dmcardle]

Do you have thoughts on the weaker "pure function" property? It allows for implementation-defined ordering, but simplifies testing by guaranteeing the output is reproducible. (As a result, our web tests wouldn't need to parse CBOR.) Incidentally, our current non-merging implementation already emits the contributions in a reproducible order.

[alexmturner]

Hmm, I think if we specified that, we'd still need to do some testing of the payloads against order-less expectations (to ensure the buckets, values, format, etc, are correct). I think it's very likely that every reasonable implementation would be a pure function, but I think I'm still not seeing the benefit of requiring/validating that property. Could you expand a bit on how it might simplify testing?

[dmcardle]

If the order of contributions is reproducible, tests won't need to check whether the report's contributions are a permutation of the expected contributions; tests only need to compare the list of contributions to the expected value.

In particular, for Chrome's web tests, this would mean we don't need a CBOR parser just yet. Reproducible ordering enables us to continue hardcoding the expected CBOR bytes into the test.

Of course, implementation-agnostic tests will still need a CBOR parser, since we're permitting the contribution order to vary across implementations.

[alexmturner]

Ah thanks for the detail. I think the hope is that all of our web tests will eventually be non-internal WPTs. This is currently blocked by allowing a way to avoid the randomized delays -- however, the tests do still rely on debug mode as well, which we'll also need to find a work around for.