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.

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

complete upload - skip db access for small object (for performance) #7486

Closed

Conversation

alphaprinz
Copy link
Contributor

Explain the changes

For small objects with one part, we know the start/end of object.
There's no need to fix values in db at the end of the put object.
(It's needed for multipart upload where each part doesn't know its start/end at single part upload).
Also, set part as committed when inserting the part (again, to skip db access for removing uncommitted at the end).

Issues: Fixed #xxx / Gap #xxx

Redundant db access at the end of upload.

Testing Instructions:

  • Doc added/updated
  • Tests added

src/server/object_services/map_server.js Outdated Show resolved Hide resolved
Comment on lines 413 to 418
} else if (req?.params?.num_parts === 1) {
//skip db access for small objects
dbg.log1("skipped _complete_object_multiparts");
map_res = {size: req.rpc_params.size, num_parts: 1};
} else {
map_res = await _complete_object_parts(obj);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to move this optimization into _complete_object_parts() (need to send params.num_parts inside).

Over there we can start by identifying a single part case, and optimize the hell of it, so that we don't even read the part from the db, which is an aggressive optimization which we also need to explain why it is safe (the reason is that mpu cannot have a single part per multipart... which is a bit of a "bad" assumption to take because it could easily get broken one day by someone who just doesn't expect it calling these apis).

We can also add a simpler condition that will still read the parts, but will skip updating them by noticing that they are already committed and do not require an offset/seq update - simply surround the push to parts_updates with a condition like

if (part.uncommitted || updates.set_updates) {
    context.parts_updates.push(updates);
}

on this line -

context.parts_updates.push(updates);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've moved the check whether to skip the db into _complete_next_parts().
Since we're going for performance, I figured it's best to skip the db network hop altogether.
You think it's not safe enough?

src/server/object_services/object_server.js Outdated Show resolved Hide resolved
src/server/object_services/object_server.js Outdated Show resolved Hide resolved
src/sdk/object_io.js Outdated Show resolved Hide resolved
src/server/object_services/object_server.js Outdated Show resolved Hide resolved
src/server/object_services/object_server.js Outdated Show resolved Hide resolved
@alphaprinz alphaprinz force-pushed the skip_complete_db_small_obj branch 2 times, most recently from b95e540 to d862801 Compare September 19, 2023 06:05
@pull-request-size pull-request-size bot added size/M and removed size/S labels Sep 19, 2023
@pull-request-size pull-request-size bot added size/S and removed size/M labels Sep 19, 2023
@pull-request-size pull-request-size bot added size/M and removed size/S labels Sep 19, 2023
@alphaprinz alphaprinz marked this pull request as ready for review September 21, 2023 10:20
@alphaprinz alphaprinz changed the title DRAFT complete upload - skip db access for small object (for performance) complete upload - skip db access for small object (for performance) Sep 21, 2023
Signed-off-by: Amit Prinz Setter <alphaprinz@gmail.com>
Signed-off-by: Amit Prinz Setter <alphaprinz@gmail.com>
…nstead of MapServer)

Signed-off-by: Amit Prinz Setter <alphaprinz@gmail.com>
Signed-off-by: Amit Prinz Setter <alphaprinz@gmail.com>
Signed-off-by: Amit Prinz Setter <alphaprinz@gmail.com>
Signed-off-by: Amit Prinz Setter <alphaprinz@gmail.com>
Copy link

This PR had no activity for too long - it will now be labeled stale. Update it to prevent it from getting closed.

@github-actions github-actions bot added the Stale label Apr 25, 2024
Copy link

This PR is stale and had no activity for too long - it will now be closed.

@github-actions github-actions bot closed this May 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants