fix copy of structs through the scheduler. #924

blagoev · 2022-09-26T11:28:26Z

We did not copy the native structs for session errors and http requests correctly. This PR fixes that

nirinchev · 2022-09-26T11:39:31Z

src/realm_dart_sync.cpp

@@ -33,19 +33,19 @@ RLM_API void realm_dart_http_request_callback(realm_userdata_t userdata, const r
    realm_http_request_t request_copy = request; // copy struct


You don't need to copy the request here - you can std::move the request and create the copy inside the scheduler callback.

I am not sure about that. The request is const arg so we should not std::move it

std::move-ing doesn't guarantee that a move will occur, it only tells the compiler it may move it.

yes and accessing the old reference is undefined behavior. Hence I am hesitant to do what you suggest

We're not accessing the old reference though, so I'm not sure I see the problem. If you do prefer to eagerly copy here, then we don't need to copy it again on line 45.

My point is a const arg should not be modified. And by moving it we will invalidate it.

on line 45 we do need to copy it again since it's not modifiable since its an rvalue.

pardon me, on line 45 its not a copy. It's a move so as efficient as it can be.

You can just make the argument not be const. Top-level const on arguments is ignored when deciding the function's type, so void(foo) and void(const foo) have the same type and can both be assigned to a void(*)(foo) function pointer. The const is only meaningful inside the implementation of the function as it makes the parameter value behave like a const object. In this case, that is probably something you don't want.

std::move-ing doesn't guarantee that a move will occur, it only tells the compiler it may move it.

Sort of. std::move makes its result a (possibly const) rvalue. That then can affect overload resolution specifically to choose overloads that then move from the object. All of this is specifically defined, and no part of it is up to the compiler (except for the poorly named copy-elision optimization where it is allowed to completely skip both copies and moves in some cases).

yes and accessing the old reference is undefined behavior.

Not quite, although it is perhaps a good rule of thumb to pretend this is the case most of the time. A) it isn't the std::move that causes this affect, it is whether a move actually occurs. B) For all standard library types, you are guaranteed that a moved-from object will be left in a "valid but unspecified state". This means that you can call any methods that don't have preconditions. For user-defined types, unless otherwise stated, it is best to assume that you can only destroy the object or assign over it. "Named assignments" such as clear() or reset() are also generally considered acceptable.

(As I said, only using std::move on object you don't touch again is a good safe rule of thumb if you don't want to think too hard. But this thread was already getting into the weeds, so I wanted to make sure there was a precise statement of what the real rules are)

Thanks @RedBeard0531. Good insights. I was with the impression that const args will help the compiler in someway optimize the function invocation. I will remove the const from the arg there.

CHANGELOG.md

blagoev · 2022-09-26T12:10:46Z

@nirinchev seems like this change is not good. Will convert to draft until it is ready

coveralls · 2022-09-26T12:42:42Z

Pull Request Test Coverage Report for Build 3158967167

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.2%) to 87.87%

Totals
Change from base Build 3126503085:	0.2%
Covered Lines:	2195
Relevant Lines:	2498

💛 - Coveralls

nirinchev · 2022-09-26T14:12:39Z

src/realm_dart_sync.cpp


    auto ud = reinterpret_cast<realm_dart_userdata_async_t>(userdata);
    ud->scheduler->invoke([ud, request_copy = std::move(request_copy), buf = std::move(buf), request_context]() {
-        (reinterpret_cast<realm_http_request_func_t>(ud->dart_callback)(ud->handle, request_copy, request_context));
+        realm_http_request_t request = std::move(request_copy);


Why is this copy necessary?

on line 45 we do need to copy it again since it's not modifiable since its an rvalue.

In other words you get this error if you try to modify request_copy.url
error C3490: 'url' cannot be modified because it is being accessed through a const object

Btw this realm_http_request_t request = std::move(request_copy); is not a copy

It's a move so as efficient as it can be.

Also, if you add mutable to the lambda, you can get rid of the realm_copy variable.

BTW, if you are OK writing this part in C++, why not just implement the C++ GenericNetworkTransport API directly?

Btw the C-API definition of the http request is this one

/** * Callback function used by Core to make a HTTP request. * * Complete the request by calling realm_http_transport_complete_request(), * passing in the request_context pointer here and the received response. * Network request are expected to be asynchronous and can be completed on any thread. * * @param request The request to send. * @param request_context Internal state pointer of Core, needed by realm_http_transport_complete_request(). */ typedef void (*realm_http_request_func_t)(realm_userdata_t userdata, const realm_http_request_t request, void* request_context);

Shouldn't that be a const realm_http_request_t& request instead. Why do they insist on copying the request object while invoking the function.

This is how they invoke it https://github.com/realm/realm-core/blob/master/src/realm/object-store/c_api/http.cpp#L60-L63.

So why not pass the second argument by reference?

@RedBeard0531 I did the fix. Could you please, check the PR again. Also this message above :)

ok I spoke with Yavor about this. seems no point on doing that. there will be not much gain

blagoev · 2022-09-26T14:13:16Z

@nirinchev I fixed the issue and opening this again. I believe the CI fails are not related to this change

CHANGELOG.md

.vscode/settings.json

RedBeard0531 · 2022-09-27T08:34:14Z

src/realm_dart_sync.cpp

@@ -33,19 +33,19 @@ RLM_API void realm_dart_http_request_callback(realm_userdata_t userdata, const r
    realm_http_request_t request_copy = request; // copy struct


You can just make the argument not be const. Top-level const on arguments is ignored when deciding the function's type, so void(foo) and void(const foo) have the same type and can both be assigned to a void(*)(foo) function pointer. The const is only meaningful inside the implementation of the function as it makes the parameter value behave like a const object. In this case, that is probably something you don't want.

std::move-ing doesn't guarantee that a move will occur, it only tells the compiler it may move it.

Sort of. std::move makes its result a (possibly const) rvalue. That then can affect overload resolution specifically to choose overloads that then move from the object. All of this is specifically defined, and no part of it is up to the compiler (except for the poorly named copy-elision optimization where it is allowed to completely skip both copies and moves in some cases).

yes and accessing the old reference is undefined behavior.

Not quite, although it is perhaps a good rule of thumb to pretend this is the case most of the time. A) it isn't the std::move that causes this affect, it is whether a move actually occurs. B) For all standard library types, you are guaranteed that a moved-from object will be left in a "valid but unspecified state". This means that you can call any methods that don't have preconditions. For user-defined types, unless otherwise stated, it is best to assume that you can only destroy the object or assign over it. "Named assignments" such as clear() or reset() are also generally considered acceptable.

(As I said, only using std::move on object you don't touch again is a good safe rule of thumb if you don't want to think too hard. But this thread was already getting into the weeds, so I wanted to make sure there was a precise statement of what the real rules are)

RedBeard0531 · 2022-09-27T08:54:12Z

src/realm_dart_sync.cpp


    auto ud = reinterpret_cast<realm_dart_userdata_async_t>(userdata);
    ud->scheduler->invoke([ud, request_copy = std::move(request_copy), buf = std::move(buf), request_context]() {
-        (reinterpret_cast<realm_http_request_func_t>(ud->dart_callback)(ud->handle, request_copy, request_context));
+        realm_http_request_t request = std::move(request_copy);


The real answer here is that you want to add mutable to the lambda. Without it, all by-value captures, including request_copy are considered as const inside the body of the lambda. So request is silently making a copy, even though there is a std::move. You probably should also remove the const from the original request parameter, and the request_copy variable and just capture request as ~request = std::move(request)~~ simply request (see next paragraph). And if it the lambda is marked mutable, you can just modify it in place without making another copy. Since the callback will only be called once this is fine.

Oh. I just noticed that realm_http_request_t is the C API type. For C types (or any other "Trivially Copiable" type in C++) there is no difference between move and copy. So all of this discussion of move vs copy is moot, and you might as well just copy since it is syntactically lighter. I'm leaving what I've already written about std::move in the hope that it is informative.

std::move is not a copy its more of a const_cast

To be pedantic, const_cast can only add or remove const and volatile qualifiers. std::move is specifically a static_cast that adds turns an lvalue into an rvalue. It does NOT remove the const. But it also doesn't copy (although the thing you pass the result to may make a copy of the referenced object).

my understanding is that you cannot std::move a const object.

You can std::move a const object, but you probably shouldn't. It produces a const T&& which is a bit of a weird thing. While a type could have a special constructor just for that, in general it will select the copy constructor during overload resolution and make a copy. So the move isn't harmful (in that it doesn't change the behavior of the program, although it may make more template instantiations and potentially slow down compiles or bloat code size), but it is misleading. We could consider making our own realm::move that will cause a compiler error on const objects so that you know to either remove the const or remove the no-op move. The reason std::move allows passing const object is both for consistency, and because it is useful in templates where there are some cases where you want to move from non-const objects but obviously you can't from const ones, but you don't know what you will be passed and want to work with either. But that isn't something we do often.

nirinchev · 2022-09-28T12:35:18Z

src/realm_dart_sync.cpp


    auto ud = reinterpret_cast<realm_dart_userdata_async_t>(userdata);
-    ud->scheduler->invoke([ud, request_copy = std::move(request_copy), buf = std::move(buf), request_context]() {
-        (reinterpret_cast<realm_http_request_func_t>(ud->dart_callback)(ud->handle, request_copy, request_context));
+    ud->scheduler->invoke([ud, request = std::move(request), buf = std::move(buf), request_context]() mutable {


Suggested change

ud->scheduler->invoke([ud, request = std::move(request), buf = std::move(buf), request_context]() mutable {

ud->scheduler->invoke([ud, request, buf = std::move(buf), request_context]() mutable {

As far as I understand, std::move doesn't really do anything for that struct.

That is true for buf as well as I understand it. I kept it for consistency. Otherwise whoever reads it later will wonder why we are not moving request but we are moving buf. Also for c++ I prefer being explicit.

If it doesn't do anything, then let's remove it from buf as well - no point in pretending we move something.

Why do you think std::move(buf) doesn't do anything? buf is not a trivially copiable C-like type. Note that it's members include C++ lifetime aware types. This id different from request which is a plain C-like struct, and is required to be as part of the C API.

TL;DR: I agree with the suggested change.

Edit: that said, after looking at this, it may be even more efficient to do auto buf = std::make_unique<request_copy_buf>, and possibly put all of your captures in the buf. It will be an extra allocation, but then you will have less moves to do. Right now, I think you are moving each member at least twice, and that adds up, maybe to more than a malloc/free pair. Your choice if you want to do this, or avoid the allocation.

I prefer to leave that so it is more understandable what the intention is. Otherwise you really need to understand there is no difference in this particular case.

Fair enough. I am not maintaining this code, so if the people who will be think this will be more maintainable, I won't stand in your way.

RedBeard0531 · 2022-09-30T09:56:59Z

src/realm_dart_sync.cpp


    auto ud = reinterpret_cast<realm_dart_userdata_async_t>(userdata);
-    ud->scheduler->invoke([ud, request_copy = std::move(request_copy), buf = std::move(buf), request_context]() {
-        (reinterpret_cast<realm_http_request_func_t>(ud->dart_callback)(ud->handle, request_copy, request_context));
+    ud->scheduler->invoke([ud, request = std::move(request), buf = std::move(buf), request_context]() mutable {


Why do you think std::move(buf) doesn't do anything? buf is not a trivially copiable C-like type. Note that it's members include C++ lifetime aware types. This id different from request which is a plain C-like struct, and is required to be as part of the C API.

TL;DR: I agree with the suggested change.

Edit: that said, after looking at this, it may be even more efficient to do auto buf = std::make_unique<request_copy_buf>, and possibly put all of your captures in the buf. It will be an extra allocation, but then you will have less moves to do. Right now, I think you are moving each member at least twice, and that adds up, maybe to more than a malloc/free pair. Your choice if you want to do this, or avoid the allocation.

RedBeard0531 · 2022-09-30T10:02:23Z

src/realm_dart_sync.cpp


    auto ud = reinterpret_cast<realm_dart_userdata_async_t>(userdata);
-    ud->scheduler->invoke([ud, session = *session, error = std::move(error), buf = std::move(buf)]() {
+    ud->scheduler->invoke([ud, session = *session, error = std::move(error), buf = std::move(buf)]() mutable {


I think the std::move of error is meaningless here as well. In fact all comments carry over.

I am amused that this code is basically identical to the above, but unfortunately, due to the structure of the c api, it is just different enough that I don't think there is a clean way to avoid the code duplication.

RedBeard0531 · 2022-09-30T10:05:42Z

src/realm_dart_sync.cpp

-RLM_API void realm_dart_http_request_callback(realm_userdata_t userdata, const realm_http_request_t request, void* request_context) {
-    // the pointers in error are to stack values, we need to make copies and move them into the scheduler invocation
+RLM_API void realm_dart_http_request_callback(realm_userdata_t userdata, realm_http_request_t request, void* request_context) {
+    // the pointers in request are to stack values, we need to make copies and move them into the scheduler invocation
    struct request_copy_buf {
        std::string url;
        std::string body;
        std::map<std::string, std::string> headers;


FYI, it is probably better to use a std::deque<std::pair<std::string, std::string>> here. You could also use a std::vector<...> as long as you reserve() the correct size up front, or do two passes, one to push everything, and another to push collect the addresses of the strings. You just need to avoid a growth event after collecting the addresses.

RedBeard0531 · 2022-09-30T10:13:17Z

src/realm_dart_sync.cpp

-    ud->scheduler->invoke([ud, request_copy = std::move(request_copy), buf = std::move(buf), request_context]() {
-        (reinterpret_cast<realm_http_request_func_t>(ud->dart_callback)(ud->handle, request_copy, request_context));
+    ud->scheduler->invoke([ud, request = std::move(request), buf = std::move(buf), request_context]() mutable {
+        request.url = buf.url.c_str();


Please add a comment stating that you are only updating the addresses due to moves, and that all other fields of the request, such as sizes do not need to be modified.

Alternatively, since you know the sizes of everything, You might as well just recreate the whole request from scratch rather than moving it around. Your call though.

RedBeard0531 · 2022-09-30T10:19:35Z

All comments are optional suggestions. The code looks correct to me now, at least as far as I can see in a quick review.

src/realm_dart_sync.cpp

fix copy of structs through the scheduler.

9c69ed7

cla-bot bot added the cla: yes label Sep 26, 2022

blagoev requested a review from nirinchev September 26, 2022 11:28

blagoev mentioned this pull request Sep 26, 2022

Add support for API key provider #919

Merged

nirinchev reviewed Sep 26, 2022

View reviewed changes

add changelog entry and fix changelog

2409862

blagoev commented Sep 26, 2022

View reviewed changes

CHANGELOG.md Show resolved Hide resolved

blagoev self-assigned this Sep 26, 2022

blagoev marked this pull request as draft September 26, 2022 12:11

use the correct struct

819342f

blagoev marked this pull request as ready for review September 26, 2022 14:11

blagoev marked this pull request as draft September 26, 2022 14:12

blagoev marked this pull request as ready for review September 26, 2022 14:12

nirinchev reviewed Sep 26, 2022

View reviewed changes

desistefanova reviewed Sep 26, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

desistefanova approved these changes Sep 26, 2022

View reviewed changes

fix changelog link

3b455dd

RedBeard0531 requested changes Sep 27, 2022

View reviewed changes

remove const from arg, remove unnecessary copies

98c4438

blagoev requested a review from RedBeard0531 September 28, 2022 09:24

nirinchev reviewed Sep 28, 2022

View reviewed changes

RedBeard0531 reviewed Sep 30, 2022

View reviewed changes

blagoev added 2 commits September 30, 2022 13:46

add comments

e6a24af

use std::vector for the temp storage

d93a7c4

RedBeard0531 reviewed Sep 30, 2022

View reviewed changes

src/realm_dart_sync.cpp Outdated Show resolved Hide resolved

use named variables

318f1f7

blagoev merged commit 1dc1dc7 into master Sep 30, 2022

blagoev deleted the fix-structs-copy branch September 30, 2022 19:46

github-actions bot locked as resolved and limited conversation to collaborators Mar 15, 2024

		@@ -33,19 +33,19 @@ RLM_API void realm_dart_http_request_callback(realm_userdata_t userdata, const r
		realm_http_request_t request_copy = request; // copy struct

	ud->scheduler->invoke([ud, request = std::move(request), buf = std::move(buf), request_context]() mutable {
	ud->scheduler->invoke([ud, request, buf = std::move(buf), request_context]() mutable {

fix copy of structs through the scheduler. #924

fix copy of structs through the scheduler. #924

Conversation

blagoev commented Sep 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blagoev commented Sep 26, 2022

coveralls commented Sep 26, 2022 • edited Loading

Pull Request Test Coverage Report for Build 3158967167

💛 - Coveralls

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blagoev commented Sep 26, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blagoev Sep 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RedBeard0531 commented Sep 30, 2022

blagoev commented Sep 26, 2022 •

edited

Loading

coveralls commented Sep 26, 2022 •

edited

Loading

blagoev Sep 28, 2022 •

edited

Loading