Page MenuHomePhabricator
Differential D6774

Start NotificationsCryptoModule API definition and implementation
ClosedPublic
Actions

Authored by marcin on Feb 20 2023, 4:05 AM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Mar 25, 11:41 AM
Unknown Object (File)
Sat, Mar 22, 9:44 PM
Unknown Object (File)
Tue, Mar 18, 9:00 AM
Unknown Object (File)
Sun, Mar 16, 1:44 AM
Unknown Object (File)
Sun, Mar 16, 1:44 AM
Unknown Object (File)
Sun, Mar 16, 1:44 AM
Unknown Object (File)
Sun, Mar 16, 1:44 AM
Unknown Object (File)
Sun, Mar 16, 1:44 AM
View All Files
Subscribers
ashoat
• jon

Details

Reviewers
tomek
bartek
atul
• jon
Commits
rCOMMbc8301ae6a64: Start NotificationsCryptoModule API definition and implementation
Summary

This differential launches definition and implementation of NotificationsCryptoModule C++ API. Foundational methods to serialize and deserialize file containing notifications olm account are defined. Relevant build files on both platforms are also modified.

Test Plan

There is not test plan for this differential. Header files are not compiled if there is no cpp file including them. Very next differentials are going to include this file and implement methods from this file.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

marcin created this revision.Feb 20 2023, 4:05 AM
Herald added a subscriber: ashoat. · View Herald TranscriptFeb 20 2023, 4:05 AM
marcin added a parent revision: D6773: Extend PlatformSpecificTools CommonCpp api with an API to retrieve notifications olm account storage path.Feb 20 2023, 4:05 AM
marcin retitled this revision from Start NotificationsCryptoModule API definitiaon and implementation to Start NotificationsCryptoModule API definition and implementation.
marcin added a child revision: D6775: Implement method to deserialize olm account from a file.Feb 20 2023, 4:19 AM
marcin published this revision for review.Feb 20 2023, 4:43 AM
Harbormaster failed remote builds in B16604: Diff 22710!Feb 20 2023, 7:23 AM
ashoat added a reviewer: • jon.Feb 20 2023, 3:46 PM
ashoat added a subscriber: • jon.

Any change to CMake files should go through @jon, especially native/android/app/CMakeLists.txt

• jon accepted this revision.Feb 20 2023, 6:19 PM

I plan on improving this anyway, looks good for now.

This revision is now accepted and ready to land.Feb 20 2023, 6:19 PM
bartek accepted this revision.Feb 21 2023, 4:08 AM
marcin updated this revision to Diff 22878.Feb 21 2023, 10:48 AM

Rebase

Harbormaster failed remote builds in B16723: Diff 22878!Feb 21 2023, 11:02 AM
Harbormaster completed remote builds in B16723: Diff 22878.Feb 21 2023, 11:31 AM
marcin updated this revision to Diff 23252.Feb 28 2023, 10:05 AM

Rebase before landing

Harbormaster failed remote builds in B16998: Diff 23252!Feb 28 2023, 10:12 AM
Harbormaster completed remote builds in B16998: Diff 23252.Feb 28 2023, 1:49 PM
marcin updated this revision to Diff 23306.Mar 1 2023, 1:20 AM

Rebase before landing

Harbormaster completed remote builds in B17023: Diff 23306.Mar 1 2023, 1:38 AM
Closed by commit rCOMMbc8301ae6a64: Start NotificationsCryptoModule API definition and implementation (authored by marcin). · Explain WhyMar 1 2023, 2:04 AM
This revision was automatically updated to reflect the committed changes.
marcin added a commit: rCOMMbc8301ae6a64: Start NotificationsCryptoModule API definition and implementation.

Revision Contents
Changeset List

PathSize
native/
android/
app/
4 lines
cpp/
CommonCpp/
Notifications/
BackgroundDataStorage/
27 lines
ios/
Comm.xcodeproj/
18 lines

Diff 23252

View Options

native/android/app/CMakeLists.txt

#include "DatabaseManagerBase.h" #include "DatabaseManagerBase.h"
#include "Item.h" #include "Item.h"
#include <aws/core/utils/Outcome.h> #include <aws/core/utils/Outcome.h>
#include <aws/dynamodb/model/BatchWriteItemRequest.h>
#include <aws/dynamodb/model/BatchWriteItemResult.h>
#include <aws/dynamodb/model/DeleteItemRequest.h> #include <aws/dynamodb/model/DeleteItemRequest.h>
#include <iostream> #include <iostream>
namespace comm { namespace comm {
namespace network { namespace network {
namespace database { namespace database {
Show All 23 Linesvoid DatabaseManagerBase::innerRemoveItem(const Item &item) {
const Aws::DynamoDB::Model::DeleteItemOutcome &outcome = const Aws::DynamoDB::Model::DeleteItemOutcome &outcome =
getDynamoDBClient()->DeleteItem(request); getDynamoDBClient()->DeleteItem(request);
if (!outcome.IsSuccess()) { if (!outcome.IsSuccess()) {
throw std::runtime_error(outcome.GetError().GetMessage()); throw std::runtime_error(outcome.GetError().GetMessage());
} }
} }
void DatabaseManagerBase::innerBatchWriteItem(
const std::string &tableName,
std::vector<Aws::DynamoDB::Model::WriteRequest> &writeRequests) {
// Split write requests to chunks by MAX_DYNAMODB_BATCH_ITEMS size and write
// them by batch
Aws::DynamoDB::Model::BatchWriteItemOutcome outcome;
std::vector<Aws::DynamoDB::Model::WriteRequest> writeRequestsChunk;
std::vector<Aws::DynamoDB::Model::WriteRequest>::iterator chunkPositionStart,
chunkPositionEnd;
for (size_t i = 0; i < writeRequests.size(); i += MAX_DYNAMODB_BATCH_ITEMS) {
chunkPositionStart = writeRequests.begin() + i;
chunkPositionEnd = writeRequests.begin() +
std::min(writeRequests.size(), i + MAX_DYNAMODB_BATCH_ITEMS);
writeRequestsChunk = std::vector<Aws::DynamoDB::Model::WriteRequest>(
chunkPositionStart, chunkPositionEnd);
tomekUnsubmitted
Done
Inline Actions
Aws::DynamoDB::Model::BatchWriteItemOutcome outcome;
- std::vector<Aws::DynamoDB::Model::WriteRequest> writeRequestsChunk;
- std::vector<Aws::DynamoDB::Model::WriteRequest>::iterator chunkPositionStart,
- chunkPositionEnd;
for (size_t i = 0; i < writeRequests.size(); i += MAX_DYNAMODB_BATCH_ITEMS) {
- chunkPositionStart = writeRequests.begin() + i;
- chunkPositionEnd = writeRequests.begin() +
+ auto chunkPositionStart = writeRequests.begin() + i;
+ auto chunkPositionEnd = writeRequests.begin() +
std::min(writeRequests.size(), i + MAX_DYNAMODB_BATCH_ITEMS);
- writeRequestsChunk = std::vector<Aws::DynamoDB::Model::WriteRequest>(
+ auto writeRequestsChunk = std::vector<Aws::DynamoDB::Model::WriteRequest>(
chunkPositionStart, chunkPositionEnd);
Aws::DynamoDB::Model::BatchWriteItemRequest writeBatchRequest;

You can simplify the code by avoiding reusing the variables and using auto

tomek: You can simplify the code by avoiding reusing the variables and using `auto`
maxAuthorUnsubmitted
Done
Inline Actions

You can simplify the code by avoiding reusing the variables and using auto

I'm not a big fan of using auto where the type can be declared explicitly. Also, reusing the same variables is a part of loop optimization here (we don't create new ones inside the loop and reuse existing allocated memory for it).

max: > You can simplify the code by avoiding reusing the variables and using `auto` I'm not a big…
tomekUnsubmitted
Not Done
Inline Actions

Also, reusing the same variables is a part of loop optimization here

Have you measured the performance difference? I don't think this type of premature optimization is beneficial - if the variable is used only locally, it is easier to reason about it: you don't have to think about its initial value and about possible usages after the loop. Also, having a variable defined inside a loop might enable some optimization (I'm guessing now). Currently, we only reuse the name - new variables are constructed in every iteration.

So overall, we have a lot more important performance issues and saving a couple of machine instructions wouldn't be noticeable. On the other hand, from readability point of view, this is a noticeable difference - you are certain that the variable is used only where it is valid.

tomek: > Also, reusing the same variables is a part of loop optimization here Have you measured the…
Aws::DynamoDB::Model::BatchWriteItemRequest writeBatchRequest;
writeBatchRequest.AddRequestItems(tableName, writeRequestsChunk);
outcome = getDynamoDBClient()->BatchWriteItem(writeBatchRequest);
if (!outcome.IsSuccess()) {
throw std::runtime_error(outcome.GetError().GetMessage());
}
tomekUnsubmitted
Done
Inline Actions
writeBatchRequest.AddRequestItems(tableName, writeRequestsChunk);
- outcome = getDynamoDBClient()->BatchWriteItem(writeBatchRequest);
+ auto outcome = getDynamoDBClient()->BatchWriteItem(writeBatchRequest);
if (!outcome.IsSuccess()) {
throw std::runtime_error(outcome.GetError().GetMessage());
}
}
}

outcome isn't used outside of this loop so we can declare it here

tomek: `outcome` isn't used outside of this loop so we can declare it here
maxAuthorUnsubmitted
Done
Inline Actions

outcome isn't used outside of this loop so we can declare it here

Same as above. I'm sure we should not use auto, where the type can explicitly be used, and allocating and reusing this variable here, is an optimized way from my perspective.

max: > `outcome` isn't used outside of this loop so we can declare it here Same as above. I'm sure…
}
}
} // namespace database } // namespace database
} // namespace network } // namespace network
} // namespace comm } // namespace comm
View Options

native/cpp/CommonCpp/Notifications/BackgroundDataStorage/NotificationsCryptoModule.h

#include "DatabaseManagerBase.h" #include "DatabaseManagerBase.h"
#include "Item.h" #include "Item.h"
#include <aws/core/utils/Outcome.h> #include <aws/core/utils/Outcome.h>
#include <aws/dynamodb/model/BatchWriteItemRequest.h>
#include <aws/dynamodb/model/BatchWriteItemResult.h>
#include <aws/dynamodb/model/DeleteItemRequest.h> #include <aws/dynamodb/model/DeleteItemRequest.h>
#include <iostream> #include <iostream>
namespace comm { namespace comm {
namespace network { namespace network {
namespace database { namespace database {
Show All 23 Linesvoid DatabaseManagerBase::innerRemoveItem(const Item &item) {
const Aws::DynamoDB::Model::DeleteItemOutcome &outcome = const Aws::DynamoDB::Model::DeleteItemOutcome &outcome =
getDynamoDBClient()->DeleteItem(request); getDynamoDBClient()->DeleteItem(request);
if (!outcome.IsSuccess()) { if (!outcome.IsSuccess()) {
throw std::runtime_error(outcome.GetError().GetMessage()); throw std::runtime_error(outcome.GetError().GetMessage());
} }
} }
void DatabaseManagerBase::innerBatchWriteItem(
const std::string &tableName,
std::vector<Aws::DynamoDB::Model::WriteRequest> &writeRequests) {
// Split write requests to chunks by MAX_DYNAMODB_BATCH_ITEMS size and write
// them by batch
Aws::DynamoDB::Model::BatchWriteItemOutcome outcome;
std::vector<Aws::DynamoDB::Model::WriteRequest> writeRequestsChunk;
std::vector<Aws::DynamoDB::Model::WriteRequest>::iterator chunkPositionStart,
chunkPositionEnd;
for (size_t i = 0; i < writeRequests.size(); i += MAX_DYNAMODB_BATCH_ITEMS) {
chunkPositionStart = writeRequests.begin() + i;
chunkPositionEnd = writeRequests.begin() +
std::min(writeRequests.size(), i + MAX_DYNAMODB_BATCH_ITEMS);
writeRequestsChunk = std::vector<Aws::DynamoDB::Model::WriteRequest>(
chunkPositionStart, chunkPositionEnd);
tomekUnsubmitted
Done
Inline Actions
Aws::DynamoDB::Model::BatchWriteItemOutcome outcome;
- std::vector<Aws::DynamoDB::Model::WriteRequest> writeRequestsChunk;
- std::vector<Aws::DynamoDB::Model::WriteRequest>::iterator chunkPositionStart,
- chunkPositionEnd;
for (size_t i = 0; i < writeRequests.size(); i += MAX_DYNAMODB_BATCH_ITEMS) {
- chunkPositionStart = writeRequests.begin() + i;
- chunkPositionEnd = writeRequests.begin() +
+ auto chunkPositionStart = writeRequests.begin() + i;
+ auto chunkPositionEnd = writeRequests.begin() +
std::min(writeRequests.size(), i + MAX_DYNAMODB_BATCH_ITEMS);
- writeRequestsChunk = std::vector<Aws::DynamoDB::Model::WriteRequest>(
+ auto writeRequestsChunk = std::vector<Aws::DynamoDB::Model::WriteRequest>(
chunkPositionStart, chunkPositionEnd);
Aws::DynamoDB::Model::BatchWriteItemRequest writeBatchRequest;

You can simplify the code by avoiding reusing the variables and using auto

tomek: You can simplify the code by avoiding reusing the variables and using `auto`
maxAuthorUnsubmitted
Done
Inline Actions

You can simplify the code by avoiding reusing the variables and using auto

I'm not a big fan of using auto where the type can be declared explicitly. Also, reusing the same variables is a part of loop optimization here (we don't create new ones inside the loop and reuse existing allocated memory for it).

max: > You can simplify the code by avoiding reusing the variables and using `auto` I'm not a big…
tomekUnsubmitted
Not Done
Inline Actions

Also, reusing the same variables is a part of loop optimization here

Have you measured the performance difference? I don't think this type of premature optimization is beneficial - if the variable is used only locally, it is easier to reason about it: you don't have to think about its initial value and about possible usages after the loop. Also, having a variable defined inside a loop might enable some optimization (I'm guessing now). Currently, we only reuse the name - new variables are constructed in every iteration.

So overall, we have a lot more important performance issues and saving a couple of machine instructions wouldn't be noticeable. On the other hand, from readability point of view, this is a noticeable difference - you are certain that the variable is used only where it is valid.

tomek: > Also, reusing the same variables is a part of loop optimization here Have you measured the…
Aws::DynamoDB::Model::BatchWriteItemRequest writeBatchRequest;
writeBatchRequest.AddRequestItems(tableName, writeRequestsChunk);
outcome = getDynamoDBClient()->BatchWriteItem(writeBatchRequest);
if (!outcome.IsSuccess()) {
throw std::runtime_error(outcome.GetError().GetMessage());
}
tomekUnsubmitted
Done
Inline Actions
writeBatchRequest.AddRequestItems(tableName, writeRequestsChunk);
- outcome = getDynamoDBClient()->BatchWriteItem(writeBatchRequest);
+ auto outcome = getDynamoDBClient()->BatchWriteItem(writeBatchRequest);
if (!outcome.IsSuccess()) {
throw std::runtime_error(outcome.GetError().GetMessage());
}
}
}

outcome isn't used outside of this loop so we can declare it here

tomek: `outcome` isn't used outside of this loop so we can declare it here
maxAuthorUnsubmitted
Done
Inline Actions

outcome isn't used outside of this loop so we can declare it here

Same as above. I'm sure we should not use auto, where the type can explicitly be used, and allocating and reusing this variable here, is an optimized way from my perspective.

max: > `outcome` isn't used outside of this loop so we can declare it here Same as above. I'm sure…
}
}
} // namespace database } // namespace database
} // namespace network } // namespace network
} // namespace comm } // namespace comm
View Options

native/ios/Comm.xcodeproj/project.pbxproj

#include "DatabaseManagerBase.h" #include "DatabaseManagerBase.h"
#include "Item.h" #include "Item.h"
#include <aws/core/utils/Outcome.h> #include <aws/core/utils/Outcome.h>
#include <aws/dynamodb/model/BatchWriteItemRequest.h>
#include <aws/dynamodb/model/BatchWriteItemResult.h>
#include <aws/dynamodb/model/DeleteItemRequest.h> #include <aws/dynamodb/model/DeleteItemRequest.h>
#include <iostream> #include <iostream>
namespace comm { namespace comm {
namespace network { namespace network {
namespace database { namespace database {
Show All 23 Linesvoid DatabaseManagerBase::innerRemoveItem(const Item &item) {
const Aws::DynamoDB::Model::DeleteItemOutcome &outcome = const Aws::DynamoDB::Model::DeleteItemOutcome &outcome =
getDynamoDBClient()->DeleteItem(request); getDynamoDBClient()->DeleteItem(request);
if (!outcome.IsSuccess()) { if (!outcome.IsSuccess()) {
throw std::runtime_error(outcome.GetError().GetMessage()); throw std::runtime_error(outcome.GetError().GetMessage());
} }
} }
void DatabaseManagerBase::innerBatchWriteItem(
const std::string &tableName,
std::vector<Aws::DynamoDB::Model::WriteRequest> &writeRequests) {
// Split write requests to chunks by MAX_DYNAMODB_BATCH_ITEMS size and write
// them by batch
Aws::DynamoDB::Model::BatchWriteItemOutcome outcome;
std::vector<Aws::DynamoDB::Model::WriteRequest> writeRequestsChunk;
std::vector<Aws::DynamoDB::Model::WriteRequest>::iterator chunkPositionStart,
chunkPositionEnd;
for (size_t i = 0; i < writeRequests.size(); i += MAX_DYNAMODB_BATCH_ITEMS) {
chunkPositionStart = writeRequests.begin() + i;
chunkPositionEnd = writeRequests.begin() +
std::min(writeRequests.size(), i + MAX_DYNAMODB_BATCH_ITEMS);
writeRequestsChunk = std::vector<Aws::DynamoDB::Model::WriteRequest>(
chunkPositionStart, chunkPositionEnd);
tomekUnsubmitted
Done
Inline Actions
Aws::DynamoDB::Model::BatchWriteItemOutcome outcome;
- std::vector<Aws::DynamoDB::Model::WriteRequest> writeRequestsChunk;
- std::vector<Aws::DynamoDB::Model::WriteRequest>::iterator chunkPositionStart,
- chunkPositionEnd;
for (size_t i = 0; i < writeRequests.size(); i += MAX_DYNAMODB_BATCH_ITEMS) {
- chunkPositionStart = writeRequests.begin() + i;
- chunkPositionEnd = writeRequests.begin() +
+ auto chunkPositionStart = writeRequests.begin() + i;
+ auto chunkPositionEnd = writeRequests.begin() +
std::min(writeRequests.size(), i + MAX_DYNAMODB_BATCH_ITEMS);
- writeRequestsChunk = std::vector<Aws::DynamoDB::Model::WriteRequest>(
+ auto writeRequestsChunk = std::vector<Aws::DynamoDB::Model::WriteRequest>(
chunkPositionStart, chunkPositionEnd);
Aws::DynamoDB::Model::BatchWriteItemRequest writeBatchRequest;

You can simplify the code by avoiding reusing the variables and using auto

tomek: You can simplify the code by avoiding reusing the variables and using `auto`
maxAuthorUnsubmitted
Done
Inline Actions

You can simplify the code by avoiding reusing the variables and using auto

I'm not a big fan of using auto where the type can be declared explicitly. Also, reusing the same variables is a part of loop optimization here (we don't create new ones inside the loop and reuse existing allocated memory for it).

max: > You can simplify the code by avoiding reusing the variables and using `auto` I'm not a big…
tomekUnsubmitted
Not Done
Inline Actions

Also, reusing the same variables is a part of loop optimization here

Have you measured the performance difference? I don't think this type of premature optimization is beneficial - if the variable is used only locally, it is easier to reason about it: you don't have to think about its initial value and about possible usages after the loop. Also, having a variable defined inside a loop might enable some optimization (I'm guessing now). Currently, we only reuse the name - new variables are constructed in every iteration.

So overall, we have a lot more important performance issues and saving a couple of machine instructions wouldn't be noticeable. On the other hand, from readability point of view, this is a noticeable difference - you are certain that the variable is used only where it is valid.

tomek: > Also, reusing the same variables is a part of loop optimization here Have you measured the…
Aws::DynamoDB::Model::BatchWriteItemRequest writeBatchRequest;
writeBatchRequest.AddRequestItems(tableName, writeRequestsChunk);
outcome = getDynamoDBClient()->BatchWriteItem(writeBatchRequest);
if (!outcome.IsSuccess()) {
throw std::runtime_error(outcome.GetError().GetMessage());
}
tomekUnsubmitted
Done
Inline Actions
writeBatchRequest.AddRequestItems(tableName, writeRequestsChunk);
- outcome = getDynamoDBClient()->BatchWriteItem(writeBatchRequest);
+ auto outcome = getDynamoDBClient()->BatchWriteItem(writeBatchRequest);
if (!outcome.IsSuccess()) {
throw std::runtime_error(outcome.GetError().GetMessage());
}
}
}

outcome isn't used outside of this loop so we can declare it here

tomek: `outcome` isn't used outside of this loop so we can declare it here
maxAuthorUnsubmitted
Done
Inline Actions

outcome isn't used outside of this loop so we can declare it here

Same as above. I'm sure we should not use auto, where the type can explicitly be used, and allocating and reusing this variable here, is an optimized way from my perspective.

max: > `outcome` isn't used outside of this loop so we can declare it here Same as above. I'm sure…
}
}
} // namespace database } // namespace database
} // namespace network } // namespace network
} // namespace comm } // namespace comm