Page MenuHomePhabricator
Differential D3487

[services] Backup - Fix copying big data chunk
ClosedPublic
Actions

Authored by • karol on Mar 22 2022, 1:23 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Apr 6, 9:14 AM
Unknown Object (File)
Sun, Apr 6, 9:14 AM
Unknown Object (File)
Sun, Apr 6, 9:14 AM
Unknown Object (File)
Sun, Apr 6, 9:14 AM
Unknown Object (File)
Sun, Apr 6, 9:14 AM
Unknown Object (File)
Sun, Apr 6, 9:14 AM
Unknown Object (File)
Sun, Apr 6, 9:14 AM
Unknown Object (File)
Sun, Apr 6, 9:14 AM
View All Files
Subscribers
• adrian
ashoat
atul
• benschac
tomek

Details

Reviewers
• max
varun
tomek
• jim
ashoat
marcin
Commits
rCOMM2ca4b0babd9b: [services] Backup - Fix copying big data chunk
Summary

Depends on D3471

Once we receive data as a gRPC field, we don't want to copy it around, especially when it's a data chunk and the size of it may reach up to 4-5MB. Instead, it's always better to have just one place in memory for it and pass the address around.

Test Plan
cd services
yarn run-backup-service

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

• karol created this revision.Mar 22 2022, 1:23 AM
Herald added subscribers: • benschac, atul, • adrian and 2 others. · View Herald TranscriptMar 22 2022, 1:23 AM
• karol retitled this revision from [services] Backup - fix copying big data chunk to [draft] [services] Backup - fix copying big data chunk.Mar 22 2022, 1:25 AM
• karol edited the summary of this revision. (Show Details)
• karol edited the test plan for this revision. (Show Details)
• karol added reviewers: • max, varun, tomek, • jim.
• karol added a parent revision: D3471: [services] Blob - Update service implementation.
Harbormaster completed remote builds in B7521: Diff 10586.Mar 22 2022, 1:28 AM
• karol requested review of this revision.Mar 22 2022, 1:28 AM
• karol added a child revision: D3488: [services] Backup - make blob client instances separate for every thread.Mar 22 2022, 2:53 AM
• karol planned changes to this revision.Mar 29 2022, 1:39 AM
• karol updated this revision to Diff 10755.Mar 29 2022, 1:40 AM

rebase

Harbormaster completed remote builds in B7660: Diff 10755.Mar 29 2022, 1:49 AM
• karol planned changes to this revision.Mar 29 2022, 2:58 AM
• karol retitled this revision from [draft] [services] Backup - fix copying big data chunk to [services] Backup - Fix copying big data chunk.
• karol updated this revision to Diff 10761.Mar 29 2022, 4:37 AM

rebase

Harbormaster completed remote builds in B7665: Diff 10761.Mar 29 2022, 4:46 AM
tomek requested changes to this revision.Mar 30 2022, 4:36 AM
tomek added inline comments.
services/backup/docker-server/contents/server/src/Reactors/client/blob/BlobPutClientReactor.h
53–54 ↗(On Diff #10761)

Are you sure this is a good idea? We take this by reference and then move - does it destroy the original data?

This revision now requires changes to proceed.Mar 30 2022, 4:36 AM
• karol requested review of this revision.Apr 1 2022, 4:45 AM
• karol added inline comments.
services/backup/docker-server/contents/server/src/Reactors/client/blob/BlobPutClientReactor.h
53–54 ↗(On Diff #10761)

Yes. I know this looks dangerous, we should use this being aware that the original data will no longer be accessible. I thought it would crash but I ran it and just had an empty string after something like this:

string *str = response.datachunk();
scheduleSendingDataChunk(str);
court << str->size << endl;

Anyway, even though it didn't crash for me, I think we should assume a crash will occur.
For now, I don't see a better way of doing this. I think if we use a unique pointer, we will have the same situation.
The goal here is to prevent copying a big amount of data around.
Even if someone mistakenly uses this value after a function call, they will quickly see a problem there - for sure we should cover this in the tests.
We can add an informative comment there BTW.

What you think?

ashoat requested changes to this revision.Apr 1 2022, 12:09 PM
ashoat added inline comments.
services/backup/docker-server/contents/server/src/Reactors/client/blob/BlobPutClientReactor.h
53–54 ↗(On Diff #10761)

Why would we have the same issue with a unique_ptr? This is basically exactly what unique_ptr is for. When you pass in a unique_ptr to a function, you are saying "this thing is now yours, I no longer can access it" and the compiler will enforce that.

When you pass a reference like you are doing, the caller has no reason to expect that the data will destroyed. When you pass a unique_ptr, the caller knows they cannot access this data ever again.

This revision now requires changes to proceed.Apr 1 2022, 12:09 PM
• karol added inline comments.Apr 4 2022, 1:00 AM
services/backup/docker-server/contents/server/src/Reactors/client/blob/BlobPutClientReactor.h
53–54 ↗(On Diff #10761)

I mean, yes, you're right about the idea behind the unique pointer but you don't seem to be right that the compiler's going to be helpful here. Something like this crashes:

#include <iostream>
#include <memory>

using namespace std;

void f(unique_ptr<string> strp) {
  cout << "f " << *strp << endl;
}

int main()
{
  unique_ptr<string> str = make_unique<string>("asdasd");
  f(move(str));
  cout << "is str nullptr? " << (str == nullptr) << endl; // true
  cout << "out " << *str << endl; // crash

  return 0;
}

So, after a move, unique_ptr holds a nullptr. We can perform a check against the nullptr but if we don't and just try to access the pointer right away and it is null, then it's going to crash.

But I agree that passing a unique pointer here is good because it indicates that the data's not going to be available anymore.

• karol updated this revision to Diff 10982.Apr 4 2022, 3:00 AM

use unique_ptr instead of raw pointers

Harbormaster failed remote builds in B7815: Diff 10982!Apr 4 2022, 3:04 AM
tomek accepted this revision.Apr 4 2022, 4:05 AM
tomek added a comment.Apr 4 2022, 4:07 AM

It looks like there's a build error caused by some other diff

• karol added inline comments.Apr 4 2022, 7:17 AM
services/backup/docker-server/contents/server/src/Reactors/client/blob/BlobPutClientReactor.h
53–54 ↗(On Diff #10761)

Plus, we definitely should avoid using raw pointers as much as possible, so this should be changed.

• karol added a reviewer: marcin.Apr 4 2022, 8:10 AM
ashoat accepted this revision.Apr 5 2022, 1:10 AM

Ah yeah, looks like you're right that the compiler doesn't enforce it. That's unfortunate, but I agree this is still more readable and clear

This revision is now accepted and ready to land.Apr 5 2022, 1:10 AM
• karol updated this revision to Diff 11033.Apr 5 2022, 1:20 AM

rebase

• karol added a comment.Apr 5 2022, 1:20 AM
In D3487#98630, @palys-swm wrote:

It looks like there's a build error caused by some other diff

This is the error I already fixed on master so I'm going to retrigger CI, should be ok!

Harbormaster completed remote builds in B7859: Diff 11033.Apr 5 2022, 1:26 AM
Closed by commit rCOMM2ca4b0babd9b: [services] Backup - Fix copying big data chunk (authored by • karol). · Explain WhyApr 5 2022, 2:14 AM
This revision was automatically updated to reflect the committed changes.
• karol added a commit: rCOMM2ca4b0babd9b: [services] Backup - Fix copying big data chunk.

Revision Contents
Changeset List

PathSize
services/
backup/
docker-server/
contents/
server/
src/
Reactors/
client/
blob/
9 lines
server/
6 lines

Diff 11040

View Options

services/backup/docker-server/contents/server/src/Reactors/client/blob/BlobPutClientReactor.h

Show All 21 Lines
a { a {
text-decoration: none; text-decoration: none;
color: #2a5db0; color: #2a5db0;
} }
img, img,
iframe { iframe {
display: block; display: block;
} }
input[type='text'], input[type='text'],
input[type='password'], input[type='password'],
textarea { textarea {
atulUnsubmitted
Not Done
Inline Actions

Moving the global input styling and moving it to the input component.

Then why are we keeping styles like border and border-radius?

Don't these styles also affect the <textarea> tag? Are you sure we want to be removing these styles for that as well? Or should we break this into two separate selectors (one for input and another for textarea)?

atul: > Moving the global input styling and moving it to the input component. Then why are we…
benschacAuthorUnsubmitted
Done
Inline Actions

Yeah, I should break these styles out a bit more. I don't think they really do anything. But you make a good point that this will also effect textarea elements.

benschac: Yeah, I should break these styles out a bit more. I don't think they really do anything. But…
-webkit-appearance: none;
-moz-appearance: none;
-webkit-border-radius: 0;
border: 1px solid #dddddd; border: 1px solid #dddddd;
border-radius: 1px; border-radius: 1px;
font-family: var(--font-stack); font-family: var(--font-stack);
} }
:global(#react-root) { :global(#react-root) {
display: flex; display: flex;
flex-direction: column; flex-direction: column;
height: 100%; height: 100%;
▲ Show 20 Lines • Show All 243 Lines • Show Last 20 Lines
View Options

services/backup/docker-server/contents/server/src/Reactors/server/CreateNewBackupReactor.h

Show All 21 Lines
a { a {
text-decoration: none; text-decoration: none;
color: #2a5db0; color: #2a5db0;
} }
img, img,
iframe { iframe {
display: block; display: block;
} }
input[type='text'], input[type='text'],
input[type='password'], input[type='password'],
textarea { textarea {
atulUnsubmitted
Not Done
Inline Actions

Moving the global input styling and moving it to the input component.

Then why are we keeping styles like border and border-radius?

Don't these styles also affect the <textarea> tag? Are you sure we want to be removing these styles for that as well? Or should we break this into two separate selectors (one for input and another for textarea)?

atul: > Moving the global input styling and moving it to the input component. Then why are we…
benschacAuthorUnsubmitted
Done
Inline Actions

Yeah, I should break these styles out a bit more. I don't think they really do anything. But you make a good point that this will also effect textarea elements.

benschac: Yeah, I should break these styles out a bit more. I don't think they really do anything. But…
-webkit-appearance: none;
-moz-appearance: none;
-webkit-border-radius: 0;
border: 1px solid #dddddd; border: 1px solid #dddddd;
border-radius: 1px; border-radius: 1px;
font-family: var(--font-stack); font-family: var(--font-stack);
} }
:global(#react-root) { :global(#react-root) {
display: flex; display: flex;
flex-direction: column; flex-direction: column;
height: 100%; height: 100%;
▲ Show 20 Lines • Show All 243 Lines • Show Last 20 Lines