Page MenuHome

readfile: Move refcounting to libquery.
ClosedPublic

Authored by Bastien Montagne (mont29) on Feb 18 2020, 3:59 PM.

Details

Summary

Having that extra ID users handling at readfile level, besides generic
one ensured by libquery, has been something bothering me for a long time
(had to fix my share of bugs due to mismatches between those two areas).

Further more, work on undo speedup will require even more complex ID
refcount management if we want to keep it in readfile.c area.

So idea is instead to generalize what we did for linked data already
when undoing: recompute properly usercount numbers after liblink step,
for all IDs.

Note that extra time required here is neglectable in a whole .blend file
reading (few extra milliseconds when loading a full production scene
e.g.).

Diff Detail

Repository
rB Blender

Event Timeline

Bastien Montagne (mont29) retitled this revision from readfile: Quick experiment with refcounting moved to libquery. to readfile: Move refcounting to libquery..Feb 18 2020, 3:59 PM
Bastien Montagne (mont29) added inline comments.
source/blender/blenloader/intern/readfile.c
1793–1794

Note that that one is kept because used by deprecated pointers (IPO ones)... Not sure how important those are currently.

The main weakness I see is that do_versions_after_linking will be running without valid user counts. I can't immediately think of any current versioning code that would fail because of this, so probably it's fine?

For IPOs, it's fine if those have zero users and get garbage collected at some point after versioning. I tried opening a really old file with IPOs, pathJumper.blend from here:
https://download.blender.org/demo/old_demos/demo225.zip

After rBed8aa154 that file opens. There are some asserts about IPO user count, but they are already there before this patch, and ignoring those the animation appears to be loaded correctly.

So probably we can just get rid of newlibadr_us entirely? I can't think of a reason why IPOs would need user count for versioning more so than other datablocks.

source/blender/blenloader/intern/readfile.c
1793–1794

If we do need this function still, probably best to rename this to something like newlibadr_us_deprecated and add a comment.

9755

This comment is confusing, what can't we do here? Is this code supposed to be commented out?

Maybe at some point in the future we can get rid of most of the lib link code (exception versioning/deprecated stuff), and automate newlibadr with libquery too.

The main weakness I see is that do_versions_after_linking will be running without valid user counts. I can't immediately think of any current versioning code that would fail because of this, so probably it's fine?

Yes, we could just call the usercount computing func twice, but for now I think it indeed does not matter…

For IPOs, it's fine if those have zero users and get garbage collected at some point after versioning. I tried opening a really old file with IPOs, pathJumper.blend from here:
https://download.blender.org/demo/old_demos/demo225.zip

After rBed8aa154 that file opens. There are some asserts about IPO user count, but they are already there before this patch, and ignoring those the animation appears to be loaded correctly.

Great, thanks for checking that!

So probably we can just get rid of newlibadr_us entirely? I can't think of a reason why IPOs would need user count for versioning more so than other datablocks.

Agreed.

Maybe at some point in the future we can get rid of most of the lib link code (exception versioning/deprecated stuff), and automate newlibadr with libquery too.

That would be nice indeed.

source/blender/blenloader/intern/readfile.c
1793–1794

Yeah, wouldn't mind removing it completely too, will try that then.

9755

We cannot call that BKE_main_id_refcount_recompute function here in undo case (i.e. when fd->memfile != NULL, because then bmain is not in its final state, some UI-related data-blocks are still to be properly swapped later in the code path.

source/blender/blenloader/intern/readfile.c
9755

Ah ok, I was a bit confused by the wording. Maybe leave out the word "here" from that sentence.

Bastien Montagne (mont29) marked 3 inline comments as done.
  • Merge branch 'master' into readfile-rework-refcount-handling
  • Merge branch 'master' into readfile-rework-refcount-handling
  • get rid of all remaining usage of newlibadr_us in readfile code.
This revision is now accepted and ready to land.Feb 19 2020, 10:33 AM