Page MenuHomePhabricator

[services] Backup - Convert attachment holders from vector to string
ClosedPublic

Authored by karol on May 19 2022, 3:01 AM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Jun 11, 6:42 AM
Unknown Object (File)
Mon, Jun 3, 9:39 AM
Unknown Object (File)
Wed, May 29, 10:58 AM
Unknown Object (File)
Wed, May 29, 10:58 AM
Unknown Object (File)
Wed, May 29, 10:58 AM
Unknown Object (File)
Wed, May 29, 10:58 AM
Unknown Object (File)
Wed, May 29, 10:52 AM
Unknown Object (File)
Sat, May 18, 1:33 PM

Details

Summary

Depends on D4076

In https://linear.app/comm/issue/ENG-1029/handle-attachments we agreed to keep attachment lists in strings delimited with a single character.

Test Plan

Services still build

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

karol edited the test plan for this revision. (Show Details)
karol added a reviewer: tomek.
tomek requested changes to this revision.May 23 2022, 5:08 AM
tomek added inline comments.
services/backup/src/DatabaseEntities/BackupItem.cpp
115–118 ↗(On Diff #12921)

I'm not sure if this is efficient. If we want to add N holders, we need to create a new string on every addition, effectively having O(N^2) complexity. What do you think about having vector which is transformed to string only when saving to db?

This revision now requires changes to proceed.May 23 2022, 5:08 AM
services/backup/src/DatabaseEntities/BackupItem.cpp
115–118 ↗(On Diff #12921)

I'm not sure, on the other hand, if it's a good idea to do this with the vector. With vector, we're going to create a new string every time. Plus, we're also going to have to convert between string and vector of string twice (we get a string from the database and we have to write a string to the database). Instead, I'd try to make this append more efficient.

As I looked around I found that:

std::string operator+ allocates a new string and copies the two operand strings every time. repeat many times and it gets expensive, O(n).
std::string append and operator+= on the other hand, bump the capacity by 50% every time the string needs to grow. Which reduces the number of memory allocations and copy operations significantly, O(log n).

So I think we'll be alright if we do:

this->attachmentHolders += attachmentHolder;
this->attachmentHolders += ATTACHMENT_DELIMITER;
karol edited the summary of this revision. (Show Details)

make appendAttachmentHolder more efficient

tomek added inline comments.
services/backup/src/DatabaseEntities/BackupItem.cpp
115–118 ↗(On Diff #12921)

Ok, that makes sense!

One thing: we can make this in one line this->attachmentHolders += (attachmentHolder + ATTACHMENT_DELIMITER).

This revision is now accepted and ready to land.May 24 2022, 3:15 AM
services/backup/src/DatabaseEntities/BackupItem.cpp
115–118 ↗(On Diff #12921)

But in this one-liner, you're using the + operator. So, having this

this->attachmentHolders += attachmentHolder;
this->attachmentHolders += ATTACHMENT_DELIMITER;

we have 3 strings allocated:

  • this->attachmentHolders
  • attachmentHolder
  • ATTACHMENT_DELIMITER

Having one-liner:

this->attachmentHolders += (attachmentHolder + ATTACHMENT_DELIMITER)

We're going to have 4 strings allocated:

  • this->attachmentHolders
  • attachmentHolder
  • ATTACHMENT_DELIMITER
  • result of + operation (attachmentHolder + ATTACHMENT_DELIMITER)

I'm going to go with the tow-liner, if you feel like we should change this, please, let me know but keep in mind what's above.