avatar: Remove URL computation in FallbackAvatarURL's pseudo-construc…

…tor.

Specifically, in its static `validateAndConstructInstance` method.

In working on the recently merged zulip#4230, we found that calling
`new URL` with ordinary input is quite expensive, and so we arranged
to not call it when rehydrating. We left the call in
`validateAndConstructInstance`, though, thinking we couldn't
possibly remove it and still validate the raw server data properly
at the edge.

When we did some performance benchmarking after zulip#4230 was
merged [1], though, we found that `api.registerForEvents` started to
take a lot longer after zulip#4230 than before it. Profiling [2]
suggested that we could help speed things up if we could just stop
calling `new URL` in `validateAndConstructInstance`.

Looking closer, thankfully, it's possible and reasonably easy. We
would normally avoid string concatenation for URLs entirely, but
this is a case where we can do it in a fairly principled way (see
the implementation comment in the diff).

[1] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/user_avatar_url_field_optional/near/1080919
[2] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/user_avatar_url_field_optional/near/1081240
[3] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/user_avatar_url_field_optional/near/1081253

src/utils/avatar.js

@@ -195,32 +195,36 @@ export class FallbackAvatarURL extends AvatarURL {
   }
 
   /**
-   * Construct from raw server data, or throw an error.
+   * Construct from raw server data (the user ID), or throw an error.
+   *
+   * The `realm` must be already validated, e.g., by coming from the
+   * Redux state.
    */
+  // We should avoid doing unnecessary `new URL` calls here. They are
+  // very expensive, and their use in these pseudo-constructors (which
+  // process data at the edge, just as it's received from the server)
+  // used to slow down `api.registerForEvents` quite a lot.
   static validateAndConstructInstance(args: {| realm: URL, userId: number |}): FallbackAvatarURL {
     const { realm, userId } = args;
-    return new FallbackAvatarURL(new URL(`/avatar/${userId.toString()}`, realm));
+    // Thankfully, this string concatenation is quite safe: we know
+    // enough about our inputs here to compose a properly formatted
+    // URL with them, without using `new URL`. (In particular,
+    // `realm.origin` doesn't have a trailing slash.)
+    return new FallbackAvatarURL(`${realm.origin}/avatar/${userId.toString()}`);
   }
 
   /**
    * Standard URL from which to generate others. PRIVATE.
    *
-   * May be a string if the instance was constructed at rehydrate
-   * time, when URL validation is unnecessary.
+   * May start out as a string, and will be converted to a URL object
+   * in the first `.get()` call.
    */
   _standardUrl: string | URL;
 
   /**
    * PRIVATE: Make an instance from already-validated data.
    *
    * Not part of the public interface; use the static methods instead.
-   *
-   * It's private because we need a path to constructing an instance
-   * without constructing URL objects, which takes more time than is
-   * acceptable when we can avoid it, e.g., during rehydration.
-   * Constructing URL objects is a necessary part of validating data
-   * from the server, but we only need to validate the data once, when
-   * it's first received.
    */
   constructor(standardUrl: string | URL) {
     super();
@@ -238,7 +242,7 @@ export class FallbackAvatarURL extends AvatarURL {
    */
   get(sizePhysicalPx: number): URL {
     // `this._standardUrl` may have begun its life as a string, to
-    // avoid computing a URL object during rehydration
+    // avoid expensively calling the URL constructor
     if (typeof this._standardUrl === 'string') {
       this._standardUrl = new URL(this._standardUrl);
     }