Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Spec: allow reordering contributions before embedding in a report #144

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Spec: allow reordering contributions before embedding in a report #144

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions spec.bs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -672,6 +672,8 @@ null |timeout|:
1. [=set/Append=] |mergedContributions|[|n|] to
|truncatedContributions|.
1. Otherwise, set |truncatedContributions| to |mergedContributions|.
1. Optionally, reorder the items of |truncatedContributions| in an
[=implementation-defined=] way.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

1. Let |contributionSum| be 0.
1. [=set/iterate|For each=] |contribution| of |truncatedContributions|:
1. [=Assert=]: |contribution|["{{PAHistogramContribution/value}}"] is
Expand Down
Loading