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.

Sign up for GitHub

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

fix copy of structs through the scheduler. #924

Merged
merged 8 commits into from
Sep 30, 2022
Merged

fix copy of structs through the scheduler. #924

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
9c69ed7
fix copy of structs through the scheduler.
blagoev Sep 26, 2022
2409862
add changelog entry and fix changelog
blagoev Sep 26, 2022
819342f
use the correct struct
blagoev Sep 26, 2022
3b455dd
fix changelog link
blagoev Sep 27, 2022
98c4438
remove const from arg, remove unnecessary copies
blagoev Sep 27, 2022
e6a24af
add comments
blagoev Sep 30, 2022
d93a7c4
use std::vector for the temp storage
blagoev Sep 30, 2022
318f1f7
use named variables
blagoev Sep 30, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 80 additions & 1 deletion .vscode/settings.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,85 @@
"xstring": "cpp",
"xutility": "cpp",
"iosfwd": "cpp",
"sstream": "cpp"
"sstream": "cpp",
RedBeard0531 marked this conversation as resolved.
Show resolved Hide resolved
"any": "cpp",
"array": "cpp",
"bit": "cpp",
"bitset": "cpp",
"cctype": "cpp",
"cfenv": "cpp",
"charconv": "cpp",
"cinttypes": "cpp",
"clocale": "cpp",
"cmath": "cpp",
"codecvt": "cpp",
"complex": "cpp",
"condition_variable": "cpp",
"csetjmp": "cpp",
"csignal": "cpp",
"cstdarg": "cpp",
"cstddef": "cpp",
"cstdint": "cpp",
"cstdio": "cpp",
"cstdlib": "cpp",
"cstring": "cpp",
"ctime": "cpp",
"cuchar": "cpp",
"cwchar": "cpp",
"cwctype": "cpp",
"deque": "cpp",
"exception": "cpp",
"forward_list": "cpp",
"future": "cpp",
"initializer_list": "cpp",
"iomanip": "cpp",
"ios": "cpp",
"iostream": "cpp",
"istream": "cpp",
"limits": "cpp",
"list": "cpp",
"locale": "cpp",
"map": "cpp",
"mutex": "cpp",
"new": "cpp",
"numeric": "cpp",
"ostream": "cpp",
"queue": "cpp",
"random": "cpp",
"ratio": "cpp",
"regex": "cpp",
"scoped_allocator": "cpp",
"set": "cpp",
"shared_mutex": "cpp",
"stack": "cpp",
"stdexcept": "cpp",
"stop_token": "cpp",
"streambuf": "cpp",
"string": "cpp",
"thread": "cpp",
"tuple": "cpp",
"type_traits": "cpp",
"typeindex": "cpp",
"typeinfo": "cpp",
"unordered_map": "cpp",
"unordered_set": "cpp",
"utility": "cpp",
"valarray": "cpp",
"xfacet": "cpp",
"xhash": "cpp",
"xiosbase": "cpp",
"xlocbuf": "cpp",
"xlocinfo": "cpp",
"xlocmes": "cpp",
"xlocnum": "cpp",
"xloctime": "cpp",
"xstddef": "cpp",
"xtr1common": "cpp",
"xtree": "cpp",
"*.tcc": "cpp",
"memory_resource": "cpp",
"string_view": "cpp",
"rope": "cpp",
"slist": "cpp"
}
}
25 changes: 16 additions & 9 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,17 @@

**This project is in the Beta stage. The API should be quite stable, but occasional breaking changes may be made.**

### Breaking Changes
* Fixed an issue that would cause passwords sent to the server (e.g. `Credentials.EmailPassword` or `EmailPasswordAuthProvider.registerUser`) to contain an extra empty byte at the end. (PR [#918](https://github.com/realm/realm-dart/pull/918)).
Notice: Any existing email users might need to be recreated because of this breaking change.
blagoev marked this conversation as resolved.
Show resolved Hide resolved

### Enhancements
* Added support for "frozen objects" - these are objects, queries, lists, or Realms that have been "frozen" at a specific version. All frozen objects can be accessed and queried as normal, but attempting to mutate them or add change listeners will throw an exception. `Realm`, `RealmObject`, `RealmList`, and `RealmResults` now have a method `freeze()` which returns an immutable version of the object, as well as an `isFrozen` property which can be used to check whether an object is frozen. ([#56](https://github.com/realm/realm-dart/issues/56))
* You can now set a realm property of type `T` to any object `o` where `o is T`. Previously it was required that `o.runtimeType == T`. ([#904](https://github.com/realm/realm-dart/issues/904))
* Performance of indexOf on realm lists has been improved. It now uses realm-core instead of the generic version from ListMixin. ([#911](https://github.com/realm/realm-dart/pull/911))
* Performance of remove on realm list has been improved. It now uses indexOf and removeAt. ([#915](https://github.com/realm/realm-dart/pull/915))
* Added support for migrations for local Realms. You can now construct a configuration with a migration callback that will be invoked if the schema version of the file on disk is lower than the schema version supplied by the callback. ([#70](https://github.com/realm/realm-dart/issues/70))

A minimal example looks like this:
Example:
```dart
final config = Configuration.local([Person.schema], schemaVersion: 4, migrationCallback: (migration, oldSchemaVersion) {
if (oldSchemaVersion == 1) {
Expand Down Expand Up @@ -37,20 +40,24 @@
}
});
```
* Added support for realm list of nullable primitive types, ie. `RealmList<int?>`. ([#163](https://github.com/realm/realm-dart/issues/163))
* Allow null arguments on query. ([#871](https://github.com/realm/realm-dart/issues/871))

### Fixed
* Allow null arguments on query. ([#871](https://github.com/realm/realm-dart/issues/871))
* Previously removeAt did not truncate length. ([#883](https://github.com/realm/realm-dart/issues/883))
* List.length= now throws, if you try to increase length, ([#894](https://github.com/realm/realm-dart/pull/894)).
* List.length= now throws, if you try to increase length. This previously succeeded silently. ([#894](https://github.com/realm/realm-dart/pull/894)).
* Queries on lists were broken. ([#909](https://github.com/realm/realm-dart/issues/909))
Example:
```dart
expect(realm.all<Person>(), [alice, bob, carol, dan]); // assume this pass, then ...
expect(team.players.query('TRUEPREDICATE'), [alice, bob]); // <-- ... this fails and return the same as realm.all<Person>()
```
* Queries on results didn't filter the existing results. ([#908](https://github.com/realm/realm-dart/issues/908)).
As an example
Example
```dart
expect(realm.query<Person>('FALSEPREDICATE').query('TRUEPREDICATE'), isEmpty);
expect(realm.query<Person>('FALSEPREDICATE').query('TRUEPREDICATE'), isEmpty); //<-- Fails if a Persion object exists
```
would fail if any Persons exists
* Fixed an issue that would cause passwords sent to the server (e.g. `Credentials.EmailPassword` or `EmailPasswordAuthProvider.registerUser`) to contain an extra empty byte at the end. (PR [#918](https://github.com/realm/realm-dart/pull/918))
* Added support for realm list of nullable primitive types, ie. `RealmList<int?>`. ([#163](https://github.com/realm/realm-dart/issues/163))
* Fixed copying of native structs for session errors and http requests. ([#924](https://github.com/realm/realm-dart/pull/924))

### Compatibility
* Realm Studio: 12.0.0 or later.
Expand Down
52 changes: 26 additions & 26 deletions src/realm_dart_sync.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,35 +18,35 @@

#include <realm/object-store/sync/sync_session.hpp>
#include <realm/sync/config.hpp>

#include "realm_dart.hpp"
#include "realm_dart_sync.h"

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;
std::vector<realm_http_header_t> headers_vector;
std::vector<std::pair<std::string, std::string>> headers_values;
std::vector<realm_http_header_t> headers;
} buf;

realm_http_request_t request_copy = request; // copy struct

buf.url = request.url;
request_copy.url = buf.url.c_str();
buf.body = std::string(request.body, request.body_size);
request_copy.body = buf.body.data();

buf.headers_vector.reserve(request.num_headers);
buf.headers_values.reserve(request.num_headers);
buf.headers.reserve(request.num_headers);
for (size_t i = 0; i < request.num_headers; i++) {
auto [it, _] = buf.headers.emplace(request.headers[i].name, request.headers[i].value);
buf.headers_vector.push_back({ it->first.c_str(), it->second.c_str() });
auto& [name, value] = buf.headers_values.emplace_back(request.headers[i].name, request.headers[i].value);
buf.headers.push_back({name.c_str(), value.c_str() });
}
request_copy.headers = buf.headers_vector.data();

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 {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@blagoev blagoev Sep 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

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.

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 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.

Copy link

Choose a reason for hiding this comment

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

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.

//we moved buf so we need to update the request pointers here.
request.url = buf.url.c_str();
Copy link

Choose a reason for hiding this comment

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

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.

request.body = buf.body.data();
request.headers = buf.headers.data();
(reinterpret_cast<realm_http_request_func_t>(ud->dart_callback)(ud->handle, request, request_context));
});
}

Expand All @@ -64,25 +64,25 @@ RLM_API void realm_dart_sync_error_handler_callback(realm_userdata_t userdata, r
struct error_copy {
std::string message;
std::string detailed_message;
std::map<std::string, std::string> user_info_map;
std::vector<realm_sync_error_user_info_t> user_info_vector;
std::vector<std::pair<std::string, std::string>> user_info_values;
std::vector<realm_sync_error_user_info_t> user_info;
} buf;

buf.message = error.error_code.message;
error.error_code.message = buf.message.c_str();

buf.detailed_message = error.detailed_message;
error.detailed_message = buf.detailed_message.c_str();

buf.user_info_vector.reserve(error.user_info_length);
buf.user_info_values.reserve(error.user_info_length);
buf.user_info.reserve(error.user_info_length);
for (size_t i = 0; i < error.user_info_length; i++) {
auto [it, _] = buf.user_info_map.emplace(error.user_info_map[i].key, error.user_info_map[i].value);
buf.user_info_vector.push_back({ it->first.c_str(), it->second.c_str() });
auto& [key, value] = buf.user_info_values.emplace_back(error.user_info_map[i].key, error.user_info_map[i].value);
buf.user_info.push_back({ key.c_str(), value.c_str() });
}
error.user_info_map = buf.user_info_vector.data();

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 {
Copy link

Choose a reason for hiding this comment

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

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.

//we moved buf so we need to update the error pointers here.
error.error_code.message = buf.message.c_str();
error.detailed_message = buf.detailed_message.c_str();
error.user_info_map = buf.user_info.data();
(reinterpret_cast<realm_sync_error_handler_func_t>(ud->dart_callback))(ud->handle, const_cast<realm_sync_session_t*>(&session), error);
});
}
Expand Down
2 changes: 1 addition & 1 deletion src/realm_dart_sync.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@

#include <realm.h>

RLM_API void realm_dart_http_request_callback(realm_userdata_t userdata, const realm_http_request_t request, void* request_context);
RLM_API void realm_dart_http_request_callback(realm_userdata_t userdata, realm_http_request_t request, void* request_context);

RLM_API void realm_dart_sync_client_log_callback(realm_userdata_t userdata, realm_log_level_e level, const char* message);

Expand Down