Skip to content

Do not include highlighted_diff_email css for each note for new review email

Nikola Milojevic requested to merge 355027-optimize-new-reivew-mail into master

What does this MR do and why?

In #355027 (closed), we discovered that Premailer::Adapter::Nokogiri#to_inline_css uses 25.93 seconds (51%) and it allocates a lot of memory: #355027 (comment 880321527)

After debugging Premailer::Adapter::Nokogiri#to_inline_css, I found that the biggest issue is that we are including highlighted_diff_email.css for each note/comment.

This means that in the case when a review contains 65 diff_discussion notes/comments, we will include mentioned CSS 65 times.

This will cause the Premailer Nokogiri adapter to search the whole HTML Dom object for each selector from that included css - (there are 81 of them). This action will then repeat 65 times, allocating 65x time more memory and time.

Premailer::Adapter::Nokogiri#to_inline_css took total of 26.55 seconds to execute for 65 diff discussions.

Click to expand
selector total time count
.diff-line-num 0.9784890040755272 65
.code > tr 0.8081630114465952 65
.diff-line-num.new 0.628456000238657 65
.line_content 0.5463110134005547 65
.diff-line-num.old 0.443707000464201 65
.line_content.new 0.4226210117340088 65
.code 0.42036799155175686 65
.n .37347900308668613 65
.line_content.old 0.3633930031210184 65
.o 0.34669999964535236 65
.line_content.new > .line > span.idiff 0.34396199882030487 65
.line_content.new > .line > span > span.idiff 0.33864900656044483 65
.k 0.33754899725317955 65
.line_content.old > .line > span > span.idiff 0.3354520071297884 65
.line_content.old > .line > span.idiff 0.3346880041062832 65
.line_content.match 0.32732499577105045 65
.... .... ....

This MR makes sure to include highlighted_diff_email css only once.

Results with fix

Screenshots or screen recordings

These are strongly recommended to assist reviewers and reduce the time to merge your change.

How to set up and validate locally highlighted_diff_email

Numbered steps to set up and validate the change are strongly suggested.

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

Related to #355027 (closed)

Edited by Nikola Milojevic

Merge request reports