Undo: support implicit-sharing in memfile undo step #106903

Merged
Jacques Lucke merged 78 commits from JacquesLucke/blender:implicit-sharing-undo into main 2024-02-29 17:15:09 +01:00
9 changed files with 163 additions and 15 deletions

View File

@ -1541,9 +1541,11 @@ void CurvesGeometry::blend_read(BlendDataReader &reader)
CustomData_blend_read(&reader, &this->curve_data, this->curve_num);
if (this->curve_offsets) {
BLO_read_int32_array(&reader, this->curve_num + 1, &this->curve_offsets);
this->runtime->curve_offsets_sharing_info = implicit_sharing::info_for_mem_free(
this->curve_offsets);
this->runtime->curve_offsets_sharing_info = BLO_read_shared(
&reader, &this->curve_offsets, [&]() {
BLO_read_int32_array(&reader, this->curve_num + 1, &this->curve_offsets);
return implicit_sharing::info_for_mem_free(this->curve_offsets);
});
}
BLO_read_list(&reader, &this->vertex_group_names);
@ -1569,7 +1571,14 @@ void CurvesGeometry::blend_write(BlendWriter &writer,
CustomData_blend_write(
&writer, &this->curve_data, write_data.curve_layers, this->curve_num, CD_MASK_ALL, &id);
BLO_write_int32_array(&writer, this->curve_num + 1, this->curve_offsets);
if (this->curve_offsets) {
BLO_write_shared(
&writer,
this->curve_offsets,
sizeof(int) * (this->curve_num + 1),
JacquesLucke marked this conversation as resolved Outdated

shouldn't this be * (this->curve_num + 1) ?

shouldn't this be `* (this->curve_num + 1)` ?
this->runtime->curve_offsets_sharing_info,
[&]() { BLO_write_int32_array(&writer, this->curve_num + 1, this->curve_offsets); });
}
BKE_defbase_blend_write(&writer, &this->vertex_group_names);
}

View File

@ -5374,7 +5374,10 @@ void CustomData_blend_write(BlendWriter *writer,
writer, CustomDataLayer, data->totlayer, data->layers, layers_to_write.data());
for (const CustomDataLayer &layer : layers_to_write) {
blend_write_layer_data(writer, layer, count);
const size_t size_in_bytes = CustomData_sizeof(eCustomDataType(layer.type)) * count;
BLO_write_shared(writer, layer.data, size_in_bytes, layer.sharing_info, [&]() {
blend_write_layer_data(writer, layer, count);
});
}
if (data->external) {
@ -5430,11 +5433,6 @@ static void blend_read_paint_mask(BlendDataReader *reader,
static void blend_read_layer_data(BlendDataReader *reader, CustomDataLayer &layer, const int count)
{
BLO_read_data_address(reader, &layer.data);
if (layer.data != nullptr) {
/* Make layer data shareable. */
layer.sharing_info = make_implicit_sharing_info_for_layer(
eCustomDataType(layer.type), layer.data, count);
}
if (CustomData_layer_ensure_data_exists(&layer, count)) {
/* Under normal operations, this shouldn't happen, but...
* For a CD_PROP_BOOL example, see #84935.
@ -5479,7 +5477,12 @@ void CustomData_blend_read(BlendDataReader *reader, CustomData *data, const int
layer->sharing_info = nullptr;
if (CustomData_verify_versions(data, i)) {
blend_read_layer_data(reader, *layer, count);
layer->sharing_info = BLO_read_shared(reader, &layer->data, [&]() {
blend_read_layer_data(reader, *layer, count);
return layer->data ? make_implicit_sharing_info_for_layer(
eCustomDataType(layer->type), layer->data, count) :
nullptr;
});
i++;
}
}

View File

@ -292,6 +292,7 @@ static void mesh_blend_write(BlendWriter *writer, ID *id, const void *id_address
}
}
const blender::bke::MeshRuntime *mesh_runtime = mesh->runtime;
mesh->runtime = nullptr;
BLO_write_id_struct(writer, Mesh, id_address, &mesh->id);
@ -317,7 +318,12 @@ static void mesh_blend_write(BlendWriter *writer, ID *id, const void *id_address
writer, &mesh->face_data, face_layers, mesh->faces_num, CD_MASK_MESH.pmask, &mesh->id);
if (mesh->face_offset_indices) {
BLO_write_int32_array(writer, mesh->faces_num + 1, mesh->face_offset_indices);
BLO_write_shared(
writer,
mesh->face_offset_indices,
sizeof(int) * mesh->faces_num,
mesh_runtime->face_offsets_sharing_info,
[&]() { BLO_write_int32_array(writer, mesh->faces_num + 1, mesh->face_offset_indices); });
}
}
@ -363,9 +369,11 @@ static void mesh_blend_read_data(BlendDataReader *reader, ID *id)
mesh->runtime = new blender::bke::MeshRuntime();
if (mesh->face_offset_indices) {
BLO_read_int32_array(reader, mesh->faces_num + 1, &mesh->face_offset_indices);
mesh->runtime->face_offsets_sharing_info = blender::implicit_sharing::info_for_mem_free(
mesh->face_offset_indices);
mesh->runtime->face_offsets_sharing_info = BLO_read_shared(
reader, &mesh->face_offset_indices, [&]() {
BLO_read_int32_array(reader, mesh->faces_num + 1, &mesh->face_offset_indices);
return blender::implicit_sharing::info_for_mem_free(mesh->face_offset_indices);
});
}
if (mesh->mselect == nullptr) {

View File

@ -125,6 +125,11 @@ class ImplicitSharingInfo : NonCopyable, NonMovable {
return version_.load(std::memory_order_acquire);
}
int strong_users() const
{
return strong_users_.load(std::memory_order_acquire);
}
/**
* Call when the data is no longer needed. This might just decrement the user count, or it might
* also delete the data if this was the last user.

View File

@ -33,6 +33,11 @@
#include "DNA_windowmanager_types.h" /* for eReportType */
#include "BLI_function_ref.hh"
namespace blender {
class ImplicitSharingInfo;
}
struct BlendDataReader;
struct BlendFileReadReport;
struct BlendLibReader;
@ -182,6 +187,21 @@ void BLO_write_string(BlendWriter *writer, const char *data_ptr);
/* Misc. */
/**
* Check if the data can be written more efficiently by making use of implicit-sharing. If yes, the
* user count of the sharing-info is increased making the data immutable. The provided callback
* should serialize the potentially shared data. It is only called when necessary.
*
* \param approximate_size_in_bytes: Used to be able to approximate how large the undo step is in
* total.
* \param write_fn: Use the #BlendWrite to serialize the potentially shared data.
JacquesLucke marked this conversation as resolved Outdated

Don't find it super nice to have to manually compute this value here... At the very least, it should be clearly documented what it is actually used for.

In the future, wouldn't it be better and more consistent to also add this info to ImplicitSharingInfo? Although I guess that would not be that simple, so probably not for this PR.

Don't find it super nice to have to manually compute this value here... At the very least, it should be clearly documented what it is actually used for. In the future, wouldn't it be better and more consistent to also add this info to `ImplicitSharingInfo`? Although I guess that would not be that simple, so probably not for this PR.

I improved the comments.

I'm not sure if this really belongs into ImplicitSharingInfo. The concept of memory usage seems orthogonal to implicit sharing. We should find a way to avoid to not compute the size again and again for more complex data, but that's not really a problem yet.

I improved the comments. I'm not sure if this really belongs into `ImplicitSharingInfo`. The concept of memory usage seems orthogonal to implicit sharing. We should find a way to avoid to not compute the size again and again for more complex data, but that's not really a problem yet.
*/
void BLO_write_shared(BlendWriter *writer,
const void *data,
size_t approximate_size_in_bytes,
const blender::ImplicitSharingInfo *sharing_info,
blender::FunctionRef<void()> write_fn);
/**
* Sometimes different data is written depending on whether the file is saved to disk or used for
* undo. This function returns true when the current file-writing is done for undo.
@ -245,6 +265,26 @@ void BLO_read_pointer_array(BlendDataReader *reader, void **ptr_p);
/* Misc. */
void blo_read_shared_impl(BlendDataReader *reader,
void *data,
const blender::ImplicitSharingInfo **r_sharing_info,
blender::FunctionRef<const blender::ImplicitSharingInfo *()> read_fn);
/**
* Check if there is any shared data for the given data pointer. If yes, return the existing
* sharing-info. If not, call the provided function to actually read the data now.
*/
template<typename T>
const blender::ImplicitSharingInfo *BLO_read_shared(
BlendDataReader *reader,
T **data_ptr,
blender::FunctionRef<const blender::ImplicitSharingInfo *()> read_fn)
{
const blender::ImplicitSharingInfo *sharing_info;
blo_read_shared_impl(reader, *data_ptr, &sharing_info, read_fn);
return sharing_info;
}
int BLO_read_fileversion_get(BlendDataReader *reader);
bool BLO_read_requires_endian_switch(BlendDataReader *reader);
bool BLO_read_data_is_undo(BlendDataReader *reader);

View File

@ -13,9 +13,22 @@
#include "BLI_listbase.h"
#include "BLI_map.hh"
namespace blender {
class ImplicitSharingInfo;
}
struct GHash;
struct Main;
struct Scene;
struct MemFileSharedStorage {
/**
* Maps the data pointer to the sharing info that it is owned by.
*/
blender::Map<const void *, const blender::ImplicitSharingInfo *> map;
~MemFileSharedStorage();
};
struct MemFileChunk {
void *next, *prev;
const char *buf;
@ -35,6 +48,11 @@ struct MemFileChunk {
struct MemFile {
ListBase chunks;
size_t size;
/**
* Some data is not serialized into a new buffer because the undo-step can take ownership of it
* without making a copy. This is faster and requires less memory.
*/
MemFileSharedStorage *shared_storage;
};
struct MemFileWriteData {

View File

@ -4956,6 +4956,32 @@ void BLO_read_pointer_array(BlendDataReader *reader, void **ptr_p)
*ptr_p = final_array;
}
void blo_read_shared_impl(
BlendDataReader *reader,
void *data,
const blender::ImplicitSharingInfo **r_sharing_info,
const blender::FunctionRef<const blender::ImplicitSharingInfo *()> read_fn)
{
if (BLO_read_data_is_undo(reader)) {
if (reader->fd->flags & FD_FLAGS_IS_MEMFILE) {
UndoReader *undo_reader = reinterpret_cast<UndoReader *>(reader->fd->file);
MemFile &memfile = *undo_reader->memfile;
if (memfile.shared_storage) {
/* Check if the data was saved with sharing-info. */
if (const blender::ImplicitSharingInfo *sharing_info =
memfile.shared_storage->map.lookup_default(data, nullptr))
{
/* Add a new owner of the data that is passed to the caller. */
sharing_info->add_user();
*r_sharing_info = sharing_info;
return;
}
}
}
}
*r_sharing_info = read_fn();
}
bool BLO_read_data_is_undo(BlendDataReader *reader)
{
return (reader->fd->flags & FD_FLAGS_IS_MEMFILE);

View File

@ -25,6 +25,7 @@
#include "DNA_listBase.h"
#include "BLI_blenlib.h"
#include "BLI_implicit_sharing.hh"
#include "BLO_readfile.hh"
#include "BLO_undofile.hh"
@ -45,9 +46,19 @@ void BLO_memfile_free(MemFile *memfile)
}
MEM_freeN(chunk);
}
MEM_delete(memfile->shared_storage);
memfile->shared_storage = nullptr;
memfile->size = 0;
}
MemFileSharedStorage::~MemFileSharedStorage()
{
for (const blender::ImplicitSharingInfo *sharing_info : map.values()) {
/* Removing the user makes sure shared data is freed when the undo step was its last owner. */
sharing_info->remove_user_and_delete_if_last();
}
}
void BLO_memfile_merge(MemFile *first, MemFile *second)
{
/* We use this mapping to store the memory buffers from second memfile chunks which are not owned

View File

@ -91,6 +91,7 @@
#include "BLI_blenlib.h"
#include "BLI_endian_defines.h"
#include "BLI_endian_switch.h"
#include "BLI_implicit_sharing.hh"
#include "BLI_link_utils.h"
#include "BLI_linklist.h"
#include "BLI_math_base.h"
@ -1824,6 +1825,33 @@ void BLO_write_string(BlendWriter *writer, const char *data_ptr)
}
}
void BLO_write_shared(BlendWriter *writer,
const void *data,
const size_t approximate_size_in_bytes,
const blender::ImplicitSharingInfo *sharing_info,
const blender::FunctionRef<void()> write_fn)
{
if (data == nullptr) {
return;
}
if (BLO_write_is_undo(writer)) {
MemFile &memfile = *writer->wd->mem.written_memfile;
if (sharing_info != nullptr) {
if (memfile.shared_storage == nullptr) {
memfile.shared_storage = MEM_new<MemFileSharedStorage>(__func__);
}
if (memfile.shared_storage->map.add(data, sharing_info)) {
/* The undo-step takes (shared) ownership of the data, which also makes it immutable. */
sharing_info->add_user();
/* This size is an estimate, but good enough to count data with many users less. */
memfile.size += approximate_size_in_bytes / sharing_info->strong_users();
return;
}
JacquesLucke marked this conversation as resolved Outdated

Pretty sure that these lines should be before the return; statement above?

Pretty sure that these lines should be before the `return;` statement above?
}
}
JacquesLucke marked this conversation as resolved Outdated

Why adding to memfile.size here? Isn't this the 'regular filewrite' case, and size being handled by the internal filewrite logic?

Why adding to `memfile.size` here? Isn't this the 'regular filewrite' case, and size being handled by the internal filewrite logic?
write_fn();
}
bool BLO_write_is_undo(BlendWriter *writer)
{
return writer->wd->use_memfile;