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

Jump to bottom

Address a performance regression from #4230 by deferring new URL computations. #4344

Merged
merged 4 commits into from
Dec 16, 2020
Merged

Address a performance regression from #4230 by deferring new URL computations. #4344

merged 4 commits into from
Dec 16, 2020
+79 −33

Commits on Dec 16, 2020

  1. 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
    @chrisbobbe @gnprice
    chrisbobbe authored and gnprice committed Dec 16, 2020
    Configuration menu
    Copy the full SHA
    ba87344 View commit details
    Browse the repository at this point in the history

  2. 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
    @chrisbobbe @gnprice
    chrisbobbe authored and gnprice committed Dec 16, 2020
    Configuration menu
    Copy the full SHA
    b5f1670 View commit details
    Browse the repository at this point in the history

  3. 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.
    @chrisbobbe @gnprice
    chrisbobbe authored and gnprice committed Dec 16, 2020
    Configuration menu
    Copy the full SHA
    78ac0a2 View commit details
    Browse the repository at this point in the history

  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.
    @chrisbobbe @gnprice
    chrisbobbe authored and gnprice committed Dec 16, 2020
    Configuration menu
    Copy the full SHA
    1f0e42c View commit details
    Browse the repository at this point in the history