Skip to content

[IMPROVE] Message rendering time#14252

Merged
sampaiodiego merged 29 commits intodevelopfrom
message-template
May 21, 2019
Merged

[IMPROVE] Message rendering time#14252
sampaiodiego merged 29 commits intodevelopfrom
message-template

Conversation

@ggazzo
Copy link
Copy Markdown
Member

@ggazzo ggazzo commented Apr 25, 2019 •
edited
Loading

the objective here is reduce the computations and finds

@ggazzo ggazzo requested review from d-gubert and tassoevan April 25, 2019 21:07
@ggazzo ggazzo force-pushed the message-template branch from 28c06c4 to 774b4e4 Compare May 2, 2019 18:23
@ggazzo ggazzo requested a review from sampaiodiego May 6, 2019 19:32
@sampaiodiego sampaiodiego changed the title [IMPROVE] messages actions and some helpers [IMPROVE] Message rendering time May 21, 2019
@sampaiodiego sampaiodiego merged commit 7ad3f25 into develop May 21, 2019
wreiske added a commit to wreiske/Rocket.Chat that referenced this pull request May 27, 2019
@wreiske
Merge branch 'new-reports-ui' of https://github.com/wreiske/Rocket.Chat…
a120595 
… into new-reports-ui

* 'new-reports-ui' of https://github.com/wreiske/Rocket.Chat: (97 commits)
  LingoHub Update 🚀 (RocketChat#14643)
  [FIX] Role name spacing on Permissions page (RocketChat#14625)
  [FIX] Avatar images on old Livechat client (RocketChat#14590)
  [FIX] Inject code at the end of <head> tag (RocketChat#14623)
  [FIX] Mailer breaking if user doesn't have an email address (RocketChat#14614)
  Ci improvements (RocketChat#14600)
  [FIX] E2E messages not decrypting in message threads (RocketChat#14580)
  Fix: emoji render performance for alias (RocketChat#14593)
  [FIX] Send replyTo for livechat offline messages (RocketChat#14568)
  Federation i18n message changes (RocketChat#14595)
  [REGRESSION] Fix Slack bridge channel owner on channel creation (RocketChat#14565)
  Fix thumbs up emoji shortname (RocketChat#14581)
  Fix broken logo url in app.json (RocketChat#14572)
  Add digitalocean button to readme (RocketChat#14583)
  Fix: Add emoji shortnames to emoji's list (RocketChat#14576)
  [IMPROVE] Message rendering time (RocketChat#14252)
  [IMPROVE] Change user presence events to Meteor Streams (RocketChat#14488)
  Removed unnecessary DDP unblocks (RocketChat#13641)
  [FIX] Downloading files when running in sub directory (RocketChat#14485)
  [FIX] Broken layout when sidebar is open on IE/Edge (RocketChat#14567)
  ...
@sampaiodiego sampaiodiego mentioned this pull request May 28, 2019
Merged
@wreiske
Copy link
Copy Markdown
Contributor

wreiske commented May 31, 2019

Curious if there are any before/after benchmarks for this?

@ggazzo
Copy link
Copy Markdown
Member Author

ggazzo commented May 31, 2019

@wreiske, actually I dont have graphs yet (I'm working on it now)...
I made that changes because I want to remove the nrr package and in the past, it was almost impossible render messages using blaze, now you can run localStorage.set('useNrr', 'false') and test, the speed should be almost the same...

@wreiske
Copy link
Copy Markdown
Contributor

wreiske commented May 31, 2019 •
edited
Loading

It would be interesting to do benchmarks in the unit tests and save them somewhere. That way, if a test that normally takes 300ms to complete now takes 1s to complete, a red flag will be thrown. I was just curious about the impact/improvement in ms time for the changes if you had done any testing on that.

@ggazzo
Copy link
Copy Markdown
Member Author

ggazzo commented May 31, 2019

It would be interesting to do benchmarks in the unit tests and save them somewhere. That way, if a test that normally takes 300ms to complete now takes 1s to complete, a red flag will be thrown.

yep, actually, for now, we have too many side effects so its quite hard to make a unit test, but part of the rewrite was, in fact, remove some side effects

@engelgabriel engelgabriel deleted the message-template branch April 2, 2020 05:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sampaiodiego sampaiodiego sampaiodiego approved these changes

@tassoevan tassoevan Awaiting requested review from tassoevan

@d-gubert d-gubert Awaiting requested review from d-gubert

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ggazzo @wreiske @sampaiodiego