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

Store ref in ArrayMixed #6565

Merged
merged 4 commits into from
May 8, 2023
Merged

Store ref in ArrayMixed #6565

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c061c74
Cleanup naming and consolidate update strategy
jedelbo Apr 26, 2023
34e3565
Allow a Mixed containing a ref to be stores in ArrayMixed
jedelbo May 3, 2023
ed18645
update after review
jedelbo May 8, 2023
0edf5cf
Another update
jedelbo May 8, 2023
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
63 changes: 48 additions & 15 deletions src/realm/array_mixed.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ ArrayMixed::ArrayMixed(Allocator& a)
, m_ints(a)
, m_int_pairs(a)
, m_strings(a)
, m_refs(a)
{
m_composite.set_parent(this, payload_idx_type);
}
Expand All @@ -45,6 +46,7 @@ void ArrayMixed::init_from_mem(MemRef mem) noexcept
m_ints.detach();
m_int_pairs.detach();
m_strings.detach();
m_refs.detach();
}

void ArrayMixed::add(Mixed value)
Expand All @@ -63,7 +65,15 @@ void ArrayMixed::set(size_t ndx, Mixed value)
set_null(ndx);
return;
}
erase_linked_payload(ndx);
auto old_type = get_type(ndx);
// If we replace a collections ref value with one of the
// same type, then it is just an update of of the
// ref stored in the parent. If the new type is a different
// type then it means that we are overwriting a collection
// with some other value and hence the collection must be
// destroyed.
bool destroy_collection = old_type != value.get_type();
erase_linked_payload(ndx, destroy_collection);
m_composite.set(ndx, store(value));
}

Expand All @@ -80,7 +90,7 @@ void ArrayMixed::set_null(size_t ndx)
{
auto val = m_composite.get(ndx);
if (val) {
erase_linked_payload(ndx);
erase_linked_payload(ndx, true);
m_composite.set(ndx, 0);
}
}
Expand Down Expand Up @@ -163,6 +173,10 @@ Mixed ArrayMixed::get(size_t ndx) const
return Mixed(UUID(bytes));
}
default:
if (size_t((val & s_payload_idx_mask) >> s_payload_idx_shift) == payload_idx_ref) {
ensure_ref_array();
return Mixed(m_refs.get(payload_ndx), CollectionType(int(type)));
}
break;
}
}
Expand All @@ -176,25 +190,19 @@ void ArrayMixed::clear()
m_ints.destroy();
m_int_pairs.destroy();
m_strings.destroy();
m_refs.destroy_deep();
Array::set(payload_idx_int, 0);
Array::set(payload_idx_pair, 0);
Array::set(payload_idx_str, 0);
Array::set(payload_idx_ref, 0);
Copy link
Member

Choose a reason for hiding this comment

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

Nothing to do with this PR, but why we don't clear also payload_idx_type and payload_idx_size here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what you mean.

}

void ArrayMixed::erase(size_t ndx)
{
erase_linked_payload(ndx);
erase_linked_payload(ndx, true);
m_composite.erase(ndx);
}

void ArrayMixed::truncate_and_destroy_children(size_t ndx)
Copy link
Member

Choose a reason for hiding this comment

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

Why removing this? Truncating seems a reasonable thing to...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not used

{
for (size_t i = size(); i > ndx; i--) {
erase_linked_payload(i - 1);
}
m_composite.truncate(ndx);
}

void ArrayMixed::move(ArrayMixed& dst, size_t ndx)
{
auto sz = size();
Expand All @@ -204,7 +212,7 @@ void ArrayMixed::move(ArrayMixed& dst, size_t ndx)
dst.add(val);
}
while (i > ndx) {
erase_linked_payload(--i);
erase_linked_payload(--i, false);
}
m_composite.truncate(ndx);
}
Expand Down Expand Up @@ -239,7 +247,7 @@ void ArrayMixed::ensure_array_accessor(Array& arr, size_t ndx_in_parent) const
arr.init_from_ref(ref);
}
else {
arr.create(type_Normal);
arr.create(ndx_in_parent == payload_idx_ref ? type_HasRefs : type_Normal);
arr.update_parent();
}
}
Expand Down Expand Up @@ -270,6 +278,14 @@ void ArrayMixed::ensure_string_array() const
}
}

void ArrayMixed::ensure_ref_array() const
{
while (Array::size() < payload_idx_ref + 1) {
const_cast<ArrayMixed*>(this)->Array::add(0);
}
ensure_array_accessor(m_refs, payload_idx_ref);
}

void ArrayMixed::replace_index(size_t old_ndx, size_t new_ndx, size_t payload_arr_index)
{
if (old_ndx != new_ndx) {
Expand All @@ -285,7 +301,7 @@ void ArrayMixed::replace_index(size_t old_ndx, size_t new_ndx, size_t payload_ar
}
}

void ArrayMixed::erase_linked_payload(size_t ndx)
void ArrayMixed::erase_linked_payload(size_t ndx, bool free_linked_arrays)
{
auto val = m_composite.get(ndx);
auto payload_arr_index = size_t((val & s_payload_idx_mask) >> s_payload_idx_shift);
Expand Down Expand Up @@ -330,6 +346,19 @@ void ArrayMixed::erase_linked_payload(size_t ndx)
m_int_pairs.truncate(last_ndx);
break;
}
case payload_idx_ref: {
ensure_ref_array();
last_ndx = m_refs.size() - 1;
auto old_ref = m_refs.get(erase_ndx);
if (erase_ndx != last_ndx) {
m_refs.set(erase_ndx, m_refs.get(last_ndx));
replace_index(last_ndx, erase_ndx, payload_arr_index);
}
m_refs.erase(last_ndx);
if (old_ref && free_linked_arrays)
Array::destroy_deep(old_ref, m_composite.get_alloc());
break;
}
default:
break;
}
Expand Down Expand Up @@ -440,7 +469,11 @@ int64_t ArrayMixed::store(const Mixed& value)
break;
}
default:
val = 0;
REALM_ASSERT(type == type_List || type == type_Dictionary || type == type_Set);
ensure_ref_array();
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer not to assume that if we don't match any type, then it is a collection. Although it is impossible to end up in such scenario, since we control the types in mixed, it is easy to make a mistake if you don't know the code very well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added an assertion.

size_t ndx = m_refs.size();
m_refs.add(value.get_ref());
val = int64_t(ndx << s_data_shift) | (payload_idx_ref << s_payload_idx_shift);
break;
}
return val + int(type) + 1;
Expand Down
9 changes: 6 additions & 3 deletions src/realm/array_mixed.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
#include <realm/array_string.hpp>
#include <realm/array_timestamp.hpp>
#include <realm/array_key.hpp>
#include <realm/array_ref.hpp>

namespace realm {

Expand Down Expand Up @@ -90,15 +91,14 @@ class ArrayMixed : public ArrayPayload, private Array {

void clear();
void erase(size_t ndx);
void truncate_and_destroy_children(size_t ndx);
void move(ArrayMixed& dst, size_t ndx);

size_t find_first(Mixed value, size_t begin = 0, size_t end = realm::npos) const noexcept;

void verify() const;

private:
enum { payload_idx_type, payload_idx_int, payload_idx_pair, payload_idx_str, payload_idx_size };
enum { payload_idx_type, payload_idx_int, payload_idx_pair, payload_idx_str, payload_idx_ref, payload_idx_size };

static constexpr int64_t s_data_type_mask = 0b0001'1111;
static constexpr int64_t s_payload_idx_mask = 0b1110'0000;
Expand All @@ -120,6 +120,8 @@ class ArrayMixed : public ArrayPayload, private Array {
mutable Array m_int_pairs;
// Used to store String and Binary
mutable ArrayString m_strings;
// Used to store nested collection refs
mutable ArrayRef m_refs;

DataType get_type(size_t ndx) const
{
Expand All @@ -130,8 +132,9 @@ class ArrayMixed : public ArrayPayload, private Array {
void ensure_int_array() const;
void ensure_int_pair_array() const;
void ensure_string_array() const;
void ensure_ref_array() const;
void replace_index(size_t old_ndx, size_t new_ndx, size_t payload_index);
void erase_linked_payload(size_t ndx);
void erase_linked_payload(size_t ndx, bool free_linked_arrays);
};
} // namespace realm

Expand Down
64 changes: 32 additions & 32 deletions src/realm/collection.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ class CollectionBase : public Collection {
/// Return true if the collection has changed since the last call to
/// `has_changed()`. Note that this function is not idempotent and updates
/// the internal state of the accessor if it has changed.
virtual bool has_changed() const = 0;
virtual bool has_changed() const noexcept = 0;

/// Returns true if the accessor is in the attached state. By default, this
/// checks if the owning object is still valid.
Expand Down Expand Up @@ -371,10 +371,10 @@ class CollectionBaseImpl : public Interface, protected ArrayParent {
/// Note: This function does not return true for an accessor that became
/// detached since the last call, even though it may look to the caller as
/// if the size of the collection suddenly became zero.
bool has_changed() const final
bool has_changed() const noexcept final
{
// `has_changed()` sneakily modifies internal state.
update_if_needed();
update_if_needed_with_status();
if (m_last_content_version != m_content_version) {
m_last_content_version = m_content_version;
return true;
Expand Down Expand Up @@ -483,22 +483,7 @@ class CollectionBaseImpl : public Interface, protected ArrayParent {
m_parent->set_collection_ref(m_index, ref);
}

/// Refresh the associated `Obj` (if needed), and update the internal
/// content version number. This is meant to be called from a derived class
/// before accessing its data.
///
/// If the `Obj` changed since the last call, or the content version was
/// bumped, this returns `UpdateStatus::Updated`. In response, the caller
/// must invoke `init_from_parent()` or similar on its internal state
/// accessors to refresh its view of the data.
///
/// If the owning object (or parent container) was deleted, this returns
/// `UpdateStatus::Detached`, and the caller is allowed to enter a
/// degenerate state.
///
/// If no change has happened to the data, this function returns
/// `UpdateStatus::NoChange`, and the caller is allowed to not do anything.
virtual UpdateStatus update_if_needed() const
UpdateStatus get_update_status() const noexcept
{
UpdateStatus status = m_parent ? m_parent->update_if_needed_with_status() : UpdateStatus::Detached;

Expand All @@ -513,27 +498,20 @@ class CollectionBaseImpl : public Interface, protected ArrayParent {
return status;
}

/// Refresh the associated `Obj` (if needed) and ensure that the
/// collection is created. Must be used in places where you
/// modify a potentially detached collection.
///
/// The caller must react to the `UpdateStatus` in the same way as with
/// `update_if_needed()`, i.e., eventually end up calling
/// `init_from_parent()` or similar.
///
/// Throws if the owning object no longer exists. Note: This means that this
/// method will never return `UpdateStatus::Detached`.
virtual UpdateStatus ensure_created()
/// Refresh the parent object (if needed) and compare version numbers.
/// Return true if the collection should initialize from parent
/// Throws if the owning object no longer exists.
bool should_update()
{
check_parent();
bool changed = m_parent->update_if_needed(); // Throws if the object does not exist.
auto content_version = m_alloc->get_content_version();

if (changed || content_version != m_content_version) {
m_content_version = content_version;
return UpdateStatus::Updated;
return true;
}
return UpdateStatus::NoChange;
return false;
}

void bump_content_version()
Expand All @@ -542,6 +520,12 @@ class CollectionBaseImpl : public Interface, protected ArrayParent {
m_content_version = m_alloc->bump_content_version();
}

void update_content_version() const
{
REALM_ASSERT(m_alloc);
m_content_version = m_alloc->get_content_version();
}

void bump_both_versions()
{
REALM_ASSERT(m_alloc);
Expand Down Expand Up @@ -631,6 +615,22 @@ class CollectionBaseImpl : public Interface, protected ArrayParent {
throw StaleAccessor("Allocator not set");
}
}
/// Refresh the associated `Obj` (if needed), and update the internal
/// content version number. This is meant to be called from a derived class
/// before accessing its data.
///
/// If the `Obj` changed since the last call, or the content version was
/// bumped, this returns `UpdateStatus::Updated`. In response, the caller
/// must invoke `init_from_parent()` or similar on its internal state
/// accessors to refresh its view of the data.
///
/// If the owning object (or parent container) was deleted, this returns
/// `UpdateStatus::Detached`, and the caller is allowed to enter a
/// degenerate state.
///
/// If no change has happened to the data, this function returns
/// `UpdateStatus::NoChange`, and the caller is allowed to not do anything.
virtual UpdateStatus update_if_needed_with_status() const noexcept = 0;
};

namespace _impl {
Expand Down
Loading