store: Add PerAccountStore.users data structure, as Map<int, User>

[chrisbobbe]

Seeing the convenient line flutter: initial fetch time: … in the logs, I decided to do some trials before and after these commits. 🙂 First after, then I went back and did before (i.e. main 9152167).

After:

3790ms
2842ms
2774ms
2781ms
2735ms
2851ms
2954ms
2926ms
2888ms
2998ms

Before:

2798ms
2551ms
2916ms
3017ms
2832ms
3032ms
2281ms
3400ms
2868ms
2387ms

[chrisbobbe]

I guess those numbers do include the time spent in the new from-JSON code, but not the time spent assembling PerAccountStore.users, hmm.

[chrisbobbe]

(And this is on CZO where as we know there are very many users.)

[gnprice]

Seeing the convenient line flutter: initial fetch time: … in the logs, I decided to do some trials before and after

Good idea!

Taking the medians of those trials (select them so they're on my clipboard, then xclip -o | sort -n | nl -ba, and look at the middle lines), it's 2850ms before and 2870ms after, suggesting a cost of about 20ms. That's milder than I'd hoped, really.

Getting some times that include the fromInitialSnapshot time would be good. Should be a matter of restarting that Stopwatch and then checking it again.

[gnprice]

suggesting a cost of about 20ms. That's milder than I'd hoped, really.

Though that's also a lot smaller than the variation within both sets of samples. So really what the data says is that the cost is smaller than this experiment could detect — which means smaller than something like 500ms.

[chrisbobbe]

Sure:

diff --git lib/model/store.dart lib/model/store.dart
index 051883a87..05c0f027f 100644
--- lib/model/store.dart
+++ lib/model/store.dart
@@ -319,11 +319,16 @@ class LivePerAccountStore extends PerAccountStore {
     // TODO log the time better
     if (kDebugMode) print("initial fetch time: ${t.inMilliseconds}ms");
 
+    stopwatch.start();
     final store = LivePerAccountStore.fromInitialSnapshot(
       account: account,
       connection: connection,
       initialSnapshot: initialSnapshot,
     );
+    final t2 = (stopwatch..stop()).elapsed;
+    // TODO log the time better
+    if (kDebugMode) print("time with LivePerAccountStore.fromInitialSnapshot: ${t2.inMilliseconds}ms");
+
     store.poll();
     return store;
   }

BEFORE:

flutter: initial fetch time: 2625ms
flutter: time with LivePerAccountStore.fromInitialSnapshot: 2625ms

flutter: initial fetch time: 2572ms
flutter: time with LivePerAccountStore.fromInitialSnapshot: 2572ms

flutter: initial fetch time: 2296ms
flutter: time with LivePerAccountStore.fromInitialSnapshot: 2296ms

flutter: initial fetch time: 2787ms
flutter: time with LivePerAccountStore.fromInitialSnapshot: 2787ms

flutter: initial fetch time: 2510ms
flutter: time with LivePerAccountStore.fromInitialSnapshot: 2510ms

AFTER:

flutter: initial fetch time: 2075ms
flutter: time with LivePerAccountStore.fromInitialSnapshot: 2081ms

flutter: initial fetch time: 2379ms
flutter: time with LivePerAccountStore.fromInitialSnapshot: 2387ms

flutter: initial fetch time: 2138ms
flutter: time with LivePerAccountStore.fromInitialSnapshot: 2144ms

flutter: initial fetch time: 2712ms
flutter: time with LivePerAccountStore.fromInitialSnapshot: 2717ms

flutter: initial fetch time: 2730ms
flutter: time with LivePerAccountStore.fromInitialSnapshot: 2739ms

So the time spent in LivePerAccountStore.fromInitialSnapshot is:

Before:

• <0.5ms
• <0.5ms
• <0.5ms
• <0.5ms
• <0.5ms

After:

• 6ms
• 8ms
• 6ms
• 5ms
• 9ms

[gnprice]

Thanks for building this! Comments below: some nits and an optimization.

From those timings, it looks like constructing the users map in LivePerAccountStore.fromInitialSnapshot is taking about 5-8 ms. That's pretty OK — especially given that the users are by far the biggest swath of the initial data in a large realm.

Specifically on CZO:

$ curl -sX POST --netrc-file ~/z/.netrc https://chat.zulip.org/api/v1/register >data.json
$ <data.json jq '.realm_users + .realm_non_active_users + .cross_realm_bots' -c | wc -c
11386100
$ <data.json jq . -c | wc -c
12480847

they're 11.4MB of the total 12.5MB, so 91% as measured by bytes of JSON.

I guess the other thing to improve in the initial-fetch timing would be to separate the time spent (a) in the network request and (b) decoding JSON from that spent (c) in fromJson. The former two are largely out of our control (short of server or API changes to reduce the amount of data sent and/or the time the server spends preparing the data), but the last one is a potential source of bottlenecks we introduce, and a potential site of optimizations.

No need to do that as part of this PR, though.

[gnprice]

Suggested change

	if (user.profileData == null) {
	return;
	}
	final profileData = (user.profileData ??= {});

Seems like if we do get an event saying to add some custom profile field data for a user that has null there, we may as well play along.

[chrisbobbe]

Trying to set user.profileData with {} is getting flagged by the analyzer because it's final:

'profileData' can't be used as a setter because it's final. (Documentation)

Try finding a different setter, or making 'profileData' non-final.

It also has an auto-fix button that says "Add 'late' modifier".

[gnprice]

Right, it'd go with making it non-final. This is a mutable class in general — the rest of the fields aren't final — so that's fine.

[chrisbobbe]

This is a mutable class in general — the rest of the fields aren't final — so that's fine.

Sure. I think it's reasonable to prefer making a property that's a Map final, so that all the code using the property will definitely be working with the same Map instance.

But here there's this performance thing that involves not making it final, and anyway we're not thinking of introducing multiple Map instances, we just want to introduce one instance in the case where we don't have one yet.

[gnprice]

A further tweak, which would represent an optimization for us, is that when we get {} for profile_data, we could represent it as null. This won't even really make any other code more complicated, because when consuming this we already have to handle the possibility that it's null. (It requires the simplification from my previous comment that uses ??= {} on getting an event to update one of these.)

That should be doable with a small readValue, I think.

The advantage of doing it that way is that we avoid allocating a huge number of LinkedHashMap data structures that we can do without. A hash table is inevitably going to involve some overhead (several words, at minimum), even when nothing's stored in it yet.

Here's some quick data from chat.zulip.org:

$ curl -sX POST --netrc-file ~/z/.netrc https://chat.zulip.org/api/v1/register --data-urlencode 'event_types=["realm_user"]' >data.json
$ <data.json jq '.realm_users[] | .profile_data' -c | sort | uniq -c | sort -n | tail -3
     47 {"13":{"value":"1"}}
    709 null
  23957 {}
$ <data.json jq '.realm_users | length'
25870

So of 25870 users in realm_users, 23957 of them (93%) have profile_data: {} and this optimization would apply. Doing the calculation more carefully including the other two lists, it's 24635/26822 ∼ 92%.

[chrisbobbe]

Would it be just as good to do this instead:

late final Map<int, ProfileFieldUserData> profileData = {};

?

From the doc:

When you mark a variable as late but initialize it at its declaration, then the initializer runs the first time the variable is used. This lazy initialization is handy in a couple of cases:

• The variable might not be needed, and initializing it is costly.
• […]

[gnprice]

That version would still mean that we'd end up allocating these if we ever look at the users' profile data. I'm not sure we necessarily would end up ever looking en masse at all or most users' profile data — but if we did, that'd be on the face of it a pretty innocent bit of code, and so this version of the field would be laying a performance trap that we could later walk into.

For that reason, better to have an explicit null. Then the consuming code will just have to write ?. for method calls on it and so on (and code that's updating it will need to write ??= {}); the type-checker means we can't forget to do those, and the performance characteristics will always be straightforward.

[chrisbobbe]

Thanks for the review! Revision pushed, with an approach to that performance thing that uses a language feature (late final with an initializer at the declaration) but that I'm not yet quite sure about.

[gnprice]

This is copying all the elements of the one Map into another Map. Better to just assign the maps:

  this.profileData = profileData;

[gnprice]

That version would still mean that we'd end up allocating these if we ever look at the users' profile data. I'm not sure we necessarily would end up ever looking en masse at all or most users' profile data — but if we did, that'd be on the face of it a pretty innocent bit of code, and so this version of the field would be laying a performance trap that we could later walk into.

For that reason, better to have an explicit null. Then the consuming code will just have to write ?. for method calls on it and so on (and code that's updating it will need to write ??= {}); the type-checker means we can't forget to do those, and the performance characteristics will always be straightforward.

[gnprice]

Thanks for the revision! Comments remaining only on the profileData performance/efficiency question. See above and also #84 (comment) further above.

[gnprice]

Merging the first commit:
901a99e lint: Suppress unnecessary_cast in generated files

because it's ready, and to avoid further rebases becoming spammy in the thread of the mentioned issue. We do in our style guide say:

You might find that a reference in a commit message causes GitHub to show a noisy list of backlinks in the issue thread, after you make revisions to the branch and push new versions of the commits. Please include the reference anyway, despite that UI bug in GitHub. We'll live with a little noise on the issue threads, and the references are extremely helpful when reading the Git log.

but when the reference is to someone else's GitHub issue — so we get the benefit in commit-message clarity, but they get the noise — then I'm more hesitant. (In this case, I'd probably have left the link in just the code comment, where it doesn't cause GitHub to generate such a backlink.)

[chrisbobbe]

to avoid further rebases becoming spammy in the thread of the mentioned issue.

Oh hmm, yeah I guess GitHub makes those backlinks not just for Fixes: #NNNN (and #NNNN and org/repo#NNNN) but also for full GitHub issue URLs of the form https://github.com/org/repo/issues/NNNN, as in this case. (The GitHub detail page for 901a99e makes it look like I wrote an org/repo#NNNN reference, but I actually wrote a URL.)

[chrisbobbe]

Thanks for the review! Revision pushed.

[gnprice]

Hmm, this doesn't seem right. See this in the doc:

Note: using this feature does not change any of the subsequent decoding logic for the field. For instance, if the field is of type DateTime we expect the function provided here to return a String.

That's a bit opaque; but more concretely, see the generated code (which this revision is missing, btw):

      profileData:
          (User._readProfileData(json, 'profile_data') as Map<String, dynamic>?)
              ?.map(
        (k, e) => MapEntry(int.parse(k),
            ProfileFieldUserData.fromJson(e as Map<String, dynamic>)),
      ),

That also reminds me of something I should have thought of earlier: we should have some unit tests for the nontrivial logic in our JSON decoding. No need to have it for all the boring fields — I don't think tests for those would do much good — but for basically the places we're using readValue.

[gnprice]

(Similar to how we have a test for MessageEvent.fromJson, in test/api/model/events_test.dart.)

[chrisbobbe]

Thanks for the review! Revision pushed, with those added tests.

[gnprice]

Thanks! Just small comments in the tests; please go ahead and merge after.

[gnprice]

A fun equivalent version:

Suggested change

	check(mkUser({'profile_data': {'1': {'value': 'foo'}}}).profileData)
	.isNotNull().entries.single
	..has((s) => s.key, 'key').equals(1)
	..has((s) => s.value, 'value').isA<ProfileFieldUserData>();
	check(mkUser({'profile_data': {'1': {'value': 'foo'}}}).profileData)
	.isNotNull().deepEquals({1: it()..isA<ProfileFieldUserData>()});

Docs:
https://pub.dev/documentation/checks/latest/checks/MapChecks/deepEquals.html

But further, the isA<ProfileFieldUserData>() is redundant with type soundness, because this is statically a Map<int, ProfileFieldUserData>?. So we can just write:

      check(mkUser({'profile_data': {'1': {'value': 'foo'}}}).profileData)
        .isNotNull().deepEquals({1: it()});

[gnprice]

nit:

Suggested change

	group('user', () {
	group('User', () {

[gnprice]

Is this comment meant as a TODO ("we could perhaps stand to refactor this test"), or as an explanation of why the assert is needed ("we could refactor how baseJson and mkUser work, and accidentally defeat this test")?

For the latter, I think we can add a test case that effectively covers that:

Suggested change

	assert(baseJson['is_system_bot'] == null); // could refactor
	check(mkUser({}).isSystemBot).equals(null);

As a bonus, that makes this test no longer care about the internals of how mkUser does its job, in the form of the relationship of the latter to baseJson.

[chrisbobbe]

or as an explanation of why the assert is needed ("we could refactor how baseJson and mkUser work, and accidentally defeat this test")?

Yeah, that's why I included the assert. But I didn't state that reason explicitly (oops); my comment "could refactor" was like a TODO to fix an awkwardness with the test setup: the test wants is_system_bot to be absent, but it has no way to ensure that, using the available interface (mkUser's specialJson arg). I think ideally it would have a way to do that. But I didn't say TODO because it won't really call out for a refactor until we change baseJson/mkUser in a way that trips up the assert, if we ever do.

As a bonus, that makes this test no longer care about the internals of how mkUser does its job, in the form of the relationship of the latter to baseJson.

Sure, that sounds good. The awkwardness I mentioned is still there, but at least the check gives the same guarantee as the assert did: it makes sure mkUser won't include isSystemBot unless asked by the caller. (I guess it now also tests a feature of the User implementation: it confirms that User.isSystemBot can be null, which is redundant with the static types.)

[gnprice]

Here's a more fun way to spell this:

Suggested change

	final json = Map.of(baseJson);
	json.addAll(specialJson);
	return User.fromJson(json);
	return User.fromJson({ ...baseJson, ...specialJson });

Should be pretty much equivalent even in performance terms, too. Here's the implementation of Map.of:

  factory LinkedHashMap.of(Map<K, V> other) =>
      LinkedHashMap<K, V>()..addAll(other);

so the current PR revision's version makes an empty map and calls addAll with the two input maps in succession, and I expect that's basically what the ... syntax expands to as well.

[gnprice]

nit:

Suggested change

	});
	User mkUser (Map<String, dynamic> specialJson) {
	});
	User mkUser (Map<String, dynamic> specialJson) {

[chrisbobbe]

Thanks for the review! Merging, with those changes.