From ba8734403609606bbbc8557860373e4947df947e Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Tue, 15 Dec 2020 18:41:52 -0500 Subject: [PATCH 1/4] avatar: Remove URL computation in FallbackAvatarURL's pseudo-constructor. Specifically, in its static `validateAndConstructInstance` method. In working on the recently merged #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 #4230 was merged [1], though, we found that `api.registerForEvents` started to take a lot longer after #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 | 28 ++++++++++++++++------------ 1 file changed, 16 insertions(+), 12 deletions(-) diff --git a/src/utils/avatar.js b/src/utils/avatar.js index f8d5e7fe4d1..ff0f34bd50b 100644 --- a/src/utils/avatar.js +++ b/src/utils/avatar.js @@ -195,18 +195,29 @@ 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; @@ -214,13 +225,6 @@ export class FallbackAvatarURL extends AvatarURL { * 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); } From b5f16705f739c13e2dd42a9b30afeb08212a914e Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Tue, 15 Dec 2020 19:46:31 -0500 Subject: [PATCH 2/4] avatar: Remove an unnecessary `new URL` call in `fromUserOrBotData`. Just as in the previous commit, we're reducing the computation it takes to process avatar URLs at the edge, just as they're received in bulk (of potentially thousands, on a large realm like CZO) from the server. See Greg's comment for profiling data suggesting this change is indicated [1]. [1] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/user_avatar_url_field_optional/near/1081272 --- src/utils/avatar.js | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/utils/avatar.js b/src/utils/avatar.js index ff0f34bd50b..6b512d6ebef 100644 --- a/src/utils/avatar.js +++ b/src/utils/avatar.js @@ -3,7 +3,6 @@ /* eslint-disable no-use-before-define */ import md5 from 'blueimp-md5'; -import { tryParseUrl } from './url'; import * as logging from './logging'; import { ensureUnreachable } from '../types'; @@ -53,7 +52,16 @@ export class AvatarURL { // which to generate the correct hash; see // https://github.com/zulip/zulip/issues/15287. Implemented at // `do_events_register` in zerver/lib/events.py on the server.) - if (tryParseUrl(rawAvatarUrl)?.origin === GravatarURL.ORIGIN) { + if ( + rawAvatarUrl.startsWith( + // Best not to use an expensive `new URL` call, when the + // following is equivalent (assuming `rawAvatarUrl` is also + // valid through its /pathname, ?query=params, and + // #fragment). The trailing slash ensures that we've + // examined the full origin in `rawAvatarUrl`. + `${GravatarURL.ORIGIN}/`, + ) + ) { const hashMatch = /[0-9a-fA-F]{32}$/.exec(new URL(rawAvatarUrl).pathname); if (hashMatch === null) { const msg = 'Unexpected Gravatar URL shape from server.'; From 78ac0a2b589f7bf4229a3bf16cb98339a7f25eea Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Tue, 15 Dec 2020 20:47:03 -0500 Subject: [PATCH 3/4] avatar: Remove a few URL computations for Gravatar URLs. Just as in previous commits, we're reducing the computation it takes to process avatar URLs at the edge, just as they're received in bulk (potentially thousands, on a large realm like CZO) from the server. --- src/utils/__tests__/avatar-test.js | 18 ++++++++--- src/utils/avatar.js | 51 +++++++++++++++++++++--------- 2 files changed, 50 insertions(+), 19 deletions(-) diff --git a/src/utils/__tests__/avatar-test.js b/src/utils/__tests__/avatar-test.js index 18da866abba..482d72dbed7 100644 --- a/src/utils/__tests__/avatar-test.js +++ b/src/utils/__tests__/avatar-test.js @@ -71,12 +71,22 @@ describe('GravatarURL', () => { }); }); - test('uses hash from server, if provided', () => { + test('uses URL from server, if provided', () => { const email = 'user13313@chat.zulip.org'; - const hash = md5('cbobbe@zulip.com'); - const instance = GravatarURL.validateAndConstructInstance({ email, hash }); + const urlFromServer = + 'https://secure.gravatar.com/avatar/de6685f1d3eb74439c1dcda84f92543e?d=identicon&version=1'; + const instance = GravatarURL.validateAndConstructInstance({ + email, + urlFromServer, + }); SIZES_TO_TEST.forEach(size => { - expect(instance.get(size).toString()).toContain(hash); + const clonedUrlOfSize = new URL(instance.get(size).toString()); + const clonedUrlFromServer = new URL(urlFromServer); + clonedUrlFromServer.searchParams.delete('s'); + clonedUrlOfSize.searchParams.delete('s'); + // `urlFromServer` should equal the result for this size, modulo + // the actual size param. + expect(clonedUrlOfSize.toString()).toEqual(clonedUrlFromServer.toString()); }); }); diff --git a/src/utils/avatar.js b/src/utils/avatar.js index 6b512d6ebef..251148e13cd 100644 --- a/src/utils/avatar.js +++ b/src/utils/avatar.js @@ -62,13 +62,7 @@ export class AvatarURL { `${GravatarURL.ORIGIN}/`, ) ) { - const hashMatch = /[0-9a-fA-F]{32}$/.exec(new URL(rawAvatarUrl).pathname); - if (hashMatch === null) { - const msg = 'Unexpected Gravatar URL shape from server.'; - logging.error(msg, { value: rawAvatarUrl }); - throw new Error(msg); - } - return GravatarURL.validateAndConstructInstance({ email, hash: hashMatch[0] }); + return GravatarURL.validateAndConstructInstance({ email, urlFromServer: rawAvatarUrl }); } // Otherwise, it's a realm-uploaded avatar, either absolute or @@ -117,16 +111,43 @@ export class GravatarURL extends AvatarURL { /** * Construct from raw server data, or throw an error. * - * Pass the hash if the server provides it, to avoid computing it - * unnecessarily. + * Pass the Gravatar URL string from the server, if we've got it, to + * avoid doing an expensive `new URL` call. */ - static validateAndConstructInstance(args: {| email: string, hash?: string |}): GravatarURL { - const { email, hash = md5(email.toLowerCase()) } = args; - - const standardSizeUrl = new URL(`/avatar/${hash}`, GravatarURL.ORIGIN); - standardSizeUrl.searchParams.set('d', 'identicon'); + // 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. + // + // In the past, we've been more conservative about validating a URL + // string that comes to us from the server at this point: we'd parse + // it with the URL constructor, grab the Gravatar hash from the + // result, and construct another URL with that hash and exactly the + // things we wanted it to have (like `d=identicon`). + // + // With some loss of validation, we've removed those two `new URL` + // calls in the path where the server provides a Gravatar URL + // string. Still, we should be able to trust that the server gives + // us properly formatted URLs; if it didn't, it seems like the kind + // of bug that would be fixed quickly. + static validateAndConstructInstance(args: {| + email: string, + urlFromServer?: string, + |}): GravatarURL { + const { email, urlFromServer } = args; - return new GravatarURL(standardSizeUrl); + if (urlFromServer !== undefined) { + // This may not be *quite* the URL we would have generated + // ourselves. In the wild, I've seen one with a `version=1`, for + // example. But we trust the server to give us one that works, + // anyway -- and perhaps any extra things we get will be a good + // bonus. + return new GravatarURL(urlFromServer); + } else { + const standardSizeUrl = new URL(`/avatar/${md5(email.toLowerCase())}`, GravatarURL.ORIGIN); + standardSizeUrl.searchParams.set('d', 'identicon'); + return new GravatarURL(standardSizeUrl); + } } static ORIGIN = 'https://secure.gravatar.com'; From 1f0e42ce578b6114262e0069e0c8bc8260e970f7 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Tue, 15 Dec 2020 20:52:53 -0500 Subject: [PATCH 4/4] avatar: Remove URL computation in GravatarURL's pseudo-constructor. Just as in previous commits, we're reducing the computation it takes to process avatar URLs at the edge, just as they're received in bulk (potentially thousands, on a large realm like CZO) from the server. --- src/utils/avatar.js | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/utils/avatar.js b/src/utils/avatar.js index 251148e13cd..0cb184f98aa 100644 --- a/src/utils/avatar.js +++ b/src/utils/avatar.js @@ -144,9 +144,12 @@ export class GravatarURL extends AvatarURL { // bonus. return new GravatarURL(urlFromServer); } else { - const standardSizeUrl = new URL(`/avatar/${md5(email.toLowerCase())}`, GravatarURL.ORIGIN); - standardSizeUrl.searchParams.set('d', 'identicon'); - return new GravatarURL(standardSizeUrl); + return new GravatarURL( + // 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`. + `${GravatarURL.ORIGIN}/avatar/${md5(email.toLowerCase())}?d=identicon`, + ); } }