Detect/find proper memchunk for a given ID during undo step writing.
ClosedPublic
Actions

Authored by Bastien Montagne (mont29) on May 29 2020, 2:43 PM.

Details

Reviewers

Brecht Van Lommel (brecht)
Campbell Barton (campbellbarton)

Maniphest Tasks

T60695: Optimized per-datablock global undo

Commits

rBcda15408582e: Undo: Detect/find proper memchunk for a given ID during undo step writing.

Summary

Most of the time current (based on order) system works fine, but when you add
or rename (i.e. re-sort) some ID, every data/memchunk afterward would be out
of sync and hence re-stored in memory (and reported as changed).

Now we are storing the ID's session_uuid in the memchunks, which allows to
actually always find the first memchunk for an already existing ID stored in
previous undo steps, and compare the right memory.

Note that current, based-on-order system is still used almost all of the time,
search in the new ghash is only performed for a few data-blocks (when needed at all).

Code is fairly straightforward I think.

Only design decision here is wether we actually want to generate and use the mapping
from ID's session_uuid to existing memchunks, or if we could just do a ListBase search
instead? GHash is only meaningfull in a few rare cases like addition of a huge number
of IDs within a single undo step, but on the other hand it's not really costly
to generate it either.

Please note that the debugprints are kept on purpose in this patch, they will obviously
be silenced in final commit.

Diff Detail

Repository

rB Blender

Branch

undo-write

Build Status

Buildable 8288
Build 8288: arc lint + arc unit

Event Timeline

Bastien Montagne (mont29) requested review of this revision.May 29 2020, 2:43 PM

Bastien Montagne (mont29) created this revision.

Harbormaster completed remote builds in B8288: Diff 25238.May 29 2020, 2:43 PM

Only design decision here is wether we actually want to generate and use the mapping from ID's session_uuid to existing memchunks, or if we could just do a ListBase search instead? GHash is only meaningfull in a few rare cases like addition of a huge number of IDs within a single undo step, but on the other hand it's not really costly to generate it either.

I think GHash should always be used as in the patch.

source/blender/blenloader/intern/writefile.c
335	To be removed?
511	`start` -> `begin` for consistency with other functions here?
515–535	I don't understand why this tries to reuse the `reference_current_chunk` pointer. Isn't this function called exactly once before writing every ID? Why isn't the implementation of this function just this? if (wd->use_memfile) { wd->mem.current_id_session_uuid = id->session_uuid; if (wd->mem.id_session_uuid_mapping) { wd->mem.reference_current_chunk = BLI_ghash_lookup(wd->mem.id_session_uuid_mapping, POINTER_FROM_UINT(id->session_uuid)); } }

This revision now requires changes to proceed.Jun 2 2020, 11:56 AM

Merge branch 'master' into undo-write
Minor cleanup.

Harbormaster completed remote builds in B8323: Diff 25385.Jun 2 2020, 12:47 PM

Bastien Montagne (mont29) added inline comments.Jun 2 2020, 12:47 PM

source/blender/blenloader/intern/writefile.c
515–535	Because this avoids us 99% of GHash lookups, as in almost all cases the last memchunk as set by `BLO_memfile_chunk_add()` is directly usable for next ID. This patch basically adds the ghash system as backup for when existing search system fails (due to reordering or insertions/deletions of IDs in their list).