Skip to content

Commit

Permalink
avatar: Remove remaining URL construction for initial load.
Browse files Browse the repository at this point in the history
This follows up on #4344 to address part of the remaining performance
regression from #4230.

We still construct URL objects in the `get` methods, lazily and
memoized; but never up front when simply taking in all the initial
data from the server, or the rehydrated data we've stored.  This
should save a further slice of time in the initial load.
  • Loading branch information
gnprice committed Dec 18, 2020
1 parent 96602b5 commit 99fdcbd
Show file tree
Hide file tree
Showing 2 changed files with 31 additions and 19 deletions.
8 changes: 4 additions & 4 deletions src/__tests__/lib/exampleData.js
Original file line number Diff line number Diff line change
Expand Up @@ -106,10 +106,10 @@ const userOrBotProperties = ({ name: _name }) => {
const name = _name ?? randString();
const capsName = name.substring(0, 1).toUpperCase() + name.substring(1);
return deepFreeze({
avatar_url: UploadedAvatarURL.validateAndConstructInstance({
realm: new URL('https://zulip.example.org'),
absoluteOrRelativeUrl: `/yo/avatar-${name}.png`,
}),
// Internally the UploadedAvatarURL mutates itself for memoization.
// That conflicts with the deepFreeze we do for tests; so construct it
// here with a full-blown URL object in the first place to prevent that.
avatar_url: new UploadedAvatarURL(new URL(`https://zulip.example.org/yo/avatar-${name}.png`)),

date_joined: `2014-04-${randInt(30)
.toString()
Expand Down
42 changes: 27 additions & 15 deletions src/utils/avatar.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import md5 from 'blueimp-md5';

import * as logging from './logging';
import { ensureUnreachable } from '../types';
import { isUrlAbsolute, isUrlPathAbsolute } from './url';

/**
* A way to get a standard avatar URL, or a sized one if available
Expand Down Expand Up @@ -311,38 +312,49 @@ export class UploadedAvatarURL extends AvatarURL {
/**
* Construct from raw server data, or throw an error.
*
* Expects a relative URL plus the realm for a local upload;
* otherwise, an absolute URL of the avatar on the S3 backend.
* Expects a relative, path-absolute URL plus the realm for a local
* upload; otherwise, an absolute URL of the avatar on the S3 backend.
*/
static validateAndConstructInstance(args: {|
realm: URL,
absoluteOrRelativeUrl: string,
|}): UploadedAvatarURL {
const { realm, absoluteOrRelativeUrl } = args;
// If `absoluteOrRelativeUrl` is absolute, the second argument
// is ignored.
return new UploadedAvatarURL(new URL(absoluteOrRelativeUrl, realm));

// Ideally, we'd say `new URL(absoluteOrRelativeUrl, realm)`.
// The URL constructor is too expensive; but we can do an exact
// equivalent, given our assumptions on the kinds of URL strings
// the server will send.
let absoluteUrl;
if (isUrlAbsolute(absoluteOrRelativeUrl)) {
// An absolute URL string. Ignore the base URL.
absoluteUrl = absoluteOrRelativeUrl;
} else if (isUrlPathAbsolute(absoluteOrRelativeUrl)) {
// A path-absolute URL string, like `/avatar/…`. We rely on our
// assumption that the realm URL equals its origin, modulo the latter
// having no trailing slash.
absoluteUrl = `${realm.origin}${absoluteOrRelativeUrl}`;
} else {
const msg = 'Unexpected form of avatar URL from server';
logging.error(msg, { avatarUrl: absoluteOrRelativeUrl });
throw new Error(msg);
}

return new UploadedAvatarURL(absoluteUrl);
}

/**
* 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();
Expand All @@ -356,7 +368,7 @@ export class UploadedAvatarURL 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);
}
Expand Down

0 comments on commit 99fdcbd

Please sign in to comment.