From 2ec1b6887dc04671905bd2fab430d68616228dfc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sybren=20A=2E=20St=C3=BCvel?= Date: Mon, 6 Jan 2025 10:58:56 +0100 Subject: [PATCH] Core: move short-lived `ID_TAG_ID_LINK_PLACEHOLDER` to runtime struct Move the `ID_TAG_ID_LINK_PLACEHOLDER` bit of `id->tag` to `id->runtime.readfile_data->tags.is_id_link_placeholder`. It also introduces the necessary stucts and allocation/freeing code. Old code: ```cpp if (id_tag & ID_TAG_ID_LINK_PLACEHOLDER) { ``` New code: ```cpp if (readfile_id_runtime_tags(id).is_id_link_placeholder) { ``` where `readfile_id_runtime_tags(id)` is a getter for `id->runtime.readfile_data->tags` that is null-safe for `id->runtime.readfile_data`. The `readfile_data` is not allocated in these cases: 1. When reading undo steps, because that doesn't have to deal with versioning or linking (which are the sole purposes for this struct). 2. When linking from another file (for example from the 'Link...' operator). The just-linked IDs will have the `readfile_data` struct, but already-loaded IDs will already have had those freed. No functional changes intended. Pull Request: https://projects.blender.org/blender/blender/pulls/132169 Design Task: #131695 --- source/blender/blenkernel/BKE_lib_id.hh | 7 + source/blender/blenkernel/intern/blendfile.cc | 1 + .../intern/blendfile_link_append.cc | 4 + .../blenkernel/intern/lib_id_delete.cc | 13 ++ source/blender/blenloader/BLO_readfile.hh | 28 ++++ source/blender/blenloader/intern/readfile.cc | 141 +++++++++++++++--- source/blender/makesdna/DNA_ID.h | 15 +- 7 files changed, 180 insertions(+), 29 deletions(-) diff --git a/source/blender/blenkernel/BKE_lib_id.hh b/source/blender/blenkernel/BKE_lib_id.hh index e335c8575c1..da9dddfa62f 100644 --- a/source/blender/blenkernel/BKE_lib_id.hh +++ b/source/blender/blenkernel/BKE_lib_id.hh @@ -395,8 +395,15 @@ enum { LIB_ID_FREE_NO_NAMEMAP_REMOVE = 1 << 10, }; +/** + * Low-level ID freeing functions. + * + * \note These functions do NOT cover embedded IDs. Those are managed by the + * owning ID, and are typically allocated/freed from the IDType callbacks. + */ void BKE_libblock_free_datablock(ID *id, int flag) ATTR_NONNULL(); void BKE_libblock_free_data(ID *id, bool do_id_user) ATTR_NONNULL(); +void BKE_libblock_free_runtime_data(ID *id) ATTR_NONNULL(); /** * In most cases #BKE_id_free_ex handles this, when lower level functions are called directly diff --git a/source/blender/blenkernel/intern/blendfile.cc b/source/blender/blenkernel/intern/blendfile.cc index 394a7d3de8f..0a444bf14c0 100644 --- a/source/blender/blenkernel/intern/blendfile.cc +++ b/source/blender/blenkernel/intern/blendfile.cc @@ -1154,6 +1154,7 @@ static void setup_app_data(bContext *C, * and/or needs to operate over the whole Main data-base * (versioning done in file reading code only operates on a per-library basis). */ BLO_read_do_version_after_setup(bmain, nullptr, reports); + BLO_readfile_id_runtime_data_free_all(*bmain); } bmain->recovered = false; diff --git a/source/blender/blenkernel/intern/blendfile_link_append.cc b/source/blender/blenkernel/intern/blendfile_link_append.cc index 496df8053b8..2fabe1818d3 100644 --- a/source/blender/blenkernel/intern/blendfile_link_append.cc +++ b/source/blender/blenkernel/intern/blendfile_link_append.cc @@ -1432,6 +1432,8 @@ void BKE_blendfile_append(BlendfileLinkAppendContext *lapp_context, ReportList * BlendFileReadReport bf_reports{}; bf_reports.reports = reports; BLO_read_do_version_after_setup(bmain, lapp_context, &bf_reports); + + BLO_readfile_id_runtime_data_free_all(*bmain); } /** \} */ @@ -1600,6 +1602,8 @@ void BKE_blendfile_link(BlendfileLinkAppendContext *lapp_context, ReportList *re BlendFileReadReport bf_reports{}; bf_reports.reports = reports; BLO_read_do_version_after_setup(lapp_context->params->bmain, lapp_context, &bf_reports); + + BLO_readfile_id_runtime_data_free_all(*lapp_context->params->bmain); } } diff --git a/source/blender/blenkernel/intern/lib_id_delete.cc b/source/blender/blenkernel/intern/lib_id_delete.cc index 27ea11e18e7..35f6423bb9d 100644 --- a/source/blender/blenkernel/intern/lib_id_delete.cc +++ b/source/blender/blenkernel/intern/lib_id_delete.cc @@ -33,6 +33,8 @@ #include "BKE_main.hh" #include "BKE_main_namemap.hh" +#include "BLO_readfile.hh" + #include "lib_intern.hh" #include "DEG_depsgraph.hh" @@ -64,6 +66,8 @@ void BKE_libblock_free_data(ID *id, const bool do_id_user) MEM_freeN(id->library_weak_reference); } + BKE_libblock_free_runtime_data(id); + BKE_animdata_free(id, do_id_user); } @@ -81,6 +85,15 @@ void BKE_libblock_free_datablock(ID *id, const int /*flag*/) BLI_assert_msg(0, "IDType Missing IDTypeInfo"); } +void BKE_libblock_free_runtime_data(ID *id) +{ + /* During "normal" file loading this data is released when versioning ends. Some versioning code + * also deletes IDs, though. For example, in the startup blend file, brushes that were replaced + * by assets are deleted. This means that the regular "delete this ID" flow (aka this code here) + * also needs to free this data. */ + BLO_readfile_id_runtime_data_free(*id); +} + static int id_free(Main *bmain, void *idv, int flag, const bool use_flag_from_idtag) { ID *id = static_cast(idv); diff --git a/source/blender/blenloader/BLO_readfile.hh b/source/blender/blenloader/BLO_readfile.hh index 501d58cee27..fdd10845f6e 100644 --- a/source/blender/blenloader/BLO_readfile.hh +++ b/source/blender/blenloader/BLO_readfile.hh @@ -532,3 +532,31 @@ BlendThumbnail *BLO_thumbnail_from_file(const char *filepath); * \return The file version */ short BLO_version_from_file(const char *filepath); + +/** + * Runtime structure on `ID.runtime.readfile_data` that is available during the readfile process. + * + * This is intended for short-lived data, for example for things that are detected in an early + * phase of versioning that should be used in a later stage of versioning. + * + * \note This is NOT allocated when 'reading' an undo step, as that doesn't have to deal with + * versioning, linking, and the other stuff that this struct was meant for. + */ +struct ID_Readfile_Data { + struct Tags { + bool is_id_link_placeholder : 1; + } tags; +}; + +/** + * Free the ID_Readfile_Data of all IDs in this bmain and all their embedded IDs. + * + * This is typically called at the end of the versioning process, as after that + * `ID.runtime.readfile_data` should no longer be needed. + */ +void BLO_readfile_id_runtime_data_free_all(Main &bmain); + +/** + * Free the ID_Readfile_Data of this ID. Does _not_ deal with embeded IDs. + */ +void BLO_readfile_id_runtime_data_free(ID &id); diff --git a/source/blender/blenloader/intern/readfile.cc b/source/blender/blenloader/intern/readfile.cc index d0bb0936541..f8e469a9052 100644 --- a/source/blender/blenloader/intern/readfile.cc +++ b/source/blender/blenloader/intern/readfile.cc @@ -2052,8 +2052,12 @@ static void direct_link_id_override_property(BlendDataReader *reader, } } -static void direct_link_id_common( - BlendDataReader *reader, Library *current_library, ID *id, ID *id_old, const int id_tag); +static void direct_link_id_common(BlendDataReader *reader, + Library *current_library, + ID *id, + ID *id_old, + int id_tag, + ID_Readfile_Data::Tags id_read_tags); static void direct_link_id_embedded_id(BlendDataReader *reader, Library *current_library, @@ -2069,7 +2073,8 @@ static void direct_link_id_embedded_id(BlendDataReader *reader, (ID *)*nodetree, id_old != nullptr ? (ID *)blender::bke::node_tree_from_id(id_old) : nullptr, - 0); + 0, + ID_Readfile_Data::Tags{}); blender::bke::node_tree_blend_read_data(reader, id, *nodetree); } @@ -2082,7 +2087,8 @@ static void direct_link_id_embedded_id(BlendDataReader *reader, &scene->master_collection->id, id_old != nullptr ? &((Scene *)id_old)->master_collection->id : nullptr, - 0); + 0, + ID_Readfile_Data::Tags{}); BKE_collection_blend_read_data(reader, scene->master_collection, &scene->id); } } @@ -2145,8 +2151,60 @@ static int direct_link_id_restore_recalc(const FileData *fd, return recalc; } -static void direct_link_id_common( - BlendDataReader *reader, Library *current_library, ID *id, ID *id_old, const int id_tag) +static void readfile_id_runtime_data_ensure(ID &id) +{ + if (id.runtime.readfile_data) { + return; + } + id.runtime.readfile_data = MEM_cnew(__func__); +} + +/** + * Return `id.runtime.readfile_data->tags` if the `readfile_data` is allocated, + * otherwise return an all-zero set of tags. + */ +static ID_Readfile_Data::Tags readfile_id_runtime_tags(ID *id) +{ + if (!id->runtime.readfile_data) { + return ID_Readfile_Data::Tags{}; + } + return id->runtime.readfile_data->tags; +} + +void BLO_readfile_id_runtime_data_free(ID &id) +{ + MEM_SAFE_FREE(id.runtime.readfile_data); +} + +void BLO_readfile_id_runtime_data_free_all(Main &bmain) +{ + ID *id; + FOREACH_MAIN_ID_BEGIN (&bmain, id) { + /* Handle the ID itself. */ + BLO_readfile_id_runtime_data_free(*id); + + /* Handle its embedded IDs, because they do not get referenced by bmain. */ + if (GS(id->name) == ID_SCE) { + Collection *collection = reinterpret_cast(id)->master_collection; + if (collection) { + BLO_readfile_id_runtime_data_free(collection->id); + } + } + + bNodeTree *node_tree = blender::bke::node_tree_from_id(id); + if (node_tree) { + BLO_readfile_id_runtime_data_free(node_tree->id); + } + } + FOREACH_MAIN_ID_END; +} + +static void direct_link_id_common(BlendDataReader *reader, + Library *current_library, + ID *id, + ID *id_old, + const int id_tag, + const ID_Readfile_Data::Tags id_read_tags) { if (!BLO_read_data_is_undo(reader)) { /* When actually reading a file, we do want to reset/re-generate session UIDS. @@ -2184,7 +2242,18 @@ static void direct_link_id_common( BLO_read_struct(reader, LibraryWeakReference, &id->library_weak_reference); } - if (id_tag & ID_TAG_ID_LINK_PLACEHOLDER) { + if (!BLO_read_data_is_undo(reader)) { + /* Reset the runtime data, as there were versions of Blender that did not do + * this before writing to disk. */ + memset(&id->runtime, 0, sizeof(id->runtime)); + + /* Only track the readfile tags when loading from disk. During 'undo' there is no versioning, + * no linking, etc. so there is no need for these flags. */ + readfile_id_runtime_data_ensure(*id); + id->runtime.readfile_data->tags = id_read_tags; + } + + if (readfile_id_runtime_tags(id).is_id_link_placeholder) { /* For placeholder we only need to set the tag and properly initialize generic ID fields above, * no further data to read. */ return; @@ -2481,7 +2550,12 @@ static void placeholders_ensure_valid(Main *bmain) } } -static bool direct_link_id(FileData *fd, Main *main, const int tag, ID *id, ID *id_old) +static bool direct_link_id(FileData *fd, + Main *main, + const int tag, + const ID_Readfile_Data::Tags id_read_tags, + ID *id, + ID *id_old) { BlendDataReader reader = {fd}; /* Sharing is only allowed within individual data-blocks currently. The clearing is done @@ -2489,9 +2563,9 @@ static bool direct_link_id(FileData *fd, Main *main, const int tag, ID *id, ID * reader.shared_data_by_stored_address.clear(); /* Read part of datablock that is common between real and embedded datablocks. */ - direct_link_id_common(&reader, main->curlib, id, id_old, tag); + direct_link_id_common(&reader, main->curlib, id, id_old, tag, id_read_tags); - if (tag & ID_TAG_ID_LINK_PLACEHOLDER) { + if (readfile_id_runtime_tags(id).is_id_link_placeholder) { /* For placeholder we only need to set the tag, no further data to read. */ id->tag = tag; return true; @@ -3001,9 +3075,11 @@ static BHead *read_libblock(FileData *fd, * to be done still. */ id_tag |= (ID_TAG_NEED_LINK | ID_TAG_NEW); + ID_Readfile_Data::Tags id_read_tags{}; + if (bhead->code == ID_LINK_PLACEHOLDER) { /* Read placeholder for linked datablock. */ - id_tag |= ID_TAG_ID_LINK_PLACEHOLDER; + id_read_tags.is_id_link_placeholder = true; if (placeholder_set_indirect_extern) { if (id->flag & ID_FLAG_INDIRECT_WEAK_LINK) { @@ -3014,7 +3090,7 @@ static BHead *read_libblock(FileData *fd, } } - direct_link_id(fd, main, id_tag, id, id_old); + direct_link_id(fd, main, id_tag, id_read_tags, id, id_old); if (main->id_map != nullptr) { BKE_main_idmap_insert_id(main->id_map, id); @@ -3026,7 +3102,7 @@ static BHead *read_libblock(FileData *fd, /* Read datablock contents. * Use convenient malloc name for debugging and better memory link prints. */ bhead = read_data_into_datamap(fd, bhead, blockname, id_type_index); - const bool success = direct_link_id(fd, main, id_tag, id, id_old); + const bool success = direct_link_id(fd, main, id_tag, id_read_tags, id, id_old); oldnewmap_clear(fd->datamap); if (!success) { @@ -4148,7 +4224,7 @@ static void expand_doit_library(void *fdhandle, Main *mainvar, void *old) else { /* Convert any previously read weak link to regular link * to signal that we want to read this data-block. */ - if (id->tag & ID_TAG_ID_LINK_PLACEHOLDER) { + if (readfile_id_runtime_tags(id).is_id_link_placeholder) { id->flag &= ~ID_FLAG_INDIRECT_WEAK_LINK; } @@ -4185,9 +4261,10 @@ static void expand_doit_library(void *fdhandle, Main *mainvar, void *old) id_sort_by_name(which_libbase(mainvar, GS(id->name)), id, static_cast(id->prev)); } else { - /* Convert any previously read weak link to regular link - * to signal that we want to read this data-block. */ - if (id->tag & ID_TAG_ID_LINK_PLACEHOLDER) { + /* Convert any previously read weak link to regular link to signal that we want to read this + * data-block. Note that this function also visits already-loaded data-blocks, and thus their + * `readfile_data` field might already have been freed. */ + if (readfile_id_runtime_tags(id).is_id_link_placeholder) { id->flag &= ~ID_FLAG_INDIRECT_WEAK_LINK; } @@ -4603,7 +4680,9 @@ static int has_linked_ids_to_read(Main *mainvar) while (a--) { LISTBASE_FOREACH (ID *, id, lbarray[a]) { - if ((id->tag & ID_TAG_ID_LINK_PLACEHOLDER) && !(id->flag & ID_FLAG_INDIRECT_WEAK_LINK)) { + if (readfile_id_runtime_tags(id).is_id_link_placeholder && + !(id->flag & ID_FLAG_INDIRECT_WEAK_LINK)) + { return true; } } @@ -4634,7 +4713,7 @@ static void read_library_linked_id( library_parent_filepath(mainvar->curlib)); } - id->tag &= ~ID_TAG_ID_LINK_PLACEHOLDER; + id->runtime.readfile_data->tags.is_id_link_placeholder = false; id->flag &= ~ID_FLAG_INDIRECT_WEAK_LINK; if (bhead) { @@ -4679,7 +4758,9 @@ static void read_library_linked_ids(FileData *basefd, while (id) { ID *id_next = static_cast(id->next); - if ((id->tag & ID_TAG_ID_LINK_PLACEHOLDER) && !(id->flag & ID_FLAG_INDIRECT_WEAK_LINK)) { + if (readfile_id_runtime_tags(id).is_id_link_placeholder && + !(id->flag & ID_FLAG_INDIRECT_WEAK_LINK)) + { BLI_remlink(lbarray[a], id); if (mainvar->id_map != nullptr) { BKE_main_idmap_remove_id(mainvar->id_map, id); @@ -4704,6 +4785,19 @@ static void read_library_linked_ids(FileData *basefd, * libraries since multiple might be referencing this ID. */ change_link_placeholder_to_real_ID_pointer(mainlist, basefd, id, realid); + /* Transfer the readfile data from the placeholder to the real ID, but + * only if the real ID has no readfile data yet. The same realid may be + * referred to by multiple placeholders. */ + if (realid && !realid->runtime.readfile_data) { + realid->runtime.readfile_data = id->runtime.readfile_data; + id->runtime.readfile_data = nullptr; + } + + /* The 'readfile' runtime data needs to be freed here, as this ID placeholder does not go + * through versioning (the usual place where this data is freed). Since `id` is not a real + * ID, this shouldn't follow any pointers to embedded IDs. */ + BLO_readfile_id_runtime_data_free(*id); + MEM_freeN(id); } id = id_next; @@ -4725,7 +4819,12 @@ static void read_library_clear_weak_links(FileData *basefd, ListBase *mainlist, while (id) { ID *id_next = static_cast(id->next); - if ((id->tag & ID_TAG_ID_LINK_PLACEHOLDER) && (id->flag & ID_FLAG_INDIRECT_WEAK_LINK)) { + + /* This function also visits already-loaded data-blocks, and thus their + * `readfile_data` field might already have been freed. */ + if (readfile_id_runtime_tags(id).is_id_link_placeholder && + (id->flag & ID_FLAG_INDIRECT_WEAK_LINK)) + { CLOG_INFO(&LOG, 3, "Dropping weak link to '%s'", id->name); change_link_placeholder_to_real_ID_pointer(mainlist, basefd, id, nullptr); BLI_freelinkN(lbarray[a], id); diff --git a/source/blender/makesdna/DNA_ID.h b/source/blender/makesdna/DNA_ID.h index 82b7a2d57d8..f3b4535ea7f 100644 --- a/source/blender/makesdna/DNA_ID.h +++ b/source/blender/makesdna/DNA_ID.h @@ -29,6 +29,7 @@ extern "C" { struct FileData; struct GHash; struct ID; +struct ID_Readfile_Data; struct Library; struct PackedFile; struct UniqueName_Map; @@ -402,7 +403,12 @@ typedef struct ID_Runtime { * are not owned by any specific depsgraph and thus this pointer is null for those. */ struct Depsgraph *depsgraph; - void *_pad; + + /** + * This data is only allocated & used during the readfile process. After that, the memory is + * freed and the pointer set to `nullptr`. + */ + struct ID_Readfile_Data *readfile_data; } ID_Runtime; typedef struct ID { @@ -872,13 +878,6 @@ enum { * RESET_AFTER_USE */ ID_TAG_NEED_EXPAND = 1 << 14, - /** - * Tag used internally in `readfile.cc`, to mark ID placeholders for linked data-blocks needing - * to be read. - * - * RESET_AFTER_USE - */ - ID_TAG_ID_LINK_PLACEHOLDER = 1 << 15, /** * Tag used internally in `readfile.cc`, to mark IDs needing to be 'lib-linked', i.e. to get * their pointers to other data-blocks updated from the 'UID' values stored in `.blend` files to